[PATCH] staging: gdm72xx: add userspace data struct

[PATCH] staging: gdm72xx: add userspace data struct

[Wim de With]

This fixes the sparse warnings about dereferencing a userspace pointer.

However, gdm_wimax_ioctl_get_data and gdm_wimax_ioctl_set_data both
carefully check whether the pointers and data are valid, gdm_update_fsm
does not. It simply casts userspace data to struct fsm_s. I haven't
fixed this, and am not sure what to do about it.

Signed-off-by: Wim de With <nauxuron@wimdewith.com>
---
 drivers/staging/gdm72xx/gdm_wimax.c | 17 +++++++++++++----
 drivers/staging/gdm72xx/wm_ioctl.h  |  7 ++++++-
 2 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/gdm72xx/gdm_wimax.c b/drivers/staging/gdm72xx/gdm_wimax.c
index d9ddced..16eac61 100644
--- a/drivers/staging/gdm72xx/gdm_wimax.c
+++ b/drivers/staging/gdm72xx/gdm_wimax.c
@@ -368,7 +368,7 @@ static void kdelete(void **buf)
 	}
 }
 
-static int gdm_wimax_ioctl_get_data(struct data_s *dst, struct data_s *src)
+static int gdm_wimax_ioctl_get_data(struct udata_s *dst, struct data_s *src)
 {
 	int size;
 
@@ -384,7 +384,7 @@ static int gdm_wimax_ioctl_get_data(struct data_s *dst, struct data_s *src)
 	return 0;
 }
 
-static int gdm_wimax_ioctl_set_data(struct data_s *dst, struct data_s *src)
+static int gdm_wimax_ioctl_set_data(struct data_s *dst, struct udata_s *src)
 {
 	if (!src->size) {
 		dst->size = 0;
@@ -460,6 +460,7 @@ static int gdm_wimax_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 	struct wm_req_s *req = (struct wm_req_s *)ifr;
 	struct nic *nic = netdev_priv(dev);
 	int ret;
+	void *fsm_buf;
 
 	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);
 			}
 			ret = gdm_wimax_ioctl_set_data(
 				&nic->sdk_data[req->data_id], &req->data);
diff --git a/drivers/staging/gdm72xx/wm_ioctl.h b/drivers/staging/gdm72xx/wm_ioctl.h
index ed8f649..631cb1d 100644
--- a/drivers/staging/gdm72xx/wm_ioctl.h
+++ b/drivers/staging/gdm72xx/wm_ioctl.h
@@ -78,13 +78,18 @@ struct data_s {
 	void	*buf;
 };
 
+struct udata_s {
+	int		size;
+	void __user	*buf;
+};
+
 struct wm_req_s {
 	union {
 		char ifrn_name[IFNAMSIZ];
 	} ifr_ifrn;
 	unsigned short	cmd;
 	unsigned short	data_id;
-	struct data_s	data;
+	struct udata_s	data;
 
 /* NOTE: sizeof(struct wm_req_s) must be less than sizeof(struct ifreq). */
 };
-- 
2.6.3

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

[Dan Carpenter]

On Thu, Dec 10, 2015 at 10:11:12AM +0100, Wim de With wrote:
> @@ -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);


No.  This change is a bug.

regards,
dan carpenter

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

[Wim de With]

On 10-12-2015 10:37, Dan Carpenter wrote:
> On Thu, Dec 10, 2015 at 10:11:12AM +0100, Wim de With wrote:
>> @@ -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);
> 
> 
> No.  This change is a bug.
> 
> regards,
> dan carpenter
> 

But what if I just keep it as:

	gdm_update_fsm(dev, req->data.buf)

Then it would just trust a __user pointer right?

Wim

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

[Dan Carpenter]

On Thu, Dec 10, 2015 at 10:42:14AM +0100, Wim de With wrote:
> On 10-12-2015 10:37, Dan Carpenter wrote:
> > On Thu, Dec 10, 2015 at 10:11:12AM +0100, Wim de With wrote:
> >> @@ -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);
> > 
> > 
> > No.  This change is a bug.
> > 
> > regards,
> > dan carpenter
> > 
> 
> But what if I just keep it as:
> 
> 	gdm_update_fsm(dev, req->data.buf)
> 
> Then it would just trust a __user pointer right?

