mwifiex: fix mwifiex_rdeeprom_read()

PATCH Superseded Dan Carpenter 3 participants 6 messages Aug 20, 2015 – Sep 29, 2015

Subsystems (2)

Kernel janitors Networking drivers (wireless)

lore

Share & tools

lore

Dan Carpenter

author

2015-08-20 10:30 UTC #0 lore raw

PATCH 1/1 mwifiex: fix mwifiex_rdeeprom_read()

There were several bugs here.

1) The done label was in the wrong place so we didn't copy any information out when there was no command given.

2) We were using PAGE_SIZE as the size of the buffer instead of

    "PAGE_SIZE - pos".

3) snprintf() returns the number of characters that would have been printed if there were enough space. If there was not enough space (and we had fixed the memory corruption bug #2) then it would result in an information leak when we do simple_read_from_buffer(). I've changed it to use scnprintf() instead.

I also removed the initialization at the start of the function, because I thought it made the code a little more clear.

Fixes 5e6e3a92b9a4 ('wireless: mwifiex: initial commit for Marvell mwifiex driver')

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

drivers/net/wireless/mwifiex/debugfs.c +5 -5

index 5a0636d4..bd2f5d2 100644
@@ -731,7 +731,7 @@ mwifiex_rdeeprom_read(struct file *file, char __user *ubuf,
 		(struct mwifiex_private *) file->private_data;
 	unsigned long addr = get_zeroed_page(GFP_KERNEL);
 	char *buf = (char *) addr;
-	int pos = 0, ret = 0, i;
+	int pos, ret, i;
 	u8 value[MAX_EEPROM_DATA];
 
 	if (!buf)

@@ -739,7 +739,7 @@ mwifiex_rdeeprom_read(struct file *file, char __user *ubuf,
 
 	if (saved_offset = -1) {
 		/* No command has been given */
-		pos += snprintf(buf, PAGE_SIZE, "0");
+		pos = snprintf(buf, PAGE_SIZE, "0");
 		goto done;
 	}

@@ -751,14 +751,14 @@ mwifiex_rdeeprom_read(struct file *file, char __user *ubuf,
 		goto done;
 	}
 
-	pos += snprintf(buf, PAGE_SIZE, "%d %d ", saved_offset, saved_bytes);
+	pos = snprintf(buf, PAGE_SIZE, "%d %d ", saved_offset, saved_bytes);
 
 	for (i = 0; i < saved_bytes; i++)
-		pos += snprintf(buf + strlen(buf), PAGE_SIZE, "%d ", value[i]);
+		pos += scnprintf(buf + pos, PAGE_SIZE - pos, "%d ", value[i]);
 
+done:
 	ret = simple_read_from_buffer(ubuf, count, ppos, buf, pos);
 
-done:
 	free_page(addr);
 	return ret;
 }

Amitkumar Karwar

Acked-by on v1 1/1

2015-08-25 06:56 UTC #1 lore raw

Hi Dan,

Quoted 66 lines

From: Dan Carpenter [mailto:[email protected]]
Sent: Thursday, August 20, 2015 4:01 PM
To: Amitkumar Karwar; Bing Zhao
Cc: Nishant Sarmukadam; Kalle Valo; [email protected]; [email protected]
Subject: [patch] mwifiex: fix mwifiex_rdeeprom_read()
There were several bugs here.
1) The done label was in the wrong place so we didn't copy any information out when there was no command given.
2) We were using PAGE_SIZE as the size of the buffer instead of
    "PAGE_SIZE - pos".
