[PATCH net] net: amd-xgbe: Replace kasprintf() with snprintf() for debugfs name

[PATCH net] net: amd-xgbe: Replace kasprintf() with snprintf() for debugfs name

[Tom Lendacky]

It was reported that using kasprintf() produced a kernel warning as the
network interface name was being changed by udev rules at the same time
that the debugfs entry for the device was being created.

Kernel panic - not syncing: panic_on_warn set ...
CPU: 1 PID: 2219 Comm: qemu-event Tainted: G           O      5.4.134 #1
Hardware name: <redacted>
Call Trace:
  dump_stack+0x50/0x63
  panic+0x102/0x2d2
  ? kvasprintf+0xb5/0xc0
  __warn.cold+0x20/0x20
  ? kvasprintf+0xb5/0xc0
  report_bug+0xcc/0x100
  do_error_trap+0xa3/0xc0
  ? kvasprintf+0xb5/0xc0
  do_invalid_op+0x37/0x40
  ? kvasprintf+0xb5/0xc0
  invalid_op+0x28/0x30
RIP: 0010:kvasprintf+0xb5/0xc0
Code: 28 00 00 00 75 28 48 83 c4 20 4c 89 e8 5d 41 5c 41 5d 41 5e 41 5f c3 4c 89 f1 89 c2 89 ee 48 c7 c7 d8 1e 0c a8 e8 b0 a5 3a 00 <0f> 0b eb c8 e8 92 cc cd ff 66 90 41 55 41 89 fd 41 54 49 89 d4 55
RSP: 0018:ffffa79f80e37c40 EFLAGS: 00010286
RAX: 0000000000000000 RBX: ffff9b71b633c7c0 RCX: 0000000000000000
RDX: 0000000000000001 RSI: ffffffffa8986566 RDI: 00000000ffffffff
RBP: 000000000000000d R08: 0000004aafbb5f98 R09: 0000000000000046
R10: ffffffffa8986900 R11: 00000000a8986553 R12: ffffa79f80e37c90
R13: ffff9b71f0dcdba0 R14: ffffffffc03c0e1a R15: 000000000000000e
  kasprintf+0x4e/0x70
  ? timecounter_init+0x20/0x50
  xgbe_debugfs_init+0x39/0x200 [amd_xgbe]
  xgbe_config_netdev+0x390/0x450 [amd_xgbe]
  xgbe_pci_probe+0x374/0x620 [amd_xgbe]
  local_pci_probe+0x26/0x50
  pci_device_probe+0x107/0x1a0
  really_probe+0x147/0x3b0
  ? driver_allows_async_probing+0x50/0x50
  bus_for_each_drv+0x7e/0xc0
  __device_attach+0xd6/0x130
  bus_rescan_devices_helper+0x35/0x80
  drivers_probe_store+0x31/0x60
  kernfs_fop_write+0xce/0x1b0
  vfs_write+0xb6/0x1a0
  ksys_write+0x5f/0xe0
  do_syscall_64+0x55/0x1c0
  entry_SYSCALL_64_after_hwframe+0x44/0xa9
RIP: 0033:0x7fa72e73bd7f
Code: 89 54 24 18 48 89 74 24 10 89 7c 24 08 e8 b9 7b f9 ff 48 8b 54 24 18 48 8b 74 24 10 41 89 c0 8b 7c 24 08 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 2d 44 89 c7 48 89 44 24 08 e8 ec 7b f9 ff 48
RSP: 002b:00007fa6de7fba10 EFLAGS: 00000293 ORIG_RAX: 0000000000000001
RAX: ffffffffffffffda RBX: 000000000000000c RCX: 00007fa72e73bd7f
RDX: 000000000000000c RSI: 00007fa72803cf60 RDI: 000000000000001c
RBP: 00007fa72803cf60 R08: 0000000000000000 R09: 0000000000000003
R10: 0000000000000000 R11: 0000000000000293 R12: 000000000000001c
R13: 000000000000001c R14: 0000000000000000 R15: 00007fa72ef0a9e8
Kernel Offset: 0x26200000 from 0xffffffff81000000 (relocation range: 0xffffffff80000000-0xffffffffbfffffff)

Replace the use of kasprintf() with snprintf() using a local buffer to
prevent this situation. It is still possible for the device name to be
changed while the debugfs entry is being created, but that will be
handled by xgbe_debugfs_rename() function.

Fixes: c5aa9e3b8156 ("amd-xgbe: Initial AMD 10GbE platform driver")
Reported-by: Anthony Pighin <anthony.pighin@nokia.com>
Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---

Please queue to stable:
- As the warning is only produced at v4.5 and above, no need to go back
  further than that.
- This patch will generate conflicts prior to the v5.4 stable tree that
  should be easy to resolve. But, if not, I'll take care of it when I
  see the emails.
---
 drivers/net/ethernet/amd/xgbe/xgbe-debugfs.c | 25 ++++++++++----------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-debugfs.c b/drivers/net/ethernet/amd/xgbe/xgbe-debugfs.c
index b0a6c96b6ef4..a6537f24dd79 100644
--- a/drivers/net/ethernet/amd/xgbe/xgbe-debugfs.c
+++ b/drivers/net/ethernet/amd/xgbe/xgbe-debugfs.c
@@ -121,6 +121,8 @@
 #include "xgbe.h"
 #include "xgbe-common.h"
 