I appologize, I didn't read the patch carefully.  This is a bugfix.  Can
you resend the patch with a better patch description where you replace
the last paragraph and say:

Once I updated the Sparse annotations, I noticed a bug in
gdm_wimax_ioctl() where we pass a user space pointer to gdm_update_fsm()
which dereferences it.  I fixed this.

regards,
dan carpenter

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

[Wim de With]

This fixes the sparse warnings about dereferencing a userspace pointer.

Once I updated the sparse annotations, I noticed a bug in
gdm_wimax_ioctl() where we pass a user space pointer to gdm_update_fsm()
which dereferences it. I fixed this.

Signed-off-by: Wim de With <nauxuron@wimdewith.com>
---
 drivers/staging/gdm72xx/gdm_wimax.c | 17 +++++++++++++----
 drivers/staging/gdm72xx/wm_ioctl.h  |  7 ++++++-
 2 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/gdm72xx/gdm_wimax.c b/drivers/staging/gdm72xx/gdm_wimax.c
index d9ddced..16eac61 100644
--- a/drivers/staging/gdm72xx/gdm_wimax.c
+++ b/drivers/staging/gdm72xx/gdm_wimax.c
@@ -368,7 +368,7 @@ static void kdelete(void **buf)
 	}
 }
 
-static int gdm_wimax_ioctl_get_data(struct data_s *dst, struct data_s *src)
+static int gdm_wimax_ioctl_get_data(struct udata_s *dst, struct data_s *src)
 {
 	int size;
 
@@ -384,7 +384,7 @@ static int gdm_wimax_ioctl_get_data(struct data_s *dst, struct data_s *src)
 	return 0;
 }
 
-static int gdm_wimax_ioctl_set_data(struct data_s *dst, struct data_s *src)
+static int gdm_wimax_ioctl_set_data(struct data_s *dst, struct udata_s *src)
 {
 	if (!src->size) {
 		dst->size = 0;
@@ -460,6 +460,7 @@ static int gdm_wimax_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 	struct wm_req_s *req = (struct wm_req_s *)ifr;
 	struct nic *nic = netdev_priv(dev);
 	int ret;
+	void *fsm_buf;
 
 	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);
 			}
 			ret = gdm_wimax_ioctl_set_data(
 				&nic->sdk_data[req->data_id], &req->data);
diff --git a/drivers/staging/gdm72xx/wm_ioctl.h b/drivers/staging/gdm72xx/wm_ioctl.h
index ed8f649..631cb1d 100644
--- a/drivers/staging/gdm72xx/wm_ioctl.h
+++ b/drivers/staging/gdm72xx/wm_ioctl.h
@@ -78,13 +78,18 @@ struct data_s {
 	void	*buf;
 };
 
+struct udata_s {
+	int		size;
+	void __user	*buf;
+};
+
 struct wm_req_s {
 	union {
 		char ifrn_name[IFNAMSIZ];
 	} ifr_ifrn;
 	unsigned short	cmd;
 	unsigned short	data_id;
-	struct data_s	data;
+	struct udata_s	data;
 
 /* NOTE: sizeof(struct wm_req_s) must be less than sizeof(struct ifreq). */
 };
-- 
2.6.3

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

[One Thousand Gnomes]

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

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

[Wim de With]

