On Tue, 10 Jan 2017, Kees Cook wrote:
> On Tue, Jan 10, 2017 at 10:28 AM, Julia Lawall <julia.law...@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
