WEXT: Correct the size of the buffer to be copied to user-space in standard GET WE ioctls.
PATCH 1/1 WEXT: Correct the size of the buffer to be copied to user-space in standard GET WE ioctls.
For the most of standard WE GET ioctls the size of the buffer to store driver's response is calculated on base of the call's descriptor (.token_size and .max_tokens fields) without taking into consideration the size of the buffer provided by application in struct iwreq. But when the response is being copied to userspace, its size is calculated from the length provided by application. This can lead to kernel internal data leak into userspace, and oopses when the buffer is located near the end of the available memory. To prevent these situations the size used during copying is set to the same one used during allocation.
---
I've actually seen those oopses on the system with 32MB of memory, when 1k object at address c1fffc00 was returned by the SLAB while handling request for allocating 568 bytes buffer (struct iw_range). Later, copy_to_user() (instructed to copy 1136 bytes, since iwlist uses 2x buffer) crashed trying to access c2000000, which is beyond the bounds of available 32MB.
The patch attached is against the Linus's tree.
net/wireless/wext.c +1 -2
@@ -866,8 +866,7 @@ static int ioctl_standard_call(struct net_device * dev, } err =copy_to_user (iwr->u.data.pointer, extra, - iwr->u.data.length * - descr->token_size); + extra_size);if (err) ret = -EFAULT ; }
Adding Jean.
Luis
Quoted 38 lines · Volodymyr G. Lukiianyk
For the most of standard WE GET ioctls the size of the buffer to store driver's response is calculated on base of the call's descriptor (.token_size and .max_tokens fields) without taking into consideration the size of the buffer provided by application in struct iwreq. But when the response is being copied to userspace, its size is calculated from the length provided by application. This can lead to kernel internal data leak into userspace, and oopses when the buffer is located near the end of the available memory. To prevent these situations the size used during copying is set to the same one used during allocation.
Signed-off-by Volodymyr G Lukiianyk <[email protected]>---
I've actually seen those oopses on the system with 32MB of memory, when 1k object at address
c1fffc00was returned by the SLAB while handling request for allocating 568 bytes buffer (structiw_range). Later,copy_to_user()(instructed to copy 1136 bytes, since iwlist uses 2x buffer) crashed trying to accessc2000000, which is beyond the bounds of available 32MB.The patch attached is against the Linus's tree.
net/wireless/wext.c +1
-2@@ -866,8 +866,7 @@ static int ioctl_standard_call(struct net_device * dev, } err =copy_to_user (iwr->u.data.pointer, extra, - iwr->u.data.length * - descr->token_size); + extra_size);if (err) ret = -EFAULT ; }
Quoted 34 lines · Luis R. Rodriguez
Adding Jean.
Luis
Quoted 29 lines · Volodymyr G. Lukiianyk
For the most of standard WE GET ioctls the size of the buffer to store driver's response is calculated on base of the call's descriptor (.token_size and .max_tokens fields) without taking into consideration the size of the buffer provided by application in struct iwreq. But when the response is being copied to userspace, its size is calculated from the length provided by application. This can lead to kernel internal data leak into userspace, and oopses when the buffer is located near the end of the available memory. To prevent these situations the size used during copying is set to the same one used during allocation.
Signed-off-by Volodymyr G Lukiianyk <[email protected]>---
I've actually seen those oopses on the system with 32MB of memory, when 1k object at address
c1fffc00was returned by the SLAB while handling request for allocating 568 bytes buffer (structiw_range). Later,copy_to_user()(instructed to copy 1136 bytes, since iwlist uses 2x buffer) crashed trying to accessc2000000, which is beyond the bounds of available 32MB.The patch attached is against the Linus's tree.
net/wireless/wext.c +0
-0@@ -866,8 +866,7 @@ static int ioctl_standard_call(struct net_device * dev, }
Quoted 10 lines
net/wireless/wext.c static int ioctl_standard_call(struct net_device * dev, · line 872
Quoted 9 lines
net/wireless/wext.c +1
-2err =copy_to_user (iwr->u.data.pointer, extra, - iwr->u.data.length * - descr->token_size); + extra_size);if (err) ret = -EFAULT ; }
Please, ignore this patch. I didn't notice that the driver's handler should set iwr->u.data.length appropriately. But is there any possibility for compiler to temporarily save the value of this field in the register and not re-read it after handler call?
Quoted 6 lines · Volodymyr G. Lukiianyk
Please, ignore this patch. I didn't notice that the driver's handler should set
iwr->u.data.lengthappropriately. But is there any possibility for compiler to temporarily save the value of this field in the register and not re-read it after handler call?
If the kernel is compiled without -fno-strict-aliasing, I've
seen similar things happening. But, I thought that using -fno-strict-aliasing would cure that issue.
Good luck...
Jean
Quoted 34 lines · Luis R. Rodriguez
Adding Jean.
Luis
Quoted 29 lines · Volodymyr G. Lukiianyk
For the most of standard WE GET ioctls the size of the buffer to store driver's response is calculated on base of the call's descriptor (.token_size and .max_tokens fields) without taking into consideration the size of the buffer provided by application in struct iwreq. But when the response is being copied to userspace, its size is calculated from the length provided by application. This can lead to kernel internal data leak into userspace, and oopses when the buffer is located near the end of the available memory. To prevent these situations the size used during copying is set to the same one used during allocation.
Signed-off-by Volodymyr G Lukiianyk <[email protected]>---
I've actually seen those oopses on the system with 32MB of memory, when 1k object at address
c1fffc00was returned by the SLAB while handling request for allocating 568 bytes buffer (structiw_range). Later,copy_to_user()(instructed to copy 1136 bytes, since iwlist uses 2x buffer) crashed trying to accessc2000000, which is beyond the bounds of available 32MB.The patch attached is against the Linus's tree.
net/wireless/wext.c +0
-0@@ -866,8 +866,7 @@ static int ioctl_standard_call(struct net_device * dev, }
Quoted 7 lines
net/wireless/wext.c static int ioctl_standard_call(struct net_device * dev, · line 872
Quoted 7 lines
net/wireless/wext.c +1
-2err =copy_to_user (iwr->u.data.pointer, extra, - iwr->u.data.length * - descr->token_size); + extra_size);if (err) ret = -EFAULT ; }
Your patch is not right either, because you could leak kernel
space memory to user space, which is not good either. And it's clearly suboptimal as many time we only fill a tiny amount of that buffer.
What you have is a driver bug.
Let's look at what happens in details...
--------------------------------------------------------
extra_size = descr->max_tokens * descr->token_size; extra = kzalloc(extra_size, GFP_KERNEL); ret = handler(dev, &info, &(iwr->u), extra); err = copy_to_user(iwr->u.data.pointer, extra, iwr->u.data.length * descr->token_size);
--------------------------------------------------------
Now, let's check a typical handler :
--------------------------------------------------------
static int ieee80211_ioctl_giwrange(struct net_device *dev,
struct iw_request_info *info, struct iw_point *data, char *extra)
{
data->length = sizeof(struct iw_range);--------------------------------------------------------
And lastly, the definition for the ioctl :
---------------------------------------------
[SIOCGIWRANGE - SIOCIWFIRST] = {
.header_type = IW_HEADER_TYPE_POINT,
.token_size = 1,
.max_tokens = sizeof(struct iw_range),
.flags = IW_DESCR_FLAG_DUMP,
},---------------------------------------------
What's happening is that the *driver* sets the actual amount
of data he wrote in iwr->u.data.length in the handler (see above). There is no way to guess how much data the driver returns, and we don't want to push more data in user space because we would leak kernel buffer (security risk).
(Note: we also check that we don't overrun the userspace
buffer).
Now, what's obviously missing is that we don't double check
that the driver returns valid data in iwr->u.data.length. If the driver screws up, everything falls apart. But, there are many other ways the driver could screw up, and we won't be able to catch everything.
I've also seen this fail when the driver and the kernel have
not been compiled with the same version of Wireless Extensions, but that's shooting yourself in the foot.
Could you point out which driver is responsible for that ? Have fun... Jean
Quoted 1 line · Jean Tourrilhes
[skiped]
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
net/wireless/wext.c +0
-0@@ -866,8 +866,7 @@ static int ioctl_standard_call(struct net_device * dev, }
Quoted 12 lines
Quoted 7 lines
net/wireless/wext.c static int ioctl_standard_call(struct net_device * dev, · line 872
Quoted 7 lines
net/wireless/wext.c +1
-2err =copy_to_user (iwr->u.data.pointer, extra, - iwr->u.data.length * - descr->token_size); + extra_size);if (err) ret = -EFAULT ; }Your patch is not right either, because you could leak kernel
space memory to user space, which is not good either. And it's clearly suboptimal as many time we only fill a tiny amount of that buffer.
What you have is a driver bug.
Yeah, it was bug in a driver.
Quoted 28 lines
Let's look at what happens in details...
--------------------------------------------------------extra_size = descr->max_tokens * descr->token_size; extra = kzalloc(extra_size, GFP_KERNEL); ret = handler(dev, &info, &(iwr->u), extra); err = copy_to_user(iwr->u.data.pointer, extra, iwr->u.data.length * descr->token_size);--------------------------------------------------------
Now, let's check a typical handler :
--------------------------------------------------------
static int ieee80211_ioctl_giwrange(structnet_device*dev,
structiw_request_info*info, structiw_point*data, char *extra){ data->length = sizeof(struct iw_range);--------------------------------------------------------
And lastly, the definition for the ioctl :
---------------------------------------------[SIOCGIWRANGE - SIOCIWFIRST] = { .header_type = IW_HEADER_TYPE_POINT, .token_size = 1, .max_tokens = sizeof(struct iw_range), .flags = IW_DESCR_FLAG_DUMP, },
Quoted 7 lines
---------------------------------------------
What's happening is that the *driver* sets the actual amount
of data he wrote iniwr->u.data.lengthin the handler (see above). There is no way to guess how much data the driver returns, and we don't want to push more data in user space because we would leak kernel buffer (security risk).
Actually, the buffer is zeroed by kzalloc(), so only 0s would be leaked :)
Quoted 12 lines
(Note: we also check that we don't overrun the userspace
buffer).Now, what's obviously missing is that we don't double check
that the driver returns valid data iniwr->u.data.length. If the driver screws up, everything falls apart. But, there are many other ways the driver could screw up, and we won't be able to catch everything.
I've also seen this fail when the driver and the kernel have
not been compiled with the same version of Wireless Extensions, but that's shooting yourself in the foot.
Thanks for the thorough explanation and sorry for the noise.
Quoted 1 line
Could you point out which driver is responsible for that ?
It is an out-of-tree driver in development of which I'm somewhat involved.