Skip to contentSkip to conversation

wifi: NULL terminate some user strings

PATCH In review Dan Carpenter 3 participants 7 messages Oct 1 – Oct 3

Subsystems (3)
CARL9170 Linux community wireless driver B43 wireless driver B43LEGACY wireless driver
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

cover wifi: NULL terminate some user strings

These strings come from the user and they need to be NUL terminated.

Dan Carpenter (3):
wifi: b43: don't pass unterminated string to sscanf()
wifi: b43legacy: debugfs: NUL terminate string in debugfs
wifi: carl9170: NUL terminate string in debugfs

 drivers/net/wireless/ath/carl9170/debug.c         | 5 +++--
 drivers/net/wireless/broadcom/b43/debugfs.c       | 2 +-
 drivers/net/wireless/broadcom/b43legacy/debugfs.c | 2 +-
 3 files changed, 5 insertions(+), 4 deletions(-)
Signature · 1 line
2.53.0
author
#1 lore raw

PATCH 1/3 wifi: b43: don't pass unterminated string to sscanf()

The "buf" buffer is a PAGE_SIZE and allocated with kzalloc(). We store a string from the user in it and then pass it to sscanf() in the write functions such as shm32write__write_file(). Only a few numbers are stored in the buffer so it never comes close to filling up the whole PAGE_SIZE. Reserve the last character of the page for a NUL terminator.

This is debugfs so it's root only.

Fixes 75388acd0cd8 ("[B43LEGACY]: add mac80211-based driver for legacy BCM43xx devices")
Signed-off-by Dan Carpenter <[email protected]>
---
 drivers/net/wireless/broadcom/b43/debugfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/wireless/broadcom/b43/debugfs.c +1 -1
