On Thursday 10 June 2010, Steven A. Falco wrote:
> > The ifreq structure passed into the ndo_ioctl function is in kernel
> > space, it gets copied there by net/core/dev.c:dev_ioctl().
> > emac_ioctl only accesses the data in that structure, so a copy_from_user
> > is wrong here as far as I can tell.
>
> net/core/dev.c:dev_ioctl() does a copy_from_user on the overall structure,
> but the structure contains a union, one member of which is an embedded
> pointer to an array. This pointer member is only used in the case of these
> two ioctls. Other ioctls use different union members, which are not pointers.
Still unconvinced.
I don't see anywhere in the structure where we actually use a
pointer from ifr_ifru. The if_mii function is defined as
static inline struct mii_ioctl_data *if_mii(struct ifreq *rq)
{
return (struct mii_ioctl_data *) &rq->ifr_ifru;
}
That just returns a pointer to the ifr_ifru member itself,
it does not read a __user pointer. Note how if_mii even
returns a kernel pointer (struct mii_ioctl_data *), not
a struct mii_ioctl_data __user *. You even added a cast
that otherwise should not be needed.
> As I understand it, the copy_from_user in dev_ioctl does not recursively
> copy the array. In fact it could not do so, because the pointer to array
> is only 4 bytes long, while the array itself is 8 bytes long - so it would
> not fit. I.e., dev_ioctl would have to allocate storage, and do a second
> copy_from_user to retrieve the array. It would have to clean up after the
> emac_ioctl ran. And it would have to do this only for these specific
> ioctl calls which use the array pointer in the union.
>
> Also, the result has to be returned to the user in the same array, which
> needs a copy_to_user of the array data, which is also not done in dev_ioctl.
There is no array, and no pointer in struct ifreq
> So I think this second copy_from/copy_to needs to be done somewhere. I
> added it in the emac_ioctl because that is where the command is fully
> decoded. It also avoids the problem of allocating space for the copied
> array. But other fixes are certainly possible as well, which is why I am
> not sure I've hit on the "proper" fix.
No other device driver implementing SIOCGMIIREG does any of this,
so I'm pretty sure it's not the proper fix.
> Thanks very much for reviewing - please let me know if my explanation is
> unclear, or if you see a better way to fix this problem.
In theory, your patch should break the code. Doing a direct access
in place of a copy_to/from_user is a security hole but should still work,
while adding a copy_to/from_user on a kernel pointer should always result
in an EFAULT.
Arnd
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
