Suspected bad cade in prism2_ioctl_giwencodeext()

[Jean Tourrilhes]

On Sat, Aug 26, 2006 at 07:04:39PM -0700, Jouni Malinen wrote:
> Looks like I missed this email and only found in while going through my
> pending mailing list folder..
> 

	That's ok, I'm not always the fastest to react ;-)

> On Wed, May 03, 2006 at 11:33:32AM -0700, Jean Tourrilhes wrote:
> 
> > 	I suspect the following bit of code does not work.
> > 
> > -----------------------------------------------------
> > static int prism2_ioctl_giwencodeext(struct net_device *dev,
> > 				     struct iw_request_info *info,
> > 				     struct iw_point *erq, char *extra)
> > {
> > [...]
> > 	struct iw_encode_ext *ext = (struct iw_encode_ext *) extra;
> > [...]
> > 	addr = ext->addr.sa_data;
> > 	if (addr[0] == 0xff && addr[1] == 0xff && addr[2] == 0xff &&
> > 	    addr[3] == 0xff && addr[4] == 0xff && addr[5] == 0xff) {
> > -----------------------------------------------------
> > 
> > 	For the GET requests, only the 'struct iw_point' part of the
> > request is passed from user-space to kernel, on the other hand 'extra'
> > is never passed from user-space to kernel. So, in the code above, you
> > have garbage in extra at the start of the function, and therefore
> > garbage in 'addr'.
> 
> Hmm.. That's not good. I don't think I've ever used this ioctl since
> both Host AP driver and madwifi are still using private ioctls for this
> functionality. What controls which information is passed to the kernel?
> I would have expecgted the user space program to make that address
> available and kernel would have possibility of copying it. Is it being
> filtered in the generic WEXT code in kernel? Or is the driver just
> expected to copy it from user space?

	Yes, the generic WE code does filter it. I don't want to
burden the other GET request with getting a useless buffer from user
space to kernel space.
	Note that this is also true for private requests, so clearly
your private ioctl code does something different.
	One ugly way around it is to do like scan, which is to have a
pair of ioctls...

> This ioctl was not added for enumerating the keys. It was added to
> provide a mechanism for fetching the current TX counters (TSC for TKIP
> and PN for CCMP) from the driver. This functionality is only needed for
> AP functionality and in practice, it would actually be enough to just be
> able to query the sequence number for the broadcast keys..

	How many bits ? Would it fit in 16 bits ?

> The part of not being able to use this to enumerate keys was by design.
> However, it would be nice to fix the part of being able to read the
> sequence numbers correctly. A minimal change would be to just define
> that SIOCGIWENCODEEXT can only be used to query group keys and remove
> the unicast case from hostap_ioctl.c (and other drivers that may have
> implemented SIOCGIWENCODEEXT)..

	I guess you know better than me ;-)

> Jouni Malinen                                            PGP id EFC895FA

	Jean