index 31a1ff00c1a4..173ef5db90fc 100644
@@ -557,7 +557,7 @@ static ssize_t b43_debugfs_write(struct file *file,
 
 	if (!count)
 		return 0;
-	if (count > PAGE_SIZE)
+	if (count >= PAGE_SIZE)
 		return -E2BIG;
 	dev = file->private_data;
 	if (!dev)
Signature · 1 line
2.53.0
author
#2 lore raw

PATCH 2/3 wifi: b43legacy: debugfs: NUL terminate string in debugfs

In reply to Dan Carpenter

The "buf" buffer is PAGE_SIZE and allocated with kzalloc(). We store a string from the user in it and then pass it to sscanf() in the write functions such as tsf_write_file(). The buffer is only used to store a number so we never come close to filling up the whole PAGE_SIZE. Reserve the last character of the page for a NUL terminator.

This is debugfs so it's root only.

Fixes 75388acd0cd8 ("[B43LEGACY]: add mac80211-based driver for legacy BCM43xx devices")
Signed-off-by Dan Carpenter <[email protected]>
---
 drivers/net/wireless/broadcom/b43legacy/debugfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/wireless/broadcom/b43legacy/debugfs.c +1 -1
index a04d90d7307c..7ce8bcf12f0a 100644
@@ -259,7 +259,7 @@ static ssize_t b43legacy_debugfs_write(struct file *file,
 
 	if (!count)
 		return 0;
-	if (count > PAGE_SIZE)
+	if (count >= PAGE_SIZE)
 		return -E2BIG;
 	dev = file->private_data;
 	if (!dev)
Signature · 1 line
2.53.0
author
#3 lore raw

PATCH 3/3 wifi: carl9170: NUL terminate string in debugfs

In reply to Dan Carpenter

The "buf" buffer comes from the user. We use it to store a number or two so it doesn't need to be large. It gets passed to sscanf() in the write functions such as carl9170_debugfs_erp_write(). Ensure that buffer is NUL terminated.

This is debugfs so it's root only.

Fixes 00c4da27a421 ("carl9170: firmware parser and debugfs code")
Signed-off-by Dan Carpenter <[email protected]>
---
 drivers/net/wireless/ath/carl9170/debug.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/net/wireless/ath/carl9170/debug.c +3 -2
index 0498df2a2160..bc6c8e0b1d16 100644
@@ -119,7 +119,7 @@ static ssize_t carl9170_debugfs_write(struct file *file,
 	if (!count)
 		return 0;
 
-	if (count > PAGE_SIZE)
+	if (count >= PAGE_SIZE)
 		return -E2BIG;
 
 	ar = file->private_data;

@@ -131,7 +131,7 @@ static ssize_t carl9170_debugfs_write(struct file *file,
 	if (!dfops->write)
 		return -ENOSYS;
 
-	buf = vmalloc(count);
+	buf = vmalloc(count + 1);
 	if (!buf)
 		return -ENOMEM;
 

@@ -139,6 +139,7 @@ static ssize_t carl9170_debugfs_write(struct file *file,
 		err = -EFAULT;
 		goto out_free;
 	}
+	buf[count] = '\0';
 
 	if (mutex_trylock(&ar->mutex) == 0) {
 		err = -EAGAIN;
Signature · 1 line
2.53.0
Acked-by on v1 3/3
#4 lore raw

Re: [PATCH 3/3] wifi: carl9170: NUL terminate string in debugfs

Quoted 6 lines · Dan Carpenter

The "buf" buffer comes from the user. We use it to store a number or two so it doesn't need to be large. It gets passed to sscanf() in the write functions such as carl9170_debugfs_erp_write(). Ensure that buffer is NUL terminated.

This is debugfs so it's root only.

Sure.

Quoted 3 lines
Fixes 00c4da27a421 ("carl9170: firmware parser and debugfs code")
Signed-off-by Dan Carpenter <[email protected]>
Acked-by Christian Lamparter <[email protected]>
Quoted 9 lines
---
  drivers/net/wireless/ath/carl9170/debug.c | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)
drivers/net/wireless/ath/carl9170/debug.c +0 -0
index 0498df2a2160..bc6c8e0b1d16 100644

drivers/net/wireless/ath/carl9170/debug.c static ssize_t carl9170_debugfs_write(struct file *file, · line 122

Quoted 5 lines
drivers/net/wireless/ath/carl9170/debug.c +1 -1
@@ -119,7 +119,7 @@ static ssize_t carl9170_debugfs_write(struct file *file,
  	if (!count)
  		return 0;
 
-	if (count > PAGE_SIZE)
+	if (count >= PAGE_SIZE)

heh. It's unnecessary to change this for the max. two numbers we get. But I'm curious if this is a change that an AI tool added?

But yeah, it's still fine.

drivers/net/wireless/ath/carl9170/debug.c static ssize_t carl9170_debugfs_write(struct file *file, · line 134

Quoted 9 lines
drivers/net/wireless/ath/carl9170/debug.c +1 -1
  		return -E2BIG;
 
  	ar = file->private_data;

@@ -131,7 +131,7 @@ static ssize_t carl9170_debugfs_write(struct file *file,
  	if (!dfops->write)
  		return -ENOSYS;
 
-	buf = vmalloc(count);
+	buf = vmalloc(count + 1);

I think there's also a vzalloc...

drivers/net/wireless/ath/carl9170/debug.c static ssize_t carl9170_debugfs_write(struct file *file, · line 142

Quoted 8 lines
drivers/net/wireless/ath/carl9170/debug.c +1 -0
  	if (!buf)
  		return -ENOMEM;
 

@@ -139,6 +139,7 @@ static ssize_t carl9170_debugfs_write(struct file *file,
  		err = -EFAULT;
  		goto out_free;
  	}
+	buf[count] = '\0';

which would eliminate that. But yeah, this is fine as well.

Quoted 3 lines
drivers/net/wireless/ath/carl9170/debug.c +0 -0
  	if (mutex_trylock(&ar->mutex) == 0) {
  		err = -EAGAIN;
author
#5 lore raw

Re: [PATCH 3/3] wifi: carl9170: NUL terminate string in debugfs

Quoted 18 lines · Christian Lamparter
Quoted 6 lines · Dan Carpenter

The "buf" buffer comes from the user. We use it to store a number or two so it doesn't need to be large. It gets passed to sscanf() in the write functions such as carl9170_debugfs_erp_write(). Ensure that buffer is NUL terminated.

This is debugfs so it's root only.

Sure.

Quoted 3 lines
Fixes 00c4da27a421 ("carl9170: firmware parser and debugfs code")
Signed-off-by Dan Carpenter <[email protected]>
Acked-by Christian Lamparter <[email protected]>
Quoted 4 lines
---
  drivers/net/wireless/ath/carl9170/debug.c | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)
Quoted 11 lines

drivers/net/wireless/ath/carl9170/debug.c static ssize_t carl9170_debugfs_write(struct file *file, · line 121

Quoted 9 lines
drivers/net/wireless/ath/carl9170/debug.c +1 -1
index 0498df2a2160..bc6c8e0b1d16 100644
@@ -119,7 +119,7 @@ static ssize_t carl9170_debugfs_write(struct file *file,
  	if (!count)
  		return 0;
-	if (count > PAGE_SIZE)
+	if (count >= PAGE_SIZE)

heh. It's unnecessary to change this for the max. two numbers we get. But I'm curious if this is a change that an AI tool added?

No. The other two patches used kzalloc() to allocate their buffers so changing "> PAGE_SIZE" to ">= PAGE_SIZE" fixed the bug. But as I was writing this, I decided that there was no way I was going to let people allocat PAGE_SIZE + 1 bytes... :P Plus, since I was adding a byte later it kind of maintained the status quo to subtract one here.

I did use AI to write the Smatch check. ;)

Quoted 13 lines

But yeah, it's still fine.

drivers/net/wireless/ath/carl9170/debug.c static ssize_t carl9170_debugfs_write(struct file *file, · line 133

Quoted 7 lines
drivers/net/wireless/ath/carl9170/debug.c +1 -1
  		return -E2BIG;
  	ar = file->private_data;

@@ -131,7 +131,7 @@ static ssize_t carl9170_debugfs_write(struct file *file,
  	if (!dfops->write)
  		return -ENOSYS;
-	buf = vmalloc(count);
+	buf = vmalloc(count + 1);

I think there's also a vzalloc...

I was more considering changing this to kzalloc() but I decided that was too much change.

Quoted 8 lines

drivers/net/wireless/ath/carl9170/debug.c static ssize_t carl9170_debugfs_write(struct file *file, · line 142

Quoted 7 lines
drivers/net/wireless/ath/carl9170/debug.c +1 -0
  	if (!buf)
  		return -ENOMEM;

@@ -139,6 +139,7 @@ static ssize_t carl9170_debugfs_write(struct file *file,
  		err = -EFAULT;
  		goto out_free;
  	}
+	buf[count] = '\0';

which would eliminate that. But yeah, this is fine as well.

Thanks!

regards,
dan carpenter

Acked-by on v1 3/3
#6 lore raw

Re: [PATCH 3/3] wifi: carl9170: NUL terminate string in debugfs

In reply to Dan Carpenter
Quoted 9 lines · Dan Carpenter

The "buf" buffer comes from the user. We use it to store a number or two so it doesn't need to be large. It gets passed to sscanf() in the write functions such as carl9170_debugfs_erp_write(). Ensure that buffer is NUL terminated.

This is debugfs so it's root only.

Fixes 00c4da27a421 ("carl9170: firmware parser and debugfs code")
Signed-off-by Dan Carpenter <[email protected]>

Johannes, since the other two in the series go through your tree, you might as well take all three. I've assigned this one to you in patchwork.

Acked-by Jeff Johnson <[email protected]>