+#define XGBE_DIR_PREFIX	"amd-xgbe-"
+
 static ssize_t xgbe_common_read(char __user *buffer, size_t count,
 				loff_t *ppos, unsigned int value)
 {
@@ -438,15 +440,17 @@ static const struct file_operations xi2c_reg_value_fops = {
 
 void xgbe_debugfs_init(struct xgbe_prv_data *pdata)
 {
-	char *buf;
+	char buf[sizeof(XGBE_DIR_PREFIX) + sizeof(pdata->netdev->name)];
+	int ret;
 
 	/* Set defaults */
 	pdata->debugfs_xgmac_reg = 0;
 	pdata->debugfs_xpcs_mmd = 1;
 	pdata->debugfs_xpcs_reg = 0;
 
-	buf = kasprintf(GFP_KERNEL, "amd-xgbe-%s", pdata->netdev->name);
-	if (!buf)
+	ret = snprintf(buf, sizeof(buf), "%s%s", XGBE_DIR_PREFIX,
+		       pdata->netdev->name);
+	if (ret >= sizeof(buf))
 		return;
 
 	pdata->xgbe_debugfs = debugfs_create_dir(buf, NULL);
@@ -493,8 +497,6 @@ void xgbe_debugfs_init(struct xgbe_prv_data *pdata)
 				    pdata->xgbe_debugfs,
 				    &pdata->debugfs_an_cdr_track_early);
 	}
-
-	kfree(buf);
 }
 
 void xgbe_debugfs_exit(struct xgbe_prv_data *pdata)
@@ -505,21 +507,20 @@ void xgbe_debugfs_exit(struct xgbe_prv_data *pdata)
 
 void xgbe_debugfs_rename(struct xgbe_prv_data *pdata)
 {
-	char *buf;
+	char buf[sizeof(XGBE_DIR_PREFIX) + sizeof(pdata->netdev->name)];
+	int ret;
 
 	if (!pdata->xgbe_debugfs)
 		return;
 
-	buf = kasprintf(GFP_KERNEL, "amd-xgbe-%s", pdata->netdev->name);
-	if (!buf)
+	ret = snprintf(buf, sizeof(buf), "%s%s", XGBE_DIR_PREFIX,
+		       pdata->netdev->name);
+	if (ret >= sizeof(buf))
 		return;
 
 	if (!strcmp(pdata->xgbe_debugfs->d_name.name, buf))
-		goto out;
+		return;
 
 	debugfs_rename(pdata->xgbe_debugfs->d_parent, pdata->xgbe_debugfs,
 		       pdata->xgbe_debugfs->d_parent, buf);
-
-out:
-	kfree(buf);
 }
-- 
2.34.1

RE: [PATCH net] net: amd-xgbe: Replace kasprintf() with snprintf() for debugfs name

[David Laight]

From: Tom Lendacky
> Sent: 18 February 2022 20:05
> 
> It was reported that using kasprintf() produced a kernel warning as the
> network interface name was being changed by udev rules at the same time
> that the debugfs entry for the device was being created.

What was the error?
I'm guessing the length changed and that made kvasprintf() unhappy??

...
> -	buf = kasprintf(GFP_KERNEL, "amd-xgbe-%s", pdata->netdev->name);
> -	if (!buf)
> +	ret = snprintf(buf, sizeof(buf), "%s%s", XGBE_DIR_PREFIX,
> +		       pdata->netdev->name);

You can do:
	snprintf(buf, sizeof buf, XGBE_DIR_PREFIX "%s", pdata->netdev->name)

> +	if (ret >= sizeof(buf))
>  		return;

Unlike kasnprintf() where kmalloc() can fail, the simple snprintf()
can't really overrun unless pdata->netdev->name isn't '\0' terminated.
Even if it being changed while you look at it that shouldn't happen.


Don't you need to synchronise this anyway?

If the debugfs create and rename can happen at the same time then
the rename can be requested before the create and you get the wrong
name.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH net] net: amd-xgbe: Replace kasprintf() with snprintf() for debugfs name

[Tom Lendacky]

On 2/18/22 15:01, David Laight wrote:
> From: Tom Lendacky
>> Sent: 18 February 2022 20:05
>>
>> It was reported that using kasprintf() produced a kernel warning as the
>> network interface name was being changed by udev rules at the same time
>> that the debugfs entry for the device was being created.
> 
> What was the error?
> I'm guessing the length changed and that made kvasprintf() unhappy??
> 
> ...
>> -	buf = kasprintf(GFP_KERNEL, "amd-xgbe-%s", pdata->netdev->name);
>> -	if (!buf)
>> +	ret = snprintf(buf, sizeof(buf), "%s%s", XGBE_DIR_PREFIX,
>> +		       pdata->netdev->name);
> 
> You can do:
> 	snprintf(buf, sizeof buf, XGBE_DIR_PREFIX "%s", pdata->netdev->name)
> 

Thought about that, just decided on the double %s, though.

>> +	if (ret >= sizeof(buf))
>>   		return;
> 
> Unlike kasnprintf() where kmalloc() can fail, the simple snprintf()
> can't really overrun unless pdata->netdev->name isn't '\0' terminated.
> Even if it being changed while you look at it that shouldn't happen.

It's a safety check, it doesn't hurt anything.

> 
> 
> Don't you need to synchronise this anyway?
> 
> If the debugfs create and rename can happen at the same time then
> the rename can be requested before the create and you get the wrong
> name.

I thought about introducing a mutex or semaphore, but thought it was 
overkill just for debugfs support and this small window. But if folks 
think it's really needed, it can be added.

Thanks,
Tom

> 
> 	David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
>