| Message ID | 4C10EFE0.6030106@harris.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
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
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
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
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 >
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
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
--- 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;
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.