3) snprintf() returns the number of characters that would have been printed if there were enough space. If there was not enough space (and we had fixed the memory corruption bug #2) then it would result in an information leak when we do simple_read_from_buffer(). I've changed it to use scnprintf() instead.
I also removed the initialization at the start of the function, because I thought it made the code a little more clear.
Fixes 5e6e3a92b9a4 ('wireless: mwifiex: initial commit for Marvell
mwifiex driver')
Signed-off-by Dan Carpenter <[email protected]>
drivers/net/wireless/mwifiex/debugfs.c +1 -1
index 5a0636d4..bd2f5d2 100644
@@ -731,7 +731,7 @@ mwifiex_rdeeprom_read(struct file *file, char __user
 *ubuf,
 		(struct mwifiex_private *) file->private_data;
 	unsigned long addr = get_zeroed_page(GFP_KERNEL);
 	char *buf = (char *) addr;
-	int pos = 0, ret = 0, i;
+	int pos, ret, i;
 	u8 value[MAX_EEPROM_DATA];
 
drivers/net/wireless/mwifiex/debugfs.c +2 -3
 	if (!buf)
@@ -739,7 +739,7 @@ mwifiex_rdeeprom_read(struct file *file, char __user
 *ubuf,
 
 	if (saved_offset = -1) {
 		/* No command has been given */
-		pos += snprintf(buf, PAGE_SIZE, "0");
+		pos = snprintf(buf, PAGE_SIZE, "0");
 		goto done;
 	}
@@ -751,14 +751,14 @@ mwifiex_rdeeprom_read(struct file *file, char
 __user *ubuf,
 		goto done;
 	}
 
-	pos += snprintf(buf, PAGE_SIZE, "%d %d ", saved_offset,
 saved_bytes);
+	pos = snprintf(buf, PAGE_SIZE, "%d %d ", saved_offset,
 saved_bytes);
 
 	for (i = 0; i < saved_bytes; i++)
-		pos += snprintf(buf + strlen(buf), PAGE_SIZE, "%d ",
 value[i]);

drivers/net/wireless/mwifiex/debugfs.c mwifiex_rdeeprom_read(struct file *file, char · line 764

Quoted 10 lines

drivers/net/wireless/mwifiex/debugfs.c +2 -1

+		pos += scnprintf(buf + pos, PAGE_SIZE - pos, "%d ",
 value[i]);
 
+done:
 	ret = simple_read_from_buffer(ubuf, count, ppos, buf, pos);
 
-done:
 	free_page(addr);
 	return ret;
 }

Looks good.

Acked-by Amitkumar Karwar <[email protected]>

Regards,
Amitkumar

Kalle Valo

2015-08-25 12:29 UTC #2 lore raw

In reply to Dan Carpenter

Quoted 14 lines · Dan Carpenter

There were several bugs here.
1) The done label was in the wrong place so we didn't copy any information out when there was no command given.
2) We were using PAGE_SIZE as the size of the buffer instead of
    "PAGE_SIZE - pos".
3) snprintf() returns the number of characters that would have been printed if there were enough space. If there was not enough space (and we had fixed the memory corruption bug #2) then it would result in an information leak when we do simple_read_from_buffer(). I've changed it to use scnprintf() instead.

Quoted 5 lines

I also removed the initialization at the start of the function, because I thought it made the code a little more clear.
Fixes 5e6e3a92b9a4 ('wireless: mwifiex: initial commit for Marvell mwifiex driver')
Signed-off-by Dan Carpenter <[email protected]>

I see a new warning:

drivers/net/wireless/mwifiex/debugfs.c:760:31: warning: pos may be used uninitialized in this function [-Wuninitialized]

Is it a valid warning or should I apply the patch anyway?

Signature · 1 line

Kalle Valo

Dan Carpenter

author

2015-09-21 16:19 UTC #3 lore raw

PATCH v2 1/1 mwifiex: fix mwifiex_rdeeprom_read()

There were several bugs here.

1) The done label was in the wrong place so we didn't copy any information out when there was no command given.

2) We were using PAGE_SIZE as the size of the buffer instead of

    "PAGE_SIZE - pos".

I also removed the initialization at the start of the function, because I thought it made the code a little more clear.

Fixes 5e6e3a92b9a4 ('wireless: mwifiex: initial commit for Marvell mwifiex driver')

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

---
v2: My first part introduced a bug. :(

Sorry for the delay on this.

drivers/net/wireless/mwifiex/debugfs.c +7 -7

index 5a0636d4..5583856 100644
@@ -731,7 +731,7 @@ mwifiex_rdeeprom_read(struct file *file, char __user *ubuf,
 		(struct mwifiex_private *) file->private_data;
 	unsigned long addr = get_zeroed_page(GFP_KERNEL);
 	char *buf = (char *) addr;
-	int pos = 0, ret = 0, i;
+	int pos, ret, i;
 	u8 value[MAX_EEPROM_DATA];
 
 	if (!buf)

@@ -739,7 +739,7 @@ mwifiex_rdeeprom_read(struct file *file, char __user *ubuf,
 
 	if (saved_offset = -1) {
 		/* No command has been given */
-		pos += snprintf(buf, PAGE_SIZE, "0");
+		pos = snprintf(buf, PAGE_SIZE, "0");
 		goto done;
 	}

@@ -748,17 +748,17 @@ mwifiex_rdeeprom_read(struct file *file, char __user *ubuf,
 				  (u16) saved_bytes, value);
 	if (ret) {
 		ret = -EINVAL;
-		goto done;
+		goto out_free;
 	}
 
-	pos += snprintf(buf, PAGE_SIZE, "%d %d ", saved_offset, saved_bytes);
+	pos = snprintf(buf, PAGE_SIZE, "%d %d ", saved_offset, saved_bytes);
 
 	for (i = 0; i < saved_bytes; i++)
-		pos += snprintf(buf + strlen(buf), PAGE_SIZE, "%d ", value[i]);
-
-	ret = simple_read_from_buffer(ubuf, count, ppos, buf, pos);
+		pos += scnprintf(buf + pos, PAGE_SIZE - pos, "%d ", value[i]);
 
 done:
+	ret = simple_read_from_buffer(ubuf, count, ppos, buf, pos);
+out_free:
 	free_page(addr);
 	return ret;
 }

Amitkumar Karwar

Acked-by on v2 1/1

2015-09-22 10:58 UTC #4 lore raw

Hi Dan,

Quoted 75 lines

From: Dan Carpenter [mailto:[email protected]]
Sent: Monday, September 21, 2015 9:50 PM
To: Amitkumar Karwar
Cc: Nishant Sarmukadam; Kalle Valo; [email protected]; [email protected]
Subject: [patch v2] mwifiex: fix mwifiex_rdeeprom_read()
There were several bugs here.
1) The done label was in the wrong place so we didn't copy any information out when there was no command given.
2) We were using PAGE_SIZE as the size of the buffer instead of
    "PAGE_SIZE - pos".
