[PATCH][RFC] ibm_newemac and SIOCGMIIREG
Steven A. Falco
sfalco at harris.com
Fri Jun 11 05:47:27 EST 2010
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
>
More information about the Linuxppc-dev
mailing list