LinuxLists.cc - [PATCH] staging: vt6655: tag data as _

2014-07-24 23:08:17

Subject: [PATCH] staging: vt6655: tag data as __user in struct tagSCmdRequest

Sparse reported that the data from tagSCmdRequest is given by
userspace, so it should be tagged as such.

Later, we were memcomparing and dereferencing it without first copying
it, fix that as well.

Signed-off-by: Guillaume Clement <[email protected]>
---
drivers/staging/vt6655/iocmd.h | 2 +-
drivers/staging/vt6655/iwctl.c | 22 +++++++++++++++-------
drivers/staging/vt6655/iwctl.h | 6 +++---
3 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/vt6655/iocmd.h b/drivers/staging/vt6655/iocmd.h
index e499f1b..dd12498 100644
--- a/drivers/staging/vt6655/iocmd.h
+++ b/drivers/staging/vt6655/iocmd.h
@@ -100,7 +100,7 @@ typedef enum tagWZONETYPE {
#pragma pack(1)
typedef struct tagSCmdRequest {
u8 name[16];
- void *data;
+ void __user *data;
u16 wResult;
u16 wCmdCode;
} SCmdRequest, *PSCmdRequest;
diff --git a/drivers/staging/vt6655/iwctl.c b/drivers/staging/vt6655/iwctl.c
index 501cd64..9291259 100644
--- a/drivers/staging/vt6655/iwctl.c
+++ b/drivers/staging/vt6655/iwctl.c
@@ -1621,14 +1621,17 @@ int iwctl_giwauth(struct net_device *dev,
int iwctl_siwgenie(struct net_device *dev,
struct iw_request_info *info,
struct iw_point *wrq,
- char *extra)
+ char __user *extra)
{
PSDevice pDevice = (PSDevice)netdev_priv(dev);
PSMgmtObject pMgmt = &(pDevice->sMgmtObj);
int ret = 0;
+ char length;

if (wrq->length) {
- if ((wrq->length < 2) || (extra[1]+2 != wrq->length)) {
+ if (get_user(length, extra + 1))
+ return -EFAULT;
+ if ((wrq->length < 2) || (length != wrq->length)) {
ret = -EINVAL;
goto out;
}
@@ -1654,7 +1657,7 @@ out://not completely ...not necessary in wpa_supplicant 0.5.8
int iwctl_giwgenie(struct net_device *dev,
struct iw_request_info *info,
struct iw_point *wrq,
- char *extra)
+ char __user *extra)
{
PSDevice pDevice = (PSDevice)netdev_priv(dev);
PSMgmtObject pMgmt = &(pDevice->sMgmtObj);
@@ -1801,18 +1804,23 @@ int iwctl_giwencodeext(struct net_device *dev,
int iwctl_siwmlme(struct net_device *dev,
struct iw_request_info *info,
struct iw_point *wrq,
- char *extra)
+ char __user *extra)
{
PSDevice pDevice = (PSDevice)netdev_priv(dev);
PSMgmtObject pMgmt = &(pDevice->sMgmtObj);
- struct iw_mlme *mlme = (struct iw_mlme *)extra;
+ struct iw_mlme mime;
+
int ret = 0;

- if (memcmp(pMgmt->abyCurrBSSID, mlme->addr.sa_data, ETH_ALEN)) {
+ ret = copy_from_user(&mime, extra, sizeof(mime));
+ if (ret)
+ return -EFAULT;
+
+ if (memcmp(pMgmt->abyCurrBSSID, mime.addr.sa_data, ETH_ALEN)) {
ret = -EINVAL;
return ret;
}
- switch (mlme->cmd) {
+ switch (mime.cmd) {
case IW_MLME_DEAUTH:
//this command seems to be not complete,please test it --einsnliu
//bScheduleCommand((void *) pDevice, WLAN_CMD_DEAUTH, (unsigned char *)&reason);
diff --git a/drivers/staging/vt6655/iwctl.h b/drivers/staging/vt6655/iwctl.h
index de0a337..7dd6310 100644
--- a/drivers/staging/vt6655/iwctl.h
+++ b/drivers/staging/vt6655/iwctl.h
@@ -176,12 +176,12 @@ int iwctl_giwauth(struct net_device *dev,
int iwctl_siwgenie(struct net_device *dev,
struct iw_request_info *info,
struct iw_point *wrq,
- char *extra);
+ char __user *extra);

int iwctl_giwgenie(struct net_device *dev,
struct iw_request_info *info,
struct iw_point *wrq,
- char *extra);
+ char __user *extra);

int iwctl_siwencodeext(struct net_device *dev,
struct iw_request_info *info,
@@ -196,7 +196,7 @@ int iwctl_giwencodeext(struct net_device *dev,
int iwctl_siwmlme(struct net_device *dev,
struct iw_request_info *info,
struct iw_point *wrq,
- char *extra);
+ char __user *extra);
#endif // #ifdef WPA_SUPPLICANT_DRIVER_WEXT_SUPPORT
//End Add -- //2008-0409-07, <Add> by Einsn Liu

--
1.8.5.5

2014-07-25 11:52:58

by Dan Carpenter

[permalink] [raw]

Subject: Re: [PATCH] staging: vt6655: tag data as __user in struct tagSCmdRequest

On Fri, Jul 25, 2014 at 01:07:40AM +0200, Guillaume Clement wrote:
> diff --git a/drivers/staging/vt6655/iwctl.c b/drivers/staging/vt6655/iwctl.c
> index 501cd64..9291259 100644
> --- a/drivers/staging/vt6655/iwctl.c
> +++ b/drivers/staging/vt6655/iwctl.c
> @@ -1621,14 +1621,17 @@ int iwctl_giwauth(struct net_device *dev,
> int iwctl_siwgenie(struct net_device *dev,
> struct iw_request_info *info,
> struct iw_point *wrq,
> - char *extra)
> + char __user *extra)
> {
> PSDevice pDevice = (PSDevice)netdev_priv(dev);
> PSMgmtObject pMgmt = &(pDevice->sMgmtObj);
> int ret = 0;
> + char length;
>
> if (wrq->length) {
> - if ((wrq->length < 2) || (extra[1]+2 != wrq->length)) {
> + if (get_user(length, extra + 1))
> + return -EFAULT;
> + if ((wrq->length < 2) || (length != wrq->length)) {
> ret = -EINVAL;
> goto out;
> }

Wow, this is confusing code. The patch description isn't clear enough
that this is a bugfix patch and not just a "tag data" patch.

I don't think this is correct. We need to check the length of the input
buffer before we call get_user(). Can we return directly or do we
*need* to go to the mysteriously named "out"? Also the + 2 is lost,
this would break everything if the current code works (not necessarily a
valid assumption). Delete all my comments in the final code.

/* 2 because we skip the first byte and read length from the
second byte */
if (wrq->length < 2) {
ret = -EINVAL;
goto out;
}

ret = get_user(length, extra + 1);
if (ret)
goto out;

if (length + 2 != wrq->length) {
ret = -EINVAL;
goto out;
}

regards,
dan carpenter

2014-07-25 12:25:32

by Guillaume Clement

[permalink] [raw]

Subject: Re: [PATCH] staging: vt6655: tag data as __user in struct tagSCmdRequest

On Fri, Jul 25, 2014 at 02:52:34PM +0300, Dan Carpenter wrote:
> On Fri, Jul 25, 2014 at 01:07:40AM +0200, Guillaume Clement wrote:
> > diff --git a/drivers/staging/vt6655/iwctl.c b/drivers/staging/vt6655/iwctl.c
> > index 501cd64..9291259 100644
> > --- a/drivers/staging/vt6655/iwctl.c
> > +++ b/drivers/staging/vt6655/iwctl.c
> > @@ -1621,14 +1621,17 @@ int iwctl_giwauth(struct net_device *dev,
> > int iwctl_siwgenie(struct net_device *dev,
> > struct iw_request_info *info,
> > struct iw_point *wrq,
> > - char *extra)
> > + char __user *extra)
> > {
> > PSDevice pDevice = (PSDevice)netdev_priv(dev);
> > PSMgmtObject pMgmt = &(pDevice->sMgmtObj);
> > int ret = 0;
> > + char length;
> >
> > if (wrq->length) {
> > - if ((wrq->length < 2) || (extra[1]+2 != wrq->length)) {
> > + if (get_user(length, extra + 1))
> > + return -EFAULT;
> > + if ((wrq->length < 2) || (length != wrq->length)) {
> > ret = -EINVAL;
> > goto out;
> > }
>
> Wow, this is confusing code. The patch description isn't clear enough
> that this is a bugfix patch and not just a "tag data" patch.
>
> I don't think this is correct. We need to check the length of the input
> buffer before we call get_user(). Can we return directly or do we
> *need* to go to the mysteriously named "out"? Also the + 2 is lost,
> this would break everything if the current code works (not necessarily a
> valid assumption). Delete all my comments in the final code.

In this case, the "out" label just does "return ret;". But I agree this
is better practice to jump to out, in case this changes at a later time,
and to keep the code consistent.

About the missing + 2, and modifying the order of sorry about that, I
sent the patch too fast.

Thank you for your review, I will be submitting an updated patch with
your remarks.

2014-07-25 12:33:47

by Dan Carpenter

[permalink] [raw]

Subject: Re: [PATCH] staging: vt6655: tag data as __user in struct tagSCmdRequest

On Fri, Jul 25, 2014 at 02:25:06PM +0200, Guillaume CL?MENT wrote:
> On Fri, Jul 25, 2014 at 02:52:34PM +0300, Dan Carpenter wrote:
> > On Fri, Jul 25, 2014 at 01:07:40AM +0200, Guillaume Clement wrote:
> > > diff --git a/drivers/staging/vt6655/iwctl.c b/drivers/staging/vt6655/iwctl.c
> > > index 501cd64..9291259 100644
> > > --- a/drivers/staging/vt6655/iwctl.c
> > > +++ b/drivers/staging/vt6655/iwctl.c
> > > @@ -1621,14 +1621,17 @@ int iwctl_giwauth(struct net_device *dev,
> > > int iwctl_siwgenie(struct net_device *dev,
> > > struct iw_request_info *info,
> > > struct iw_point *wrq,
> > > - char *extra)
> > > + char __user *extra)
> > > {
> > > PSDevice pDevice = (PSDevice)netdev_priv(dev);
> > > PSMgmtObject pMgmt = &(pDevice->sMgmtObj);
> > > int ret = 0;
> > > + char length;
> > >
> > > if (wrq->length) {
> > > - if ((wrq->length < 2) || (extra[1]+2 != wrq->length)) {
> > > + if (get_user(length, extra + 1))
> > > + return -EFAULT;
> > > + if ((wrq->length < 2) || (length != wrq->length)) {
> > > ret = -EINVAL;
> > > goto out;
> > > }
> >
> > Wow, this is confusing code. The patch description isn't clear enough
> > that this is a bugfix patch and not just a "tag data" patch.
> >
> > I don't think this is correct. We need to check the length of the input
> > buffer before we call get_user(). Can we return directly or do we
> > *need* to go to the mysteriously named "out"? Also the + 2 is lost,
> > this would break everything if the current code works (not necessarily a
> > valid assumption). Delete all my comments in the final code.
>
> In this case, the "out" label just does "return ret;". But I agree this
> is better practice to jump to out, in case this changes at a later time,
> and to keep the code consistent.
>

No, absolutely not. "out" labels are the worst. We shouldn't make the
code unreadable and mysterious *now* just because of something which
is possible but frankly unlikely "at some later time".

Feel free to return directly if you want.

regards,
dan carpenter

2014-07-25 12:49:24

by Guillaume Clement

[permalink] [raw]

Subject: [PATCH] staging: vt6655: fix direct dereferencing of user pointer

Sparse reported that the data from tagSCmdRequest is given by
userspace, so it should be tagged as such.

Later, we were memcomparing and dereferencing it without first copying
it, fix that as well.

Signed-off-by: Guillaume Clement <[email protected]>
---
drivers/staging/vt6655/iocmd.h | 2 +-
drivers/staging/vt6655/iwctl.c | 32 ++++++++++++++++++++++----------
drivers/staging/vt6655/iwctl.h | 6 +++---
3 files changed, 26 insertions(+), 14 deletions(-)

diff --git a/drivers/staging/vt6655/iocmd.h b/drivers/staging/vt6655/iocmd.h
index e499f1b..dd12498 100644
--- a/drivers/staging/vt6655/iocmd.h
+++ b/drivers/staging/vt6655/iocmd.h
@@ -100,7 +100,7 @@ typedef enum tagWZONETYPE {
#pragma pack(1)
typedef struct tagSCmdRequest {
u8 name[16];
- void *data;
+ void __user *data;
u16 wResult;
u16 wCmdCode;
} SCmdRequest, *PSCmdRequest;
diff --git a/drivers/staging/vt6655/iwctl.c b/drivers/staging/vt6655/iwctl.c
index 501cd64..7ce23b5 100644
--- a/drivers/staging/vt6655/iwctl.c
+++ b/drivers/staging/vt6655/iwctl.c
@@ -1621,17 +1621,24 @@ int iwctl_giwauth(struct net_device *dev,
int iwctl_siwgenie(struct net_device *dev,
struct iw_request_info *info,
struct iw_point *wrq,
- char *extra)
+ char __user *extra)
{
PSDevice pDevice = (PSDevice)netdev_priv(dev);
PSMgmtObject pMgmt = &(pDevice->sMgmtObj);
int ret = 0;
+ char length;

if (wrq->length) {
- if ((wrq->length < 2) || (extra[1]+2 != wrq->length)) {
- ret = -EINVAL;
- goto out;
- }
+ if (wrq->length < 2)
+ return -EINVAL;
+
+ ret = get_user(length, extra + 1);
+ if (ret)
+ return ret;
+
+ if (length + 2 != wrq->length)
+ return -EINVAL;
+
if (wrq->length > MAX_WPA_IE_LEN) {
ret = -ENOMEM;
goto out;
@@ -1654,7 +1661,7 @@ out://not completely ...not necessary in wpa_supplicant 0.5.8
int iwctl_giwgenie(struct net_device *dev,
struct iw_request_info *info,
struct iw_point *wrq,
- char *extra)
+ char __user *extra)
{
PSDevice pDevice = (PSDevice)netdev_priv(dev);
PSMgmtObject pMgmt = &(pDevice->sMgmtObj);
@@ -1801,18 +1808,23 @@ int iwctl_giwencodeext(struct net_device *dev,
int iwctl_siwmlme(struct net_device *dev,
struct iw_request_info *info,
struct iw_point *wrq,
- char *extra)
+ char __user *extra)
{
PSDevice pDevice = (PSDevice)netdev_priv(dev);
PSMgmtObject pMgmt = &(pDevice->sMgmtObj);
- struct iw_mlme *mlme = (struct iw_mlme *)extra;
+ struct iw_mlme mime;
+
int ret = 0;

- if (memcmp(pMgmt->abyCurrBSSID, mlme->addr.sa_data, ETH_ALEN)) {
+ ret = copy_from_user(&mime, extra, sizeof(mime));
+ if (ret)
+ return -EFAULT;
+
+ if (memcmp(pMgmt->abyCurrBSSID, mime.addr.sa_data, ETH_ALEN)) {
ret = -EINVAL;
return ret;
}
- switch (mlme->cmd) {
+ switch (mime.cmd) {
case IW_MLME_DEAUTH:
//this command seems to be not complete,please test it --einsnliu
//bScheduleCommand((void *) pDevice, WLAN_CMD_DEAUTH, (unsigned char *)&reason);
diff --git a/drivers/staging/vt6655/iwctl.h b/drivers/staging/vt6655/iwctl.h
index de0a337..7dd6310 100644
--- a/drivers/staging/vt6655/iwctl.h
+++ b/drivers/staging/vt6655/iwctl.h
@@ -176,12 +176,12 @@ int iwctl_giwauth(struct net_device *dev,
int iwctl_siwgenie(struct net_device *dev,
struct iw_request_info *info,
struct iw_point *wrq,
- char *extra);
+ char __user *extra);

int iwctl_giwgenie(struct net_device *dev,
struct iw_request_info *info,
struct iw_point *wrq,
- char *extra);
+ char __user *extra);

int iwctl_siwencodeext(struct net_device *dev,
struct iw_request_info *info,
@@ -196,7 +196,7 @@ int iwctl_giwencodeext(struct net_device *dev,
int iwctl_siwmlme(struct net_device *dev,
struct iw_request_info *info,
struct iw_point *wrq,
- char *extra);
+ char __user *extra);
#endif // #ifdef WPA_SUPPLICANT_DRIVER_WEXT_SUPPORT
//End Add -- //2008-0409-07, <Add> by Einsn Liu

--
1.8.5.5

2014-07-25 23:10:13

by Malcolm Priestley

[permalink] [raw]

Subject: Re: [PATCH] staging: vt6655: fix direct dereferencing of user pointer

Hi Guillaume

On 25/07/14 13:47, Guillaume Clement wrote:
> Sparse reported that the data from tagSCmdRequest is given by
> userspace, so it should be tagged as such.
extra is not in user space

All Wireless Extensions ioctl extra calls originate from
ioctl_standard_iw_point in wext-core.

Either through ioctl or iw_handler

All these functions should have been converted to iw_handler.

Regards

Malcolm

>
> Later, we were memcomparing and dereferencing it without first copying
> it, fix that as well.
>
> Signed-off-by: Guillaume Clement <[email protected]>
> ---
> drivers/staging/vt6655/iocmd.h | 2 +-
> drivers/staging/vt6655/iwctl.c | 32 ++++++++++++++++++++++----------
> drivers/staging/vt6655/iwctl.h | 6 +++---
> 3 files changed, 26 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/staging/vt6655/iocmd.h b/drivers/staging/vt6655/iocmd.h
> index e499f1b..dd12498 100644
> --- a/drivers/staging/vt6655/iocmd.h
> +++ b/drivers/staging/vt6655/iocmd.h
> @@ -100,7 +100,7 @@ typedef enum tagWZONETYPE {
> #pragma pack(1)
> typedef struct tagSCmdRequest {
> u8 name[16];
> - void *data;
> + void __user *data;
> u16 wResult;
> u16 wCmdCode;
> } SCmdRequest, *PSCmdRequest;
> diff --git a/drivers/staging/vt6655/iwctl.c b/drivers/staging/vt6655/iwctl.c
> index 501cd64..7ce23b5 100644
> --- a/drivers/staging/vt6655/iwctl.c
> +++ b/drivers/staging/vt6655/iwctl.c
> @@ -1621,17 +1621,24 @@ int iwctl_giwauth(struct net_device *dev,
> int iwctl_siwgenie(struct net_device *dev,
> struct iw_request_info *info,
> struct iw_point *wrq,
> - char *extra)
> + char __user *extra)
> {
> PSDevice pDevice = (PSDevice)netdev_priv(dev);
> PSMgmtObject pMgmt = &(pDevice->sMgmtObj);
> int ret = 0;
> + char length;
>
> if (wrq->length) {
> - if ((wrq->length < 2) || (extra[1]+2 != wrq->length)) {
> - ret = -EINVAL;
> - goto out;
> - }
> + if (wrq->length < 2)
> + return -EINVAL;
> +
> + ret = get_user(length, extra + 1);
> + if (ret)
> + return ret;
> +
> + if (length + 2 != wrq->length)
> + return -EINVAL;
> +
> if (wrq->length > MAX_WPA_IE_LEN) {
> ret = -ENOMEM;
> goto out;
> @@ -1654,7 +1661,7 @@ out://not completely ...not necessary in wpa_supplicant 0.5.8
> int iwctl_giwgenie(struct net_device *dev,
> struct iw_request_info *info,
> struct iw_point *wrq,
> - char *extra)
> + char __user *extra)
> {
> PSDevice pDevice = (PSDevice)netdev_priv(dev);
> PSMgmtObject pMgmt = &(pDevice->sMgmtObj);
> @@ -1801,18 +1808,23 @@ int iwctl_giwencodeext(struct net_device *dev,
> int iwctl_siwmlme(struct net_device *dev,
> struct iw_request_info *info,
> struct iw_point *wrq,
> - char *extra)
> + char __user *extra)
> {
> PSDevice pDevice = (PSDevice)netdev_priv(dev);
> PSMgmtObject pMgmt = &(pDevice->sMgmtObj);
> - struct iw_mlme *mlme = (struct iw_mlme *)extra;
> + struct iw_mlme mime;
> +
> int ret = 0;
>
> - if (memcmp(pMgmt->abyCurrBSSID, mlme->addr.sa_data, ETH_ALEN)) {
> + ret = copy_from_user(&mime, extra, sizeof(mime));
> + if (ret)
> + return -EFAULT;
> +
> + if (memcmp(pMgmt->abyCurrBSSID, mime.addr.sa_data, ETH_ALEN)) {
> ret = -EINVAL;
> return ret;
> }
> - switch (mlme->cmd) {
> + switch (mime.cmd) {
> case IW_MLME_DEAUTH:
> //this command seems to be not complete,please test it --einsnliu
> //bScheduleCommand((void *) pDevice, WLAN_CMD_DEAUTH, (unsigned char *)&reason);
> diff --git a/drivers/staging/vt6655/iwctl.h b/drivers/staging/vt6655/iwctl.h
> index de0a337..7dd6310 100644
> --- a/drivers/staging/vt6655/iwctl.h
> +++ b/drivers/staging/vt6655/iwctl.h
> @@ -176,12 +176,12 @@ int iwctl_giwauth(struct net_device *dev,
> int iwctl_siwgenie(struct net_device *dev,
> struct iw_request_info *info,
> struct iw_point *wrq,
> - char *extra);
> + char __user *extra);
>
> int iwctl_giwgenie(struct net_device *dev,
> struct iw_request_info *info,
> struct iw_point *wrq,
> - char *extra);
> + char __user *extra);
>
> int iwctl_siwencodeext(struct net_device *dev,
> struct iw_request_info *info,
> @@ -196,7 +196,7 @@ int iwctl_giwencodeext(struct net_device *dev,
> int iwctl_siwmlme(struct net_device *dev,
> struct iw_request_info *info,
> struct iw_point *wrq,
> - char *extra);
> + char __user *extra);
> #endif // #ifdef WPA_SUPPLICANT_DRIVER_WEXT_SUPPORT
> //End Add -- //2008-0409-07, <Add> by Einsn Liu
>
>

2014-07-25 23:16:53

by Malcolm Priestley

[permalink] [raw]

Subject: Re: [PATCH] staging: vt6655: fix direct dereferencing of user pointer

2014-07-26 08:25:01

by Guillaume Clement

[permalink] [raw]

Subject: Re: [PATCH] staging: vt6655: fix direct dereferencing of user pointer

Hi Malcolm,

On Sat, Jul 26, 2014 at 12:09:49AM +0100, Malcolm Priestley wrote:
> Hi Guillaume
>
> On 25/07/14 13:47, Guillaume Clement wrote:
> > Sparse reported that the data from tagSCmdRequest is given by
> > userspace, so it should be tagged as such.
> extra is not in user space
>

All right.

This is still confusing to me because, taking the SIOCSIWGENIE ioctl as
an example, in device_main.c, we have this code:

rc = iwctl_siwgenie(dev, NULL, &(wrq->u.data), wrq->u.data.pointer);

Here the extra parameter is the last one, wrq->u.data.pointer.

I was led to believe that wrq->u.data.pointer is in userspace (this was
reported by sparse actually) because the pointer field in data is
actually defined as __user.

I can understand it's not though, if it was pre-processed by another
function. Or if that pointer was never given by userspace in the first
place (if so, why would it be inside a __user pointer) ?

> All Wireless Extensions ioctl extra calls originate from
> ioctl_standard_iw_point in wext-core.
>
> Either through ioctl or iw_handler

After digging into the code a bit more, I think that there may still be
an issue. Are we really going through ioctl_standard_iw_point in this
specific case ?

In the iwctl_handler definition, we have no handler because of:

(iw_handler) NULL, // SIOCSIWGENIE

In the wireless_process_ioctl function, the returned handler should be
NULL if I understand correctly. I believe we're going through the "old
API" part with ndo_do_ioctl being called, which is consistent with the
fact that the code I showed earlier comes from that big switch in
device_ioctl in device_main.c.

This means we don't go to ioctl_standard_call, that would have called
ioctl_standard_iw_point, the function that should have done the
copy_from_user.

I didn't see a copy_from_user of the pointer field in the paths that I
think lead to device_ioctl in device_main.c.

Maybe the proper fix should have been to copy the content of
wrq->u.data.pointer to some buffer before calling iwctl_siwgenie, and
let this function not taking __user data?

This way, the driver is still ready to be converted to iw_handler
because it keeps the proper signature.

> All these functions should have been converted to iw_handler.

Yes certainly, but with no access to the real hardware for testing, I'd
rather not make deep changes like this for now.

2014-07-26 09:19:14

by Guillaume Clement

[permalink] [raw]

Subject: Re: [PATCH] staging: vt6655: fix direct dereferencing of user pointer

On Sat, Jul 26, 2014 at 10:24:30AM +0200, Guillaume CL?MENT wrote:
> Hi Malcolm,
>
> On Sat, Jul 26, 2014 at 12:09:49AM +0100, Malcolm Priestley wrote:
> > Hi Guillaume
> >
> > On 25/07/14 13:47, Guillaume Clement wrote:
> > > Sparse reported that the data from tagSCmdRequest is given by
> > > userspace, so it should be tagged as such.
> > extra is not in user space
> >
>
> All right.
>
> This is still confusing to me because, taking the SIOCSIWGENIE ioctl as
> an example, in device_main.c, we have this code:
>
> rc = iwctl_siwgenie(dev, NULL, &(wrq->u.data), wrq->u.data.pointer);
>
> Here the extra parameter is the last one, wrq->u.data.pointer.
>
> I was led to believe that wrq->u.data.pointer is in userspace (this was
> reported by sparse actually) because the pointer field in data is
> actually defined as __user.
>
>
By the way, the original code (before my patch) reads:

if ((wrq->length < 2) || (extra[1]+2 != wrq->length)) {
ret = -EINVAL;
goto out;
}
if (wrq->length > MAX_WPA_IE_LEN) {
ret = -ENOMEM;
goto out;
}
memset(pMgmt->abyWPAIE, 0, MAX_WPA_IE_LEN);
if (copy_from_user(pMgmt->abyWPAIE, extra, wrq->length)) {
ret = -EFAULT;
goto out;
}

Note extra[1] and later copy_from_user(x, extra, y).

If extra is not in userspace, we should not call copy_from_user, and if
it is, we should not dereference it. There is definitely something fishy
here.

2014-07-26 10:45:07

by Malcolm Priestley

[permalink] [raw]

Subject: Re: [PATCH] staging: vt6655: fix direct dereferencing of user pointer

On 26/07/14 10:18, Guillaume CL?MENT wrote:
> On Sat, Jul 26, 2014 at 10:24:30AM +0200, Guillaume CL?MENT wrote:
>> Hi Malcolm,
>>
>> On Sat, Jul 26, 2014 at 12:09:49AM +0100, Malcolm Priestley wrote:
>>> Hi Guillaume
>>>
>>> On 25/07/14 13:47, Guillaume Clement wrote:
>>>> Sparse reported that the data from tagSCmdRequest is given by
>>>> userspace, so it should be tagged as such.
>>> extra is not in user space
>>>
>>
>> All right.
>>
>> This is still confusing to me because, taking the SIOCSIWGENIE ioctl as
>> an example, in device_main.c, we have this code:
>>
>> rc = iwctl_siwgenie(dev, NULL, &(wrq->u.data), wrq->u.data.pointer);
>>
>> Here the extra parameter is the last one, wrq->u.data.pointer.
>>
>> I was led to believe that wrq->u.data.pointer is in userspace (this was
>> reported by sparse actually) because the pointer field in data is
>> actually defined as __user.
>>
>>
> By the way, the original code (before my patch) reads:
>
> if ((wrq->length < 2) || (extra[1]+2 != wrq->length)) {
> ret = -EINVAL;
> goto out;
> }
> if (wrq->length > MAX_WPA_IE_LEN) {
> ret = -ENOMEM;
> goto out;
> }
> memset(pMgmt->abyWPAIE, 0, MAX_WPA_IE_LEN);
> if (copy_from_user(pMgmt->abyWPAIE, extra, wrq->length)) {
> ret = -EFAULT;
> goto out;
> }
>
> Note extra[1] and later copy_from_user(x, extra, y).
>
> If extra is not in userspace, we should not call copy_from_user, and if
> it is, we should not dereference it. There is definitely something fishy
> here.
>

I got it wrong when the iw_handler is not present a standard ioctl is
called extra is in userspace.

Sorry for the noise.

2014-07-27 18:21:04

by Greg Kroah-Hartman

[permalink] [raw]

Subject: Re: [PATCH] staging: vt6655: fix direct dereferencing of user pointer

On Sat, Jul 26, 2014 at 11:44:57AM +0100, Malcolm Priestley wrote:
> On 26/07/14 10:18, Guillaume CL?MENT wrote:
> >On Sat, Jul 26, 2014 at 10:24:30AM +0200, Guillaume CL?MENT wrote:
> >>Hi Malcolm,
> >>
> >>On Sat, Jul 26, 2014 at 12:09:49AM +0100, Malcolm Priestley wrote:
> >>>Hi Guillaume
> >>>
> >>>On 25/07/14 13:47, Guillaume Clement wrote:
> >>>>Sparse reported that the data from tagSCmdRequest is given by
> >>>>userspace, so it should be tagged as such.
> >>>extra is not in user space
> >>>
> >>
> >>All right.
> >>
> >>This is still confusing to me because, taking the SIOCSIWGENIE ioctl as
> >>an example, in device_main.c, we have this code:
> >>
> >>rc = iwctl_siwgenie(dev, NULL, &(wrq->u.data), wrq->u.data.pointer);
> >>
> >>Here the extra parameter is the last one, wrq->u.data.pointer.
> >>
> >>I was led to believe that wrq->u.data.pointer is in userspace (this was
> >>reported by sparse actually) because the pointer field in data is
> >>actually defined as __user.
> >>
> >>
> >By the way, the original code (before my patch) reads:
> >
> > if ((wrq->length < 2) || (extra[1]+2 != wrq->length)) {
> > ret = -EINVAL;
> > goto out;
> > }
> > if (wrq->length > MAX_WPA_IE_LEN) {
> > ret = -ENOMEM;
> > goto out;
> > }
> > memset(pMgmt->abyWPAIE, 0, MAX_WPA_IE_LEN);
> > if (copy_from_user(pMgmt->abyWPAIE, extra, wrq->length)) {
> > ret = -EFAULT;
> > goto out;
> > }
> >
> >Note extra[1] and later copy_from_user(x, extra, y).
> >
> >If extra is not in userspace, we should not call copy_from_user, and if
> >it is, we should not dereference it. There is definitely something fishy
> >here.
> >
>
> I got it wrong when the iw_handler is not present a standard ioctl is called
> extra is in userspace.
>
> Sorry for the noise.

So this patch is acceptable?

confused,

greg k-h

2014-07-27 18:44:17

by Malcolm Priestley

[permalink] [raw]

Subject: Re: [PATCH] staging: vt6655: fix direct dereferencing of user pointer

On 27/07/14 19:21, Greg Kroah-Hartman wrote:
> On Sat, Jul 26, 2014 at 11:44:57AM +0100, Malcolm Priestley wrote:
>> On 26/07/14 10:18, Guillaume CL?MENT wrote:
>>> On Sat, Jul 26, 2014 at 10:24:30AM +0200, Guillaume CL?MENT wrote:
>>>> Hi Malcolm,
>>>>
>>>> On Sat, Jul 26, 2014 at 12:09:49AM +0100, Malcolm Priestley wrote:
>>>>> Hi Guillaume
>>>>>
>>>>> On 25/07/14 13:47, Guillaume Clement wrote:
>>>>>> Sparse reported that the data from tagSCmdRequest is given by
>>>>>> userspace, so it should be tagged as such.
>>>>> extra is not in user space
>>>>>
>>>>
>>>> All right.
>>>>
>>>> This is still confusing to me because, taking the SIOCSIWGENIE ioctl as
>>>> an example, in device_main.c, we have this code:
>>>>
>>>> rc = iwctl_siwgenie(dev, NULL, &(wrq->u.data), wrq->u.data.pointer);
>>>>
>>>> Here the extra parameter is the last one, wrq->u.data.pointer.
>>>>
>>>> I was led to believe that wrq->u.data.pointer is in userspace (this was
>>>> reported by sparse actually) because the pointer field in data is
>>>> actually defined as __user.
>>>>
>>>>
>>> By the way, the original code (before my patch) reads:
>>>
>>> if ((wrq->length < 2) || (extra[1]+2 != wrq->length)) {
>>> ret = -EINVAL;
>>> goto out;
>>> }
>>> if (wrq->length > MAX_WPA_IE_LEN) {
>>> ret = -ENOMEM;
>>> goto out;
>>> }
>>> memset(pMgmt->abyWPAIE, 0, MAX_WPA_IE_LEN);
>>> if (copy_from_user(pMgmt->abyWPAIE, extra, wrq->length)) {
>>> ret = -EFAULT;
>>> goto out;
>>> }
>>>
>>> Note extra[1] and later copy_from_user(x, extra, y).
>>>
>>> If extra is not in userspace, we should not call copy_from_user, and if
>>> it is, we should not dereference it. There is definitely something fishy
>>> here.
>>>
>>
>> I got it wrong when the iw_handler is not present a standard ioctl is called
>> extra is in userspace.
>>
>> Sorry for the noise.
>
> So this patch is acceptable?
>
Yes, this patch is okay.

> confused,
>
> greg k-h
>