3) snprintf() returns the number of characters that would have been printed if there were enough space. If there was not enough space (and we had fixed the memory corruption bug #2) then it would result in an information leak when we do simple_read_from_buffer(). I've changed it to use scnprintf() instead.
I also removed the initialization at the start of the function, because I thought it made the code a little more clear.
Fixes 5e6e3a92b9a4 ('wireless: mwifiex: initial commit for Marvell
mwifiex driver')
Signed-off-by Dan Carpenter <[email protected]>
---
v2: My first part introduced a bug. :(
Sorry for the delay on this.
drivers/net/wireless/mwifiex/debugfs.c +1 -1
index 5a0636d4..5583856 100644
@@ -731,7 +731,7 @@ mwifiex_rdeeprom_read(struct file *file, char __user
 *ubuf,
 		(struct mwifiex_private *) file->private_data;
 	unsigned long addr = get_zeroed_page(GFP_KERNEL);
 	char *buf = (char *) addr;
-	int pos = 0, ret = 0, i;
+	int pos, ret, i;
 	u8 value[MAX_EEPROM_DATA];
 
drivers/net/wireless/mwifiex/debugfs.c +3 -5
 	if (!buf)
@@ -739,7 +739,7 @@ mwifiex_rdeeprom_read(struct file *file, char __user
 *ubuf,
 
 	if (saved_offset = -1) {
 		/* No command has been given */
-		pos += snprintf(buf, PAGE_SIZE, "0");
+		pos = snprintf(buf, PAGE_SIZE, "0");
 		goto done;
 	}
@@ -748,17 +748,17 @@ mwifiex_rdeeprom_read(struct file *file, char
 __user *ubuf,
 				  (u16) saved_bytes, value);
 	if (ret) {
 		ret = -EINVAL;
-		goto done;
+		goto out_free;
 	}
 
-	pos += snprintf(buf, PAGE_SIZE, "%d %d ", saved_offset,
 saved_bytes);
+	pos = snprintf(buf, PAGE_SIZE, "%d %d ", saved_offset,
 saved_bytes);
 
 	for (i = 0; i < saved_bytes; i++)
-		pos += snprintf(buf + strlen(buf), PAGE_SIZE, "%d ",
 value[i]);
-

drivers/net/wireless/mwifiex/debugfs.c mwifiex_rdeeprom_read(struct file *file, char · line 764

Quoted 10 lines

drivers/net/wireless/mwifiex/debugfs.c +3 -1

-	ret = simple_read_from_buffer(ubuf, count, ppos, buf, pos);
+		pos += scnprintf(buf + pos, PAGE_SIZE - pos, "%d ",
 value[i]);
 
 done:
+	ret = simple_read_from_buffer(ubuf, count, ppos, buf, pos);
+out_free:
 	free_page(addr);
 	return ret;
 }

Acked-by Amitkumar Karwar <[email protected]>

Regards,
Amitkumar

Kalle Valo

Applied

2015-09-29 08:09 UTC #5 lore raw

In reply to Dan Carpenter

Quoted 15 lines

There were several bugs here.
1) The done label was in the wrong place so we didn't copy any information out when there was no command given.
2) We were using PAGE_SIZE as the size of the buffer instead of
    "PAGE_SIZE - pos".
3) snprintf() returns the number of characters that would have been printed if there were enough space. If there was not enough space (and we had fixed the memory corruption bug #2) then it would result in an information leak when we do simple_read_from_buffer(). I've changed it to use scnprintf() instead.

Quoted 6 lines

I also removed the initialization at the start of the function, because I thought it made the code a little more clear.
Fixes 5e6e3a92b9a4 ('wireless: mwifiex: initial commit for Marvell mwifiex driver')
Signed-off-by Dan Carpenter <[email protected]>
Acked-by Amitkumar Karwar <[email protected]>

Thanks, applied to wireless-drivers-next.git.

Kalle Valo