Re: [PATCH v2] staging: gdm72xx: add userspace data struct

[One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk>]

>  	if (cmd != SIOCWMIOCTL)
>  		return -EOPNOTSUPP;
> @@ -482,8 +483,16 @@ static int gdm_wimax_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
>  				/* NOTE: gdm_update_fsm should be called
>  				 * before gdm_wimax_ioctl_set_data is called.
>  				 */
> -				gdm_update_fsm(dev,
> -					       req->data.buf);
> +				fsm_buf = kmalloc(sizeof(fsm_s), GFP_KERNEL);
> +				if (!fsm_buf)
> +					return -ENOMEM;
> +				if (copy_from_user(fsm_buf, req->data.buf,
> +						   sizeof(fsm_s))) {
> +					kfree(fsm_buf);
> +					return -EFAULT;
> +				}
> +				gdm_update_fsm(dev, fsm_buf);
> +				kfree(fsm_buf);

fsm_s is a total of 12 bytes so this is complete overkill. If you are
copying a large object then yes the pattern you have used is correct
(except that you mean sizeof(struct fsm_s) and it doesn't compile at the
moment!

data_s can just be modified to be __user. All uses of it follow that
rule.

All I think you need in this case is

	struct fsm_s fsm_buf;

	if (copy_from_user(&fsm_buf, req->data.buf,sizeof(buf))
		return -EFAULT
	gdm_update_fsm(&fsm_buf);

If you are touching the structs it might be wise to fix the other
problems with them notably the use of int. sizes when used are unsigned -
and signed sizes are asking for errors. In fact if you look at the
existing uses of the size checks they look deeply suspicious the moment
anything malicious passes in negative numbers.



All the types in the ioctl structures also ought to be proper fixed sizes
but that's another matter.