[PATCH] Staging: rtl8712: Use mem_dup() instead of copy_from_user()

[PATCH] Staging: rtl8712: Use mem_dup() instead of copy_from_user()

[Cristina Opriceana]

Use mem_dup() instead of its duplicated implementation in order
to simplify code. Found with coccinelle.

Signed-off-by: Cristina Opriceana <cristina.opriceana@gmail.com>
---
 drivers/staging/rtl8712/rtl871x_ioctl_linux.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
index 81f39c3..c39d031 100644
--- a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
+++ b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
@@ -1912,13 +1912,9 @@ static int r871x_mp_ioctl_hdl(struct net_device *dev,
 	bset = (u8)(p->flags & 0xFFFF);
 	len = p->length;
 	pparmbuf = NULL;
-	pparmbuf = kmalloc(len, GFP_ATOMIC);
-	if (pparmbuf == NULL) {
-		ret = -ENOMEM;
-		goto _r871x_mp_ioctl_hdl_exit;
-	}
-	if (copy_from_user(pparmbuf, p->pointer, len)) {
-		ret = -EFAULT;
+	pparmbuf = memdup_user(p->pointer, len);
+	if (IS_ERR(pparmbuf)) {
+		ret = PTR_ERR(pparmbuf);
 		goto _r871x_mp_ioctl_hdl_exit;
 	}
 	poidparam = (struct mp_ioctl_param *)pparmbuf;
-- 
1.9.1

Re: [Outreachy kernel] [PATCH] Staging: rtl8712: Use mem_dup() instead of copy_from_user()

[Julia Lawall]

On Sun, 22 Mar 2015, Cristina Opriceana wrote:

> Use mem_dup() instead of its duplicated implementation in order
> to simplify code. Found with coccinelle.
> 
> Signed-off-by: Cristina Opriceana <cristina.opriceana@gmail.com>
> ---
>  drivers/staging/rtl8712/rtl871x_ioctl_linux.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> index 81f39c3..c39d031 100644
> --- a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> +++ b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> @@ -1912,13 +1912,9 @@ static int r871x_mp_ioctl_hdl(struct net_device *dev,
>  	bset = (u8)(p->flags & 0xFFFF);
>  	len = p->length;
>  	pparmbuf = NULL;
> -	pparmbuf = kmalloc(len, GFP_ATOMIC);
> -	if (pparmbuf == NULL) {
> -		ret = -ENOMEM;
> -		goto _r871x_mp_ioctl_hdl_exit;
> -	}
> -	if (copy_from_user(pparmbuf, p->pointer, len)) {
> -		ret = -EFAULT;
> +	pparmbuf = memdup_user(p->pointer, len);

There is a potential problem with this.  memdup_user uses GFP_KERNEL 
(blocking allowed), while the original code used GFP_ATOMIC (blocking not 
allowed).  You could check whether any locks can be held on the way to 
this code, which would require GFP_ATOMIC.  On the other hand, it may be 
that copy_from_user can only be used when blocking is allowed.

julia


> +	if (IS_ERR(pparmbuf)) {
> +		ret = PTR_ERR(pparmbuf);
>  		goto _r871x_mp_ioctl_hdl_exit;
>  	}
>  	poidparam = (struct mp_ioctl_param *)pparmbuf;
> -- 
> 1.9.1
> 
> -- 
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20150322190400.GA1115%40Inspiron.
> For more options, visit https://groups.google.com/d/optout.
> 

Re: [Outreachy kernel] [PATCH] Staging: rtl8712: Use mem_dup() instead of copy_from_user()

[Cristina Opriceana]

On Du, 2015-03-22 at 21:04 +0100, Julia Lawall wrote:
> 
> On Sun, 22 Mar 2015, Cristina Opriceana wrote:
> 
> > Use mem_dup() instead of its duplicated implementation in order
> > to simplify code. Found with coccinelle.
> > 
> > Signed-off-by: Cristina Opriceana <cristina.opriceana@gmail.com>
> > ---
> >  drivers/staging/rtl8712/rtl871x_ioctl_linux.c | 10 +++-------
> >  1 file changed, 3 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> > index 81f39c3..c39d031 100644
> > --- a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> > +++ b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> > @@ -1912,13 +1912,9 @@ static int r871x_mp_ioctl_hdl(struct net_device *dev,
> >  	bset = (u8)(p->flags & 0xFFFF);
> >  	len = p->length;
> >  	pparmbuf = NULL;
> > -	pparmbuf = kmalloc(len, GFP_ATOMIC);
> > -	if (pparmbuf == NULL) {
> > -		ret = -ENOMEM;
> > -		goto _r871x_mp_ioctl_hdl_exit;
> > -	}
> > -	if (copy_from_user(pparmbuf, p->pointer, len)) {
> > -		ret = -EFAULT;
> > +	pparmbuf = memdup_user(p->pointer, len);
> 
> There is a potential problem with this.  memdup_user uses GFP_KERNEL 
> (blocking allowed), while the original code used GFP_ATOMIC (blocking not 
> allowed).  You could check whether any locks can be held on the way to 
> this code, which would require GFP_ATOMIC.  On the other hand, it may be 
> that copy_from_user can only be used when blocking is allowed.
> 
> julia
> 
As far as i understand, since copy_from_user() may sleep, it cannot be
used while holding a lock. So, the use of GFP_ATOMIC might be useless in
this context.
Either that or copy_from_user shouldn't be used, which is not the case
because the purpose of this function is to receive a command from the
userspace and call its associated handler.

Cristina

Re: [Outreachy kernel] [PATCH] Staging: rtl8712: Use mem_dup() instead of copy_from_user()

[Julia Lawall]

On Mon, 23 Mar 2015, Cristina Opriceana wrote:

> On Du, 2015-03-22 at 21:04 +0100, Julia Lawall wrote:
> >
> > On Sun, 22 Mar 2015, Cristina Opriceana wrote:
> >
> > > Use mem_dup() instead of its duplicated implementation in order
> > > to simplify code. Found with coccinelle.
> > >
> > > Signed-off-by: Cristina Opriceana <cristina.opriceana@gmail.com>
> > > ---
> > >  drivers/staging/rtl8712/rtl871x_ioctl_linux.c | 10 +++-------
> > >  1 file changed, 3 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> > > index 81f39c3..c39d031 100644
> > > --- a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> > > +++ b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> > > @@ -1912,13 +1912,9 @@ static int r871x_mp_ioctl_hdl(struct net_device *dev,
> > >  	bset = (u8)(p->flags & 0xFFFF);
> > >  	len = p->length;
> > >  	pparmbuf = NULL;
> > > -	pparmbuf = kmalloc(len, GFP_ATOMIC);
> > > -	if (pparmbuf == NULL) {
> > > -		ret = -ENOMEM;
> > > -		goto _r871x_mp_ioctl_hdl_exit;
> > > -	}
> > > -	if (copy_from_user(pparmbuf, p->pointer, len)) {
> > > -		ret = -EFAULT;
> > > +	pparmbuf = memdup_user(p->pointer, len);
> >
> > There is a potential problem with this.  memdup_user uses GFP_KERNEL
> > (blocking allowed), while the original code used GFP_ATOMIC (blocking not
> > allowed).  You could check whether any locks can be held on the way to
> > this code, which would require GFP_ATOMIC.  On the other hand, it may be
> > that copy_from_user can only be used when blocking is allowed.
> >
> > julia
> >
> As far as i understand, since copy_from_user() may sleep, it cannot be
> used while holding a lock. So, the use of GFP_ATOMIC might be useless in
> this context.
> Either that or copy_from_user shouldn't be used, which is not the case
> because the purpose of this function is to receive a command from the
> userspace and call its associated handler.

OK, it would be good to add a note about this to the commit message.

julia