isdn: return -EFAULT if copy_from_user() fails

PATCH Applied Dan Carpenter 3 participants 7 messages Dec 10, 2010 – Dec 11, 2010

Subsystems (3)

Networking Network drivers Kernel janitors

lore

Share & tools

lore

Dan Carpenter

author

2010-12-10 12:40 UTC #0 lore raw

PATCH 1/1 isdn: return -EFAULT if copy_from_user() fails

We should be returning -EFAULT here.

Mostly this patch is to silence a smatch warning. The upper levels of this driver turn all non-zero return values from isar_load_firmware() into 1.

Signed-off-by Dan Carpenter <[email protected]>

drivers/isdn/hisax/isar.c +2 -2

index 2e72227..9cd4829 100644
@@ -212,9 +212,9 @@ isar_load_firmware(struct IsdnCardState *cs, u_char __user *buf)
 	cs->debug &= ~(L1_DEB_HSCX | L1_DEB_HSCX_FIFO);
 #endif
 	
-	if ((ret = copy_from_user(&size, p, sizeof(int)))) {
+	if (copy_from_user(&size, p, sizeof(int))) {
 		printk(KERN_ERR"isar_load_firmware copy_from_user ret %d\n", ret);
-		return ret;
+		return -EFAULT;
 	}
 	p += sizeof(int);
 	printk(KERN_DEBUG"isar_load_firmware size: %d\n", size);

David Miller

maintainer Applied

2010-12-11 00:15 UTC #1 lore raw

From: Dan Carpenter <[email protected]>
Date: Fri, 10 Dec 2010 15:40:09 +0300

Quoted 7 lines

We should be returning -EFAULT here.
Mostly this patch is to silence a smatch warning. The upper levels of this driver turn all non-zero return values from isar_load_firmware() into 1.
Signed-off-by Dan Carpenter <[email protected]>

Applied.

Nicolas Kaiser

2010-12-11 00:41 UTC #2 lore raw

In reply to Dan Carpenter

Ahem, we're printing this return value:

* Dan Carpenter <[email protected]>:

drivers/isdn/hisax/isar.c isar_load_firmware(struct IsdnCardState *cs, u_char __user *buf) · line 215

Quoted 1 line

drivers/isdn/hisax/isar.c +0 -1
-	if ((ret = copy_from_user(&size, p, sizeof(int)))) {

^^^

drivers/isdn/hisax/isar.c isar_load_firmware(struct IsdnCardState *cs, u_char __user *buf) · line 216

Quoted 2 lines

drivers/isdn/hisax/isar.c +1 -0

+	if (copy_from_user(&size, p, sizeof(int))) {
 		printk(KERN_ERR"isar_load_firmware copy_from_user ret %d\n", ret);

^^^

drivers/isdn/hisax/isar.c isar_load_firmware(struct IsdnCardState *cs, u_char __user *buf) · line 217

Quoted 2 lines

drivers/isdn/hisax/isar.c +1 -1
-		return ret;
+		return -EFAULT;

Cheers,
Nicolas Kaiser

David Miller

maintainer

2010-12-11 00:46 UTC #3 lore raw

From: Nicolas Kaiser <[email protected]>
Date: Sat, 11 Dec 2010 01:41:54 +0100

Quoted 10 lines

Ahem, we're printing this return value:
* Dan Carpenter <[email protected]>:
drivers/isdn/hisax/isar.c isar_load_firmware(struct IsdnCardState *cs, u_char __user *buf) · line 215
Quoted 1 line
drivers/isdn/hisax/isar.c +0 -1
-	if ((ret = copy_from_user(&size, p, sizeof(int)))) {
             ^^^
drivers/isdn/hisax/isar.c isar_load_firmware(struct IsdnCardState *cs, u_char __user *buf) · line 216
Quoted 2 lines
drivers/isdn/hisax/isar.c +1 -0
+	if (copy_from_user(&size, p, sizeof(int))) {
 		printk(KERN_ERR"isar_load_firmware copy_from_user ret %d\n", ret);
                                                                             ^^^
drivers/isdn/hisax/isar.c isar_load_firmware(struct IsdnCardState *cs, u_char __user *buf) · line 217
Quoted 2 lines
drivers/isdn/hisax/isar.c +1 -1
-		return ret;
+		return -EFAULT;

I'll fix this, thanks.

David Miller

maintainer

2010-12-11 00:49 UTC #4 lore raw

From: David Miller <[email protected]>
Date: Fri, 10 Dec 2010 16:47:21 -0800 (PST)

Quoted 8 lines

From: Nicolas Kaiser <[email protected]>
Date: Sat, 11 Dec 2010 01:41:54 +0100
Quoted 5 lines
Ahem, we're printing this return value:
* Dan Carpenter <[email protected]>:
Review
Quoted 1 line
(quoted diff) +0 -1
-	if ((ret = copy_from_user(&size, p, sizeof(int)))) {
             ^^^

Quoted 7 lines

Quoted 5 lines

Review

Quoted 2 lines

(quoted diff) +1 -0

+	if (copy_from_user(&size, p, sizeof(int))) {
 		printk(KERN_ERR"isar_load_firmware copy_from_user ret %d\n", ret);

^^^

Review

Quoted 2 lines

(quoted diff) +1 -1
-		return ret;
+		return -EFAULT;

I'll fix this, thanks.

As follows:

Quoted 1 line

From cf108fdd482e80161128c2ed01e7f4fb5bc728b9 Mon Sep 17 00:00:00 2001

From: David S. Miller <[email protected]>
Date: Fri, 10 Dec 2010 16:49:24 -0800
Subject: [PATCH] isdn: Fix printed out copy_from_user() return value after previous change.

Signed-off-by David S. Miller <[email protected]>

---
 drivers/isdn/hisax/isar.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

drivers/isdn/hisax/isar.c +4 -3

index c9cd4d2..313ce2d 100644
@@ -189,7 +189,7 @@ ISARVersion(struct IsdnCardState *cs, char *s)
 static int
 isar_load_firmware(struct IsdnCardState *cs, u_char __user *buf)
 {
-	int ret, size, cnt, debug;
+	int cfu_ret, size, cnt, debug;
 	u_char len, nom, noc;
 	u_short sadr, left, *sp;
 	u_char __user *p = buf;

@@ -212,8 +212,9 @@ isar_load_firmware(struct IsdnCardState *cs, u_char __user *buf)
 	cs->debug &= ~(L1_DEB_HSCX | L1_DEB_HSCX_FIFO);
 #endif
 	
-	if (copy_from_user(&size, p, sizeof(int))) {
-		printk(KERN_ERR"isar_load_firmware copy_from_user ret %d\n", ret);
+	cfu_ret = copy_from_user(&size, p, sizeof(int));
+	if (cfu_ret) {
+		printk(KERN_ERR"isar_load_firmware copy_from_user ret %d\n", cfu_ret);
 		return -EFAULT;
 	}
 	p += sizeof(int);

Signature · 1 line

1.7.3.2

David Miller

maintainer

2010-12-11 00:52 UTC #5 lore raw

From: David Miller <[email protected]>
Date: Fri, 10 Dec 2010 16:50:03 -0800 (PST)

Quoted 14 lines

From: David Miller <[email protected]>
Date: Fri, 10 Dec 2010 16:47:21 -0800 (PST)
Quoted 11 lines
From: Nicolas Kaiser <[email protected]>
Date: Sat, 11 Dec 2010 01:41:54 +0100
Quoted 8 lines
Ahem, we're printing this return value:
* Dan Carpenter <[email protected]>:
Review
Quoted 1 line
(quoted diff) +0 -1
-	if ((ret = copy_from_user(&size, p, sizeof(int)))) {
             ^^^
Review
Quoted 2 lines
(quoted diff) +1 -0
+	if (copy_from_user(&size, p, sizeof(int))) {
 		printk(KERN_ERR"isar_load_firmware copy_from_user ret %d\n", ret);
                                                                             ^^^

Quoted 6 lines

Quoted 4 lines
Quoted 2 lines
Review
Quoted 2 lines
(quoted diff) +1 -1
-		return ret;
+		return -EFAULT;
I'll fix this, thanks.
As follows:

Ugh, that was buggy, let's try this :-)

--------------------
isdn: Fix printed out copy_from_user() return value after previous change.

Signed-off-by David S. Miller <[email protected]>

---
 drivers/isdn/hisax/isar.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

drivers/isdn/hisax/isar.c +4 -3

index c9cd4d2..d4cce33 100644
@@ -189,7 +189,7 @@ ISARVersion(struct IsdnCardState *cs, char *s)
 static int
 isar_load_firmware(struct IsdnCardState *cs, u_char __user *buf)
 {
-	int ret, size, cnt, debug;
+	int cfu_ret, ret, size, cnt, debug;
 	u_char len, nom, noc;
 	u_short sadr, left, *sp;
 	u_char __user *p = buf;

@@ -212,8 +212,9 @@ isar_load_firmware(struct IsdnCardState *cs, u_char __user *buf)
 	cs->debug &= ~(L1_DEB_HSCX | L1_DEB_HSCX_FIFO);
 #endif
 	
-	if (copy_from_user(&size, p, sizeof(int))) {
-		printk(KERN_ERR"isar_load_firmware copy_from_user ret %d\n", ret);
+	cfu_ret = copy_from_user(&size, p, sizeof(int));
+	if (cfu_ret) {
+		printk(KERN_ERR"isar_load_firmware copy_from_user ret %d\n", cfu_ret);
 		return -EFAULT;
 	}
 	p += sizeof(int);

Signature · 1 line

1.7.3.2

Dan Carpenter

author

2010-12-11 05:07 UTC #6 lore raw

Quoted 2 lines

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Ahem, we're printing this return value:

Doh! Thanks for that guys.

regards,
dan carpenter