[PATCH] WEXT: Correct the size of the buffer to be copied to user-space in standard GET WE ioctls.

[PATCH] WEXT: Correct the size of the buffer to be copied to user-space in standard GET WE ioctls.

[Volodymyr G. Lukiianyk]

[-- Attachment #1: Type: text/plain, Size: 1109 bytes --]

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 <volodymyrgl@gmail.com>
---

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.


[-- Attachment #2: fix_copied_buffer_size.diff --]
[-- Type: text/plain, Size: 392 bytes --]

diff --git a/net/wireless/wext.c b/net/wireless/wext.c
index 47e80cc..c6ce59b 100644
--- a/net/wireless/wext.c
+++ b/net/wireless/wext.c
@@ -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;
 		}

Re: [PATCH] WEXT: Correct the size of the buffer to be copied to user-space in standard GET WE ioctls.

[Luis R. Rodriguez]

Adding Jean.

  Luis

On Jan 20, 2008 1:30 PM, Volodymyr G. Lukiianyk <volodymyrgl@gmail.com> wrote:
> 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 <volodymyrgl@gmail.com>
> ---
>
> 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.
>
>
> diff --git a/net/wireless/wext.c b/net/wireless/wext.c
> index 47e80cc..c6ce59b 100644
> --- a/net/wireless/wext.c
> +++ b/net/wireless/wext.c
> @@ -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;
>                 }
>
>

Re: [PATCH] WEXT: Correct the size of the buffer to be copied to user-space in standard GET WE ioctls.

[Volodymyr G. Lukiianyk]

Luis R. Rodriguez wrote:
> Adding Jean.
> 
>   Luis
> 
> On Jan 20, 2008 1:30 PM, Volodymyr G. Lukiianyk <volodymyrgl@gmail.com> wrote:
>> 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 <volodymyrgl@gmail.com>
>> ---
>>
>> 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.
>>
>>
>> diff --git a/net/wireless/wext.c b/net/wireless/wext.c
>> index 47e80cc..c6ce59b 100644
>> --- a/net/wireless/wext.c
>> +++ b/net/wireless/wext.c
>> @@ -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;
>>                 }
>>
>>
> 


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?

Re: [PATCH] WEXT: Correct the size of the buffer to be copied to user-space in standard GET WE ioctls.

[Jean Tourrilhes]

On Sun, Jan 20, 2008 at 02:04:02PM -0500, Luis R. Rodriguez wrote:
> Adding Jean.
> 
>   Luis
> 
> On Jan 20, 2008 1:30 PM, Volodymyr G. Lukiianyk <volodymyrgl@gmail.com> wrote:
> > 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 <volodymyrgl@gmail.com>
> > ---
> >
> > 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.
> >
> >
> > diff --git a/net/wireless/wext.c b/net/wireless/wext.c
> > index 47e80cc..c6ce59b 100644
> > --- a/net/wireless/wext.c
> > +++ b/net/wireless/wext.c
> > @@ -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;
> >                 }

	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

Re: [PATCH] WEXT: Correct the size of the buffer to be copied to user-space in standard GET WE ioctls.

[Jean Tourrilhes]

On Mon, Jan 21, 2008 at 06:07:29PM +0200, Volodymyr G. Lukiianyk wrote:
> 
> 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?
> 

	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

Re: [PATCH] WEXT: Correct the size of the buffer to be copied to user-space in standard GET WE ioctls.

[Volodymyr G. Lukiianyk]

Jean Tourrilhes wrote:
>

[skiped]

>>> diff --git a/net/wireless/wext.c b/net/wireless/wext.c
>>> index 47e80cc..c6ce59b 100644
>>> --- a/net/wireless/wext.c
>>> +++ b/net/wireless/wext.c
>>> @@ -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;
>>>                 }
> 
> 	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.

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

Actually, the buffer is zeroed by kzalloc(), so only 0s would be leaked :)

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

Thanks for the thorough explanation and sorry for the noise.

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