Skip to contentSkip to conversation

Staging: rtl8712: Use mem_dup() instead of copy_from_user()

PATCH Superseded Cristina Opriceana 2 participants 4 messages Mar 22, 2015 – Mar 23, 2015 Staging

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Cristina Opriceana

author
#0 lore raw

PATCH 1/1 Staging: rtl8712: Use mem_dup() instead of copy_from_user()

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

Signed-off-by Cristina Opriceana <[email protected]>
---
 drivers/staging/rtl8712/rtl871x_ioctl_linux.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)
drivers/staging/rtl8712/rtl871x_ioctl_linux.c +3 -7
index 81f39c3..c39d031 100644
@@ -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;
Signature · 1 line
1.9.1
#1 lore raw
Quoted 14 lines · Cristina Opriceana

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

Signed-off-by Cristina Opriceana <[email protected]>
---
 drivers/staging/rtl8712/rtl871x_ioctl_linux.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)
drivers/staging/rtl8712/rtl871x_ioctl_linux.c +0 -0
index 81f39c3..c39d031 100644
@@ -1912,13 +1912,9 @@ static int r871x_mp_ioctl_hdl(struct net_device *dev,
 	bset = (u8)(p->flags & 0xFFFF);

drivers/staging/rtl8712/rtl871x_ioctl_linux.c static int r871x_mp_ioctl_hdl(struct net_device *dev, · line 1915

Quoted 10 lines
drivers/staging/rtl8712/rtl871x_ioctl_linux.c +1 -7
 	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

Quoted 15 lines
drivers/staging/rtl8712/rtl871x_ioctl_linux.c +2 -0
+	if (IS_ERR(pparmbuf)) {
+		ret = PTR_ERR(pparmbuf);
 		goto _r871x_mp_ioctl_hdl_exit;
 	}
 	poidparam = (struct mp_ioctl_param *)pparmbuf;
Signature · 8 lines
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 [email protected].
To post to this group, send email to [email protected].
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.
author
#2 lore raw
Quoted 34 lines · Julia Lawall
Quoted 14 lines · Cristina Opriceana

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

Signed-off-by Cristina Opriceana <[email protected]>
---
 drivers/staging/rtl8712/rtl871x_ioctl_linux.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)
drivers/staging/rtl8712/rtl871x_ioctl_linux.c +0 -0
index 81f39c3..c39d031 100644
@@ -1912,13 +1912,9 @@ static int r871x_mp_ioctl_hdl(struct net_device *dev,
 	bset = (u8)(p->flags & 0xFFFF);

drivers/staging/rtl8712/rtl871x_ioctl_linux.c static int r871x_mp_ioctl_hdl(struct net_device *dev, · line 1915

Quoted 10 lines
drivers/staging/rtl8712/rtl871x_ioctl_linux.c +1 -7
 	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.

Quoted 2 lines

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

#3 lore raw
Quoted 35 lines · Cristina Opriceana
Quoted 34 lines · Julia Lawall
Quoted 14 lines · Cristina Opriceana

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

Signed-off-by Cristina Opriceana <[email protected]>
---
 drivers/staging/rtl8712/rtl871x_ioctl_linux.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)
drivers/staging/rtl8712/rtl871x_ioctl_linux.c +0 -0
index 81f39c3..c39d031 100644
@@ -1912,13 +1912,9 @@ static int r871x_mp_ioctl_hdl(struct net_device *dev,
 	bset = (u8)(p->flags & 0xFFFF);

drivers/staging/rtl8712/rtl871x_ioctl_linux.c static int r871x_mp_ioctl_hdl(struct net_device *dev, · line 1915

Quoted 10 lines
drivers/staging/rtl8712/rtl871x_ioctl_linux.c +1 -7
 	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.

Quoted 8 lines
Quoted 2 lines

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