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

["Volodymyr G. Lukiianyk" <volodymyrgl@gmail.com>]

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.