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

One Thousand Gnomes gnomes at lxorguk.ukuu.org.uk
Thu Dec 10 14:44:45 UTC 2015


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



More information about the devel mailing list