Messages in this thread | | | Date | Tue, 10 Jan 2017 20:30:29 +0100 (CET) | From | Julia Lawall <> | Subject | Re: [RFC] coccicheck: add a test for repeat memory fetches |
| |
On Tue, 10 Jan 2017, Kees Cook wrote:
> On Tue, Jan 10, 2017 at 10:28 AM, Julia Lawall <julia.lawall@lip6.fr> wrote: > >> +./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2159 > >> +./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2257 > >> +./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2302 > >> +./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2342 > >> +./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2365 > >> +./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2406 > >> +./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2439 > >> +./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2491 > > > > Do you want the above results? They have the form: > > > > if (copy_from_user(&t, useraddr, sizeof(t))) > > > > My reasoning was that there could be no problem here, because the size is > > the size of the destination structure. It doesn't depend on user level data. > > They're likely false positives, but it does follow the pattern of > reading the same userspace location twice: > > if (copy_from_user(&cmd, useraddr, sizeof(cmd))) > return -EFAULT; > > switch (cmd) { > case CHELSIO_SET_QSET_PARAMS:{ > int i; > struct qset_params *q; > struct ch_qset_params t; > int q1 = pi->first_qset; > int nqsets = pi->nqsets; > > if (!capable(CAP_NET_ADMIN)) > return -EPERM; > if (copy_from_user(&t, useraddr, sizeof(t))) > return -EFAULT; > > If there is any logic that examines cmd (u32) and operates on t > (struct ch_qset_params), there could be a flaw. It doesn't look like > it here, but a "correct" version of this would be: > > if (copy_from_user(&t, useraddr, sizeof(t))) > return -EFAULT; > t.cmd = cmd;
OK, I'm fine with putting them all back.
For another issue, what about code like the following:
if (copy_from_user(&u_cmd, arg, sizeof(u_cmd))) return -EFAULT;
if ((u_cmd.outsize > EC_MAX_MSG_BYTES) || (u_cmd.insize > EC_MAX_MSG_BYTES)) return -EINVAL;
s_cmd = kmalloc(sizeof(*s_cmd) + max(u_cmd.outsize, u_cmd.insize), GFP_KERNEL); if (!s_cmd) return -ENOMEM;
if (copy_from_user(s_cmd, arg, sizeof(*s_cmd) + u_cmd.outsize)) { ret = -EFAULT; goto exit; }
It doesn't actually test sizeof(*s_cmd) + u_cmd.outsize, but it does test u_cmd.outsize > EC_MAX_MSG_BYTES, and presumably that test accounts for the extra sizeof(*s_cmd) value.
julia
| |