On Thu, Dec 10, 2015 at 02:44:45PM +0000, One Thousand Gnomes wrote:
> (except that you mean sizeof(struct fsm_s) and it doesn't compile at the
> moment!

Oops, sloppy mistake.

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

What do you mean? The data still needs to be copied from user space to kernel
space, if I'm not mistaken. And not all uses follow that rule, since in both
gdm_wimax_ioctl_get_data() and gdm_wimax_ioctl_set_data() it is used as both
the source and destination in the copy_from_user() and copy_to_user() call.

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

Do you mean sizeof(fsm_s)? I realize this would have been far simpler than my
overkill solution.

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

I would love to do that, but it is a bit outside the scope of this patch, so I
would rather safe this for another patch.

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

[One Thousand Gnomes]

On Fri, 11 Dec 2015 00:47:38 +0100
Wim de With <nauxuron@wimdewith.com> wrote:

> On Thu, Dec 10, 2015 at 02:44:45PM +0000, One Thousand Gnomes wrote:
> > (except that you mean sizeof(struct fsm_s) and it doesn't compile at the
> > moment!
> 
> Oops, sloppy mistake.

Compile/test/send - even when in a hurry

> > data_s can just be modified to be __user. All uses of it follow that
> > rule.
> 
> What do you mean? The data still needs to be copied from user space to kernel
> space, if I'm not mistaken. And not all uses follow that rule, since in both
> gdm_wimax_ioctl_get_data() and gdm_wimax_ioctl_set_data() it is used as both
> the source and destination in the copy_from_user() and copy_to_user() call.

Good point I missed that.

> > 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);
> 
> Do you mean sizeof(fsm_s)? I realize this would have been far simpler than my
> overkill solution.

Yes either sizeof(struct fsm_s) or sizeof(fsm_buf). The former is often
safer.

> > 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.
> 
> I would love to do that, but it is a bit outside the scope of this patch, so I
> would rather safe this for another patch.

Absolutely right - it should be another patch

Alan

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

[Wim de With]

This fixes the sparse warnings about dereferencing a userspace pointer.

Once I updated the sparse annotations, I noticed a bug in
gdm_wimax_ioctl() where we pass a user space pointer to gdm_update_fsm()
which dereferences it. I fixed this.

Signed-off-by: Wim de With <nauxuron@wimdewith.com>
---
 drivers/staging/gdm72xx/gdm_wimax.c | 12 ++++++++----
 drivers/staging/gdm72xx/wm_ioctl.h  |  7 ++++++-
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/gdm72xx/gdm_wimax.c b/drivers/staging/gdm72xx/gdm_wimax.c
index d9ddced..5da9ad9 100644
--- a/drivers/staging/gdm72xx/gdm_wimax.c
+++ b/drivers/staging/gdm72xx/gdm_wimax.c
@@ -368,7 +368,7 @@ static void kdelete(void **buf)
 	}
 }
 
-static int gdm_wimax_ioctl_get_data(struct data_s *dst, struct data_s *src)
+static int gdm_wimax_ioctl_get_data(struct udata_s *dst, struct data_s *src)
 {
 	int size;
 
@@ -384,7 +384,7 @@ static int gdm_wimax_ioctl_get_data(struct data_s *dst, struct data_s *src)
 	return 0;
 }
 
-static int gdm_wimax_ioctl_set_data(struct data_s *dst, struct data_s *src)
+static int gdm_wimax_ioctl_set_data(struct data_s *dst, struct udata_s *src)
 {
 	if (!src->size) {
 		dst->size = 0;
@@ -460,6 +460,7 @@ static int gdm_wimax_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 	struct wm_req_s *req = (struct wm_req_s *)ifr;
 	struct nic *nic = netdev_priv(dev);
 	int ret;
+	struct fsm_s fsm_buf;
 
 	if (cmd != SIOCWMIOCTL)
 		return -EOPNOTSUPP;
@@ -482,8 +483,11 @@ 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);
+				if (copy_from_user(&fsm_buf, req->data.buf,
+						   sizeof(struct fsm_s)))
+					return -EFAULT;
+
+				gdm_update_fsm(dev, &fsm_buf);
 			}
 			ret = gdm_wimax_ioctl_set_data(
 				&nic->sdk_data[req->data_id], &req->data);
diff --git a/drivers/staging/gdm72xx/wm_ioctl.h b/drivers/staging/gdm72xx/wm_ioctl.h
index ed8f649..631cb1d 100644
--- a/drivers/staging/gdm72xx/wm_ioctl.h
+++ b/drivers/staging/gdm72xx/wm_ioctl.h
@@ -78,13 +78,18 @@ struct data_s {
 	void	*buf;
 };
 
+struct udata_s {
+	int		size;
+	void __user	*buf;
+};
+
 struct wm_req_s {
 	union {
 		char ifrn_name[IFNAMSIZ];
 	} ifr_ifrn;
 	unsigned short	cmd;
 	unsigned short	data_id;
-	struct data_s	data;
+	struct udata_s	data;
 
 /* NOTE: sizeof(struct wm_req_s) must be less than sizeof(struct ifreq). */
 };
-- 
2.6.3