[PATCH v2 2/2] staging/rtl8192e: userspace ptr deref + incorrect declarations

[PATCH v2 2/2] staging/rtl8192e: userspace ptr deref + incorrect declarations

[Dominique van den Broeck]

. userspace pointer dereference ;

These issues have been fixed by a concurrent patch:
. missing inclusions of needed header files (fixed by concurrent patch);
. unrequired static function declaration (confusing another *.c file).

Signed-off-by: Dominique van den Broeck <domdevlin@free.fr>
---
v1 : I submit this patch as a result for Task #16 of the Eudyptula Challenge.
v2 : Resubmitted because of a conflit with commit 5169af2309f42bb4cb0ebfefe6bf8bc888d4ce33 .
     Successfully tested against commit b5c8d48bf8f4273a9fe680bd834f991005c8ab59 .
     I resubmit only the 2/2 one, since the 1/2 as already been accepted.

     Levente, still agree with you about numeric values that should be changed into symbols.
     This will form another future patch.

diff --git a/drivers/staging/rtl8192e/rtl8192e/rtl_wx.c b/drivers/staging/rtl8192e/rtl8192e/rtl_wx.c
index 498995d..d87cdfa 100644
--- a/drivers/staging/rtl8192e/rtl8192e/rtl_wx.c
+++ b/drivers/staging/rtl8192e/rtl8192e/rtl_wx.c
@@ -1131,11 +1131,18 @@ static int r8192_wx_set_PromiscuousMode(struct net_device *dev,
 	struct r8192_priv *priv = rtllib_priv(dev);
 	struct rtllib_device *ieee = priv->rtllib;
 
-	u32 *info_buf = (u32 *)(wrqu->data.pointer);
+	u32 info_buf[3];
 
-	u32 oid = info_buf[0];
-	u32 bPromiscuousOn = info_buf[1];
-	u32 bFilterSourceStationFrame = info_buf[2];
+	u32 oid;
+	u32 bPromiscuousOn;
+	u32 bFilterSourceStationFrame;
+
+	if (copy_from_user(info_buf, wrqu->data.pointer, sizeof(info_buf)))
+		return -EFAULT;
+
+	oid = info_buf[0];
+	bPromiscuousOn = info_buf[1];
+	bFilterSourceStationFrame = info_buf[2];
 
 	if (OID_RT_INTEL_PROMISCUOUS_MODE == oid) {
 		ieee->IntelPromiscuousModeInfo.bPromiscuousOn =

Re: [PATCH v2 2/2] staging/rtl8192e: userspace ptr deref + incorrect declarations

[Levente Kurusa]

[-- Attachment #1: Type: text/plain, Size: 2887 bytes --]

Hi,

On Sun, May 04, 2014 at 04:46:27PM +0200, Dominique van den Broeck wrote:
> . userspace pointer dereference ;
> 

What is that period in the commit message? And the semicolon?
You should also be a bit more specific. Also, the Subject line is
very bad. Better go with something like this:

staging: rtl8192e: fix userspace pointer dereference

And when you resend a patchset, please resend the full patchset.

> These issues have been fixed by a concurrent patch:
> . missing inclusions of needed header files (fixed by concurrent patch);
> . unrequired static function declaration (confusing another *.c file).

This is totally unneccessary.

> 
> Signed-off-by: Dominique van den Broeck <domdevlin@free.fr>
> ---
> v1 : I submit this patch as a result for Task #16 of the Eudyptula Challenge.
> v2 : Resubmitted because of a conflit with commit 5169af2309f42bb4cb0ebfefe6bf8bc888d4ce33 .
>      Successfully tested against commit b5c8d48bf8f4273a9fe680bd834f991005c8ab59 .
>      I resubmit only the 2/2 one, since the 1/2 as already been accepted.
> 
>      Levente, still agree with you about numeric values that should be changed into symbols.
>      This will form another future patch.

When you cite a commit please don't include the full hash, that is
non informational. Better put the first 7 characters of the hash and
the first line of the commit message as well in parantheses, like so:

5169af2 ("Staging: rtl8192e: Fix declaration of symbols")
(I even have a command for this in vim :-) )

Are you sure that 1/2 was applied to the staging tree?
It's unlikely that 1/2 is applied while 2/2 is left alone.

Oh, I am unable to find commit b5c8d48 in Linus' or staging-next.
In which tree is it?

> 
> diff --git a/drivers/staging/rtl8192e/rtl8192e/rtl_wx.c b/drivers/staging/rtl8192e/rtl8192e/rtl_wx.c
> index 498995d..d87cdfa 100644
> --- a/drivers/staging/rtl8192e/rtl8192e/rtl_wx.c
> +++ b/drivers/staging/rtl8192e/rtl8192e/rtl_wx.c
> @@ -1131,11 +1131,18 @@ static int r8192_wx_set_PromiscuousMode(struct net_device *dev,
>  	struct r8192_priv *priv = rtllib_priv(dev);
>  	struct rtllib_device *ieee = priv->rtllib;
>  
> -	u32 *info_buf = (u32 *)(wrqu->data.pointer);
> +	u32 info_buf[3];

Could you please as well remove that empty line in the declarations?

>  
> -	u32 oid = info_buf[0];
> -	u32 bPromiscuousOn = info_buf[1];
> -	u32 bFilterSourceStationFrame = info_buf[2];
> +	u32 oid;
> +	u32 bPromiscuousOn;
> +	u32 bFilterSourceStationFrame;
> +
> +	if (copy_from_user(info_buf, wrqu->data.pointer, sizeof(info_buf)))
> +		return -EFAULT;
> +
> +	oid = info_buf[0];
> +	bPromiscuousOn = info_buf[1];
> +	bFilterSourceStationFrame = info_buf[2];
>  
>  	if (OID_RT_INTEL_PROMISCUOUS_MODE == oid) {
>  		ieee->IntelPromiscuousModeInfo.bPromiscuousOn =

--
Regards,
Levente Kurusa

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [PATCH v2 2/2] staging/rtl8192e: userspace ptr deref + incorrect declarations

[Dominique van den Broeck]

Good evening,

Forgive my mistakes, I sent only a few patches yet and I'm still learning.
Nevertheless:

> What is that period in the commit message? And the semicolon?

Semicolons is what one use to ponctuate an enumerated list (at least
in french). In fact, it was their primary use before computer era.

Is there something wrong with it ? I check all my patches with
checkpatch.pl --strict before sending them and it didn't seem to
complain...

> You should also be a bit more specific. Also, the Subject line is
> very bad. Better go with something like this:
> 
> staging: rtl8192e: fix userspace pointer dereference

Right. I used the slash as a subpart of the tree. Didn't know what
was the best for this situation.

> And when you resend a patchset, please resend the full patchset.

I usually do but I've got an acceptation message for the first one
(see below).

> This is totally unneccessary.

Should at least have gone in the body below indeed.

> When you cite a commit please don't include the full hash, that is
> non informational. Better put the first 7 characters of the hash and
> the first line of the commit message as well in parantheses, like so:
>
> 5169af2 ("Staging: rtl8192e: Fix declaration of symbols")
> (I even have a command for this in vim :-) )

I'm interested. I use vim too.

> Are you sure that 1/2 was applied to the staging tree?
> It's unlikely that 1/2 is applied while 2/2 is left alone.

As stated before, I received the common automatic mail from
Greg KH regarding this one. So I'll now wait before I do a v3
for this issue. If so, I'll resent the complete set if required.

> Oh, I am unable to find commit b5c8d48 in Linus' or staging-next.
> In which tree is it?

It's linux-next. If I quote a commit, I should the tree as well,
indeed. But since the v1 was performed partially for the Eudyptula
Project and since it was a response to a modification request, 
I though it was implicit.

> Could you please as well remove that empty line in the declarations?

I'll do.
Cheers.

Re: [PATCH v2 2/2] staging/rtl8192e: userspace ptr deref + incorrect declarations

[Levente Kurusa]

[-- Attachment #1: Type: text/plain, Size: 3253 bytes --]

Hi,

On Mon, May 05, 2014 at 01:59:49AM +0200, Dominique van den Broeck wrote:
> 
> Good evening,
> 
> Forgive my mistakes, I sent only a few patches yet and I'm still learning.
> Nevertheless:

Not a problem at all, since that is what the challenge is for.

> 
> > What is that period in the commit message? And the semicolon?
> 
> Semicolons is what one use to ponctuate an enumerated list (at least
> in french). In fact, it was their primary use before computer era.
> 
> Is there something wrong with it ? I check all my patches with
> checkpatch.pl --strict before sending them and it didn't seem to
> complain...

Oh, I see now! Well I guess it's better to have a commit message
that says WHY the change is good and WHAT did it change, not just
a list of what you did.

i.e. in this case you could also inline the sparse warning or at least
a part of it.

> 
> > You should also be a bit more specific. Also, the Subject line is
> > very bad. Better go with something like this:
> > 
> > staging: rtl8192e: fix userspace pointer dereference
> 
> Right. I used the slash as a subpart of the tree. Didn't know what
> was the best for this situation.

That is good as well, I just prefer the latter one, but feel
free to use whichever you feel better. :-)

> 
> > And when you resend a patchset, please resend the full patchset.
> 
> I usually do but I've got an acceptation message for the first one
> (see below).
> 

In that case, that's good.

> > This is totally unneccessary.
> 
> Should at least have gone in the body below indeed.
> 
> > When you cite a commit please don't include the full hash, that is
> > non informational. Better put the first 7 characters of the hash and
> > the first line of the commit message as well in parantheses, like so:
> >
> > 5169af2 ("Staging: rtl8192e: Fix declaration of symbols")
> > (I even have a command for this in vim :-) )
> 
> I'm interested. I use vim too.
> 

I have this:
----------%<-----------
function! GetGitCommit(commit)
    exec ":.!git log --oneline --pretty=\"format:\\%h ('\\%s')\" ".a:commit." -1"
endfunction
---------->%-----------

> > Are you sure that 1/2 was applied to the staging tree?
> > It's unlikely that 1/2 is applied while 2/2 is left alone.
> 
> As stated before, I received the common automatic mail from
> Greg KH regarding this one. So I'll now wait before I do a v3
> for this issue. If so, I'll resent the complete set if required.

No, if that was applied it's unneccessary to resend the full set.

> 
> > Oh, I am unable to find commit b5c8d48 in Linus' or staging-next.
> > In which tree is it?
> 
> It's linux-next. If I quote a commit, I should the tree as well,
> indeed. But since the v1 was performed partially for the Eudyptula
> Project and since it was a response to a modification request, 
> I though it was implicit.

No, the tree's name is not needed. In fact, I should have checked it
in linux-next, but I only checked Linus', and staging-next thinking,
since you said it was applied, it was applied to staging-next. :-)
> 
> > Could you please as well remove that empty line in the declarations?
> 
> I'll do.
> Cheers.

Regards,
Levente Kurusa

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]