[PATCH][RFC] ibm_newemac and SIOCGMIIREG

[PATCH][RFC] ibm_newemac and SIOCGMIIREG

[Steven A. Falco]

SIOCGMIIREG and SIOCSMIIREG access a user data structure via a void
pointer to user space.  So, we need copy_from_user and copy_to_user
to move the data.

Signed-off-by: Steven A. Falco <sfalco@harris.com>

---

I believe there is a bug in the way the ibm_newemac driver handles the
SIOCGMIIREG (and SIOCSMIIREG) ioctl.  The problem is that emac_ioctl
is handed a "struct ifreq *rq" which contains a user-land pointer to
an array of 16-bit integers.

However, emac_ioctl directly accesses the data, which doesn't work.
I added the following patch to copy the data in and out.

Please note that this patch was tested in an older kernel (2.6.30)
because that is what we are using on our custom hardware.  I think
this is still a problem in the current code, but I'd like reviewers
to take a look, to be sure.

--- drivers/net/ibm_newemac/core.c	2010-06-09 19:57:26.000000000 -0400
+++ /home/sfalco/core.c	2010-06-10 09:38:22.000000000 -0400
@@ -2218,6 +2218,7 @@
 {
 	struct emac_instance *dev = netdev_priv(ndev);
 	struct mii_ioctl_data *data = if_mii(rq);
+	struct mii_ioctl_data user_data;
 
 	DBG(dev, "ioctl %08x" NL, cmd);
 
@@ -2229,13 +2230,19 @@
 		data->phy_id = dev->phy.address;
 		/* Fall through */
 	case SIOCGMIIREG:
-		data->val_out = emac_mdio_read(ndev, dev->phy.address,
-					       data->reg_num);
+		if (copy_from_user(user_data, (char __user *)data, sizeof(user_data)))
+			return -EFAULT;
+		user_data->val_out = emac_mdio_read(ndev, dev->phy.address,
+					       user_data->reg_num);
+		if (copy_to_user((char __user *)rq->ifr_data, user_data, sizeof(user_data)))
+			return -EFAULT;
 		return 0;
 
 	case SIOCSMIIREG:
-		emac_mdio_write(ndev, dev->phy.address, data->reg_num,
-				data->val_in);
+		if (copy_from_user(user_data, (char __user *)data, sizeof(user_data)))
+			return -EFAULT;
+		emac_mdio_write(ndev, dev->phy.address, user_data->reg_num,
+				user_data->val_in);
 		return 0;
 	default:
 		return -EOPNOTSUPP;

Re: [PATCH][RFC] ibm_newemac and SIOCGMIIREG

[Arnd Bergmann]

On Thursday 10 June 2010, Steven A. Falco wrote:
> I believe there is a bug in the way the ibm_newemac driver handles the
> SIOCGMIIREG (and SIOCSMIIREG) ioctl.  The problem is that emac_ioctl
> is handed a "struct ifreq *rq" which contains a user-land pointer to
> an array of 16-bit integers.

Did you actually see a bug here, or just think that this could be
a problem?

> However, emac_ioctl directly accesses the data, which doesn't work.
> I added the following patch to copy the data in and out.
> 
> Please note that this patch was tested in an older kernel (2.6.30)
> because that is what we are using on our custom hardware.  I think
> this is still a problem in the current code, but I'd like reviewers
> to take a look, to be sure.

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.

	Arnd

Re: [PATCH][RFC] ibm_newemac and SIOCGMIIREG

[Steven A. Falco]

On 06/10/2010 10:31 AM, Arnd Bergmann wrote:
> 
> On Thursday 10 June 2010, Steven A. Falco wrote:
>> I believe there is a bug in the way the ibm_newemac driver handles the
>> SIOCGMIIREG (and SIOCSMIIREG) ioctl.  The problem is that emac_ioctl
>> is handed a "struct ifreq *rq" which contains a user-land pointer to
>> an array of 16-bit integers.
> 
> Did you actually see a bug here, or just think that this could be
> a problem?

I did see a problem.  I tried using the ioctl, and I didn't get the
correct result.  I then added some printk in the driver, and saw that
garbage was being passed in the data array.

I added the copy_from/copy_to, and the ioctl started working.

> 
>> However, emac_ioctl directly accesses the data, which doesn't work.
>> I added the following patch to copy the data in and out.
>>
>> Please note that this patch was tested in an older kernel (2.6.30)
>> because that is what we are using on our custom hardware.  I think
>> this is still a problem in the current code, but I'd like reviewers
>> to take a look, to be sure.
> 
> 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.

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.

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.

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.

	Steve

> 
> 	Arnd


-- 
A: Because it makes the logic of the discussion difficult to follow.
Q: Why shouldn't I top post?
A: No.
Q: Should I top post?

Re: [PATCH][RFC] ibm_newemac and SIOCGMIIREG

[Arnd Bergmann]

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

Re: [PATCH][RFC] ibm_newemac and SIOCGMIIREG

[Steven A. Falco]

On 06/10/2010 01:03 PM, Arnd Bergmann wrote:
> 
> 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.

Ok - here is the user-space program I am using to test this.  It simply
uses the ioctl to peek and poke the phy.  If I run it on an unmodified
kernel, and I read the phy ID registers, i.e. registers 2 and 3, I get:

# ./phy -r -a 2
2: 0000
# ./phy -r -a 3
3: 0000

which is clearly wrong.  Once I load my modified kernel, I get:

# ./phy -a 2
2: 0022
# ./phy -a 3
3: 1512

which is correct for the phy on my board (i.e. phy id is 00221512).  If you
could try this program on a sequoia or similar, I'd be interested in whether
you see the problem.

------ cut here ------

#define _GNU_SOURCE

#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>

#include <getopt.h>
#include <string.h>
#include <unistd.h>

#include <linux/mii.h>
#include <linux/sockios.h>

#include <net/if.h>

#include <sys/ioctl.h>
#include <sys/socket.h>

#define READING 0
#define WRITING 1

void use()
{
    fprintf(stderr, "\nphy [OPTIONS]\n\n");
    fprintf(stderr, "  -a address (register within phy to access)\n");
    fprintf(stderr, "  -d device (something like eth0, eth1, etc.)\n");
    fprintf(stderr, "  -r (read the register)\n");
    fprintf(stderr, "  -w (write the register)\n");
    fprintf(stderr, "  -v value (the value to write)\n");
    fprintf(stderr, "  -h (this help message)\n");
}

int
main(int argc, char *argv[])
{
    int		    fd;
    struct ifreq    request;
    struct mii_ioctl_data data;
    int		    opt;
    int		    dir = READING;
    char	    *pDev = "eth0";
    uint32_t	    addr = 0;
    uint32_t	    value = 0;

    while((opt = getopt(argc, argv, "a:d:hrv:w")) != -1) {
	switch(opt) {
	    case 'a':
		addr = strtoul(optarg, NULL, 16);
		break;

	    case 'd':
		pDev = optarg;
		break;

	    case 'h':
		use();
		exit(0);

	    case 'r':
		dir = READING;
		break;

	    case 'v':
		value = strtoul(optarg, NULL, 16);
		break;

	    case 'w':
		dir = WRITING;
		break;

	    default:
		fprintf(stderr, "bad option\n");
		use();
		exit(1);
	}
    }

    fd = socket(AF_INET, SOCK_DGRAM, 0);
    if(fd < 0) {
	fprintf(stderr, "no sock\n");
	exit(1);
    }

    memset(&request, 0, sizeof(request));
    memset(&data, 0, sizeof(data));
    strncpy(request.ifr_name, pDev, sizeof(request.ifr_name));
    data.reg_num = addr;
    y

    if(dir == READING) {
	if(ioctl(fd, SIOCGMIIREG, &request) < 0) {
	    fprintf(stderr, "SIOCGMIIREG failed\n");
	    exit(1);
	}

	printf("%d: %04x\n", addr, data.val_out);
    } else {
	data.val_in = value;
	if(ioctl(fd, SIOCSMIIREG, &request) < 0) {
	    fprintf(stderr, "SIOCSMIIREG failed\n");
	    exit(1);
	}
    }

    exit(0);
}

------ cut here ------

> 
> 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,

But that pointer points to a separate "struct mii_ioctl_data"
in user space.  In my test code above, I do:

request.ifr_data = (__caddr_t)&data;

where data is a "struct mii_ioctl_data" that is separate from the
"struct ifreq".  It's not one monolithic structure, it is two
structures, one pointing to the other, both in user space.

> 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.

I disagree - the structure, as shown in "include/linux/if.h" has
"void __user *ifru_data", which is clearly a user pointer.

> 
>> 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

struct ifreq {
	.....
	union {
		.....
		void __user *	ifru_data;
		.....
	} ifr_ifru;
};

I claim that "ifru_data" is a pointer to a user-space structure,
which for these two ioctls is defined as:

struct mii_ioctl_data {
	__u16		phy_id;
	__u16		reg_num;
	__u16		val_in;
	__u16		val_out;
};

I misspoke when calling this an array.  In earlier versions of the
kernel code, this was simply an array of four __u16, but it is now a
struct.  Doesn't really matter though - it is a separate structure from
the ifreq one.  So in my test program, I have:

    struct ifreq    request;
    struct mii_ioctl_data data;
	.....
    request.ifr_data = (__caddr_t)&data;

which is where I fill in the "struct ifreq" with my user pointer to my
"struct mii_ioctl_data".

The key is that the union does not have:

	struct mii_ioctl_data ifru_data;

rather it has a pointer:

	void __user *	ifru_data;

and that is why the second copy_from_user is needed.  It would have been
much nicer if the union contained the actual mii_ioctl_data structure,
but I guess the void pointer is more flexible.

>  
>> 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.

That is my main worry as well.  However, my test program does not work
unless I modify the kernel.  Perhaps there is something wrong with the
program, but I don't think so, based on the union definition as I showed
it above.

> 
>> 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.

My patch does not give an EFAULT, as you will see if you are able to try
my test program.  I won't claim that proves I am right however. :-)

Your statement that copy_to/from_user is just there for security is
something I did not know.  Yet, that doesn't appear to be the case,
given my experiments.

I appreciate your patience - perhaps there is something unique about the
4xx CPU's that makes the direct access fail.  I should add that my
kernel does have Xenomai patches added.  I also don't know if that has
any bearing on the problem.

I'll try to resurrect my sequoia board with an unmodified kernel and
test further.  One thing for sure - something strange is going on.

	Steve

> 
> 	Arnd
> 

Re: [PATCH][RFC] ibm_newemac and SIOCGMIIREG

[Arnd Bergmann]

On Thursday 10 June 2010 21:47:27 Steven A. Falco wrote:
> On 06/10/2010 01:03 PM, Arnd Bergmann wrote:
> > Still unconvinced.
> 
> Ok - here is the user-space program I am using to test this.  It simply
> uses the ioctl to peek and poke the phy.  If I run it on an unmodified
> kernel, and I read the phy ID registers, i.e. registers 2 and 3, I get:
> 
> # ./phy -r -a 2
> 2: 0000
> # ./phy -r -a 3
> 3: 0000
> 
> which is clearly wrong.  Once I load my modified kernel, I get:
> 
> # ./phy -a 2
> 2: 0022
> # ./phy -a 3
> 3: 1512
> 
> which is correct for the phy on my board (i.e. phy id is 00221512).  If you
> could try this program on a sequoia or similar, I'd be interested in whether
> you see the problem.

It seems that your program is wrong. On my PC here, it also shows zero,
while mii-tool works fine.

> int
> main(int argc, char *argv[])
> {
>     int		    fd;
>     struct ifreq    request;
>     struct mii_ioctl_data data;

This should be

struct mii_ioctl_data *data = &request.ifr_ifru

> > 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,
> 
> But that pointer points to a separate "struct mii_ioctl_data"
> in user space.  In my test code above, I do:
> 
> request.ifr_data = (__caddr_t)&data;
> 
> where data is a "struct mii_ioctl_data" that is separate from the
> "struct ifreq".  It's not one monolithic structure, it is two
> structures, one pointing to the other, both in user space.

Only in your code, when you look at mii-tool, it does (correctly)

static int mdio_read(int skfd, int location)
{
    struct mii_data *mii = (struct mii_data *)&ifr.ifr_data;
    mii->reg_num = location;
    if (ioctl(skfd, SIOCGMIIREG, &ifr) < 0) {
        fprintf(stderr, "SIOCGMIIREG on %s failed: %s\n", ifr.ifr_name,
                strerror(errno));
        return -1;
    }
    return mii->val_out;
}

> > 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.
> 
> I disagree - the structure, as shown in "include/linux/if.h" has
> "void __user *ifru_data", which is clearly a user pointer.

This is used for SIOCSHWTSTAMP, SIOCBONDINFOQUERY, SIOCETHTOOL
and possibly a few others but not SIOCGMIIREG.
 
> and that is why the second copy_from_user is needed.  It would have been
> much nicer if the union contained the actual mii_ioctl_data structure,
> but I guess the void pointer is more flexible.

No, the void pointer is broken for a number of reasons, that's why
we don't use it.

> My patch does not give an EFAULT, as you will see if you are able to try
> my test program.  I won't claim that proves I am right however. :-)
> 
> Your statement that copy_to/from_user is just there for security is
> something I did not know.  Yet, that doesn't appear to be the case,
> given my experiments.

Right, I misread one line of your patch: when you do

	if (copy_from_user(user_data, (char __user *)data, sizeof(user_data)))
		return -EFAULT;
	user_data->val_out = emac_mdio_read(ndev, dev->phy.address,
				       user_data->reg_num);
	if (copy_to_user((char __user *)rq->ifr_data, user_data, sizeof(user_data)))

You actually copy in from a different pointer than you copy out to.
The first one is a cast from a kernel to a user pointer, which
should actually result in -EFAULT (I don't known why it doesn't)
while the second one is where you change the interface to match
your (broken) program.

	Arnd

Re: [PATCH][RFC] ibm_newemac and SIOCGMIIREG

[Steven A. Falco]

On 06/10/2010 04:35 PM, Arnd Bergmann wrote:
> 
> It seems that your program is wrong. On my PC here, it also shows zero,
> while mii-tool works fine.
> 

You are correct.  I didn't realize I should embed the data
in the ifr_data area - I thought I needed to pass a pointer.

Now I see why you avoid that.  And since IFNAMSIZ is 16,
there is enough space for the struct mii_ioctl_data.  A bit
dirty, but it obviously works.

Anyway, thanks very much for taking the time to debug my
program, and set me straight.  Naturally, I withdraw the
patch. :-)

	Steve

> 
> 	Arnd