ibm_newemac and SIOCGMIIREG

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.

---

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.

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

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

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

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.

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

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?

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.

There is no array, and no pointer in struct ifreq

No other device driver implementing SIOCGMIIREG does any of this, so I'm pretty sure it's not the proper fix.

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

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

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.

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

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.

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.

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

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

This should be

struct mii_ioctl_data *data = &request.ifr_ifru

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

This is used for SIOCSHWTSTAMP, SIOCBONDINFOQUERY, SIOCETHTOOL and possibly a few others but not SIOCGMIIREG.

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

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

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