From: Arnd Bergmann <arnd@arndb•de>
To: "Steven A. Falco" <sfalco@harris•com>
Cc: linuxppc-dev@lists•ozlabs.org
Subject: Re: [PATCH][RFC] ibm_newemac and SIOCGMIIREG
Date: Thu, 10 Jun 2010 19:03:43 +0200 [thread overview]
Message-ID: <201006101903.43882.arnd@arndb.de> (raw)
In-Reply-To: <4C11046D.4080205@harris.com>
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
next prev parent reply other threads:[~2010-06-10 17:03 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-06-10 14:00 [PATCH][RFC] ibm_newemac and SIOCGMIIREG Steven A. Falco
2010-06-10 14:31 ` Arnd Bergmann
2010-06-10 15:27 ` Steven A. Falco
2010-06-10 17:03 ` Arnd Bergmann [this message]
2010-06-10 19:47 ` Steven A. Falco
2010-06-10 20:35 ` Arnd Bergmann
2010-06-10 21:26 ` Steven A. Falco
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=201006101903.43882.arnd@arndb.de \
--to=arnd@arndb$(echo .)de \
--cc=linuxppc-dev@lists$(echo .)ozlabs.org \
--cc=sfalco@harris$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox