staging/rtl8712: Remove all strcpy() uses in favor of strscpy()
PATCH 1/1 staging/rtl8712: Remove all strcpy() uses in favor of strscpy()
strcpy() performs no bounds checking on the destination buffer. This could result in linear overflows beyond the end of the buffer, leading to all kinds of misbehaviors. The safe replacement is strscpy().
--- drivers/staging/rtl8712/os_intfs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/staging/rtl8712/os_intfs.c +1 -1
@@ -203,7 +203,7 @@ struct net_device *r8712_init_netdev(void)if (!pnetdev)return NULL ;if (dev_alloc_name (pnetdev, ifname) <0 ) { -strcpy (ifname,"wlan%d" ); +strscpy (ifname,"wlan%d" ,sizeof (ifname));dev_alloc_name (pnetdev, ifname); } padapter =netdev_priv (pnetdev);
--
2.25.1
Quoted 14 lines · Len Baker
strcpy()performs no bounds checking on the destination buffer. This could result in linear overflows beyond the end of the buffer, leading to all kinds of misbehaviors. The safe replacement isstrscpy().Signed-off-by Len Baker <[email protected]>--- drivers/staging/rtl8712/os_intfs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/staging/rtl8712/os_intfs.c +0
-0
drivers/staging/rtl8712/os_intfs.c struct net_device *r8712_init_netdev(void) · line 207
Quoted 6 lines
drivers/staging/rtl8712/os_intfs.c +1
-1@@ -203,7 +203,7 @@ struct net_device *r8712_init_netdev(void)if (!pnetdev)return NULL ;if (dev_alloc_name (pnetdev, ifname) <0 ) { -strcpy (ifname,"wlan%d" ); +strscpy (ifname,"wlan%d" ,sizeof (ifname));dev_alloc_name (pnetdev, ifname);
Not related to your patch but this code is bad. What it does is the "ifname" can be set as a module parameter. So instead of testing if it has been set, it uses the checking inside dev_alloc_name() to see if we can allocate what the user requested. If not then set it to "wlan%d". If we cannot allocate what the user wants then we should return an error.
It should do:
if (ifname[0] == '\0')
strscpy(ifname, "wlan%d", sizeof(ifname));
ret = dev_alloc_name(pnetdev, ifname);
if (ret < 0) {
dev_err(pnetdev, "allocating device name failed.\n");
return NULL;
}regards,
dan carpenter
From: Dan Carpenter
Quoted 1 line
Sent: 19 July 2021 06:38
...
Quoted 9 lines
Not related to your patch but this code is bad. What it does is the "ifname" can be set as a module parameter. So instead of testing if it has been set, it uses the checking inside
dev_alloc_name()to see if we can allocate what the user requested. If not then set it to "wlan%d". If we cannot allocate what the user wants then we should return an error.It should do:
Quoted 8 lines
if (ifname[0] == '\0')
strscpy(ifname, "wlan%d", sizeof(ifname));
ret = dev_alloc_name(pnetdev, ifname);
if (ret < 0) {
dev_err(pnetdev, "allocating device name failed.\n");
return NULL;
}I know only root can set module parameters, but having one that contains a string used as a printf format seems dangerous at best.
Isn't it best to let userspace rename the interfaces later on?
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Quoted 21 lines · David Laight
From: Dan Carpenter
Quoted 1 line
Sent: 19 July 2021 06:38
...
Quoted 9 lines
Not related to your patch but this code is bad. What it does is the "ifname" can be set as a module parameter. So instead of testing if it has been set, it uses the checking inside
dev_alloc_name()to see if we can allocate what the user requested. If not then set it to "wlan%d". If we cannot allocate what the user wants then we should return an error.It should do:
Quoted 8 lines
if (ifname[0] == '\0') strscpy(ifname, "wlan%d", sizeof(ifname)); ret = dev_alloc_name(pnetdev, ifname); if (ret < 0) { dev_err(pnetdev, "allocating device name failed.\n"); return NULL; }
Quoted 5 lines
I know only root can set module parameters, but having one that contains a string used as a printf format seems dangerous at best.
Isn't it best to let userspace rename the interfaces later on?
Yeah. I think you're right.
regards,
dan carpenter
Hi,
Quoted 22 lines · Dan Carpenter
Quoted 21 lines · David Laight
From: Dan Carpenter
Quoted 1 line
Sent: 19 July 2021 06:38
...
Quoted 17 lines
Not related to your patch but this code is bad. What it does is the "ifname" can be set as a module parameter. So instead of testing if it has been set, it uses the checking inside
dev_alloc_name()to see if we can allocate what the user requested. If not then set it to "wlan%d". If we cannot allocate what the user wants then we should return an error.It should do:
if (ifname[0] == '\0') strscpy(ifname, "wlan%d", sizeof(ifname)); ret = dev_alloc_name(pnetdev, ifname); if (ret < 0) { dev_err(pnetdev, "allocating device name failed.\n"); return NULL; }
Quoted 7 lines
Quoted 5 lines
I know only root can set module parameters, but having one that contains a string used as a printf format seems dangerous at best.
Isn't it best to let userspace rename the interfaces later on?
Yeah. I think you're right.
Sorry, but I don't understand if the code needs to be improved or not. Is the code shown by Dan not correct?
Quoted 4 lines
regards,
dan carpenter
Thanks for your time,
Len
Quoted 29 lines · Len Baker
Hi,
Quoted 26 lines · Dan Carpenter
Quoted 25 lines · David Laight
From: Dan Carpenter
Quoted 1 line
Sent: 19 July 2021 06:38
...
Quoted 9 lines
Not related to your patch but this code is bad. What it does is the "ifname" can be set as a module parameter. So instead of testing if it has been set, it uses the checking inside
dev_alloc_name()to see if we can allocate what the user requested. If not then set it to "wlan%d". If we cannot allocate what the user wants then we should return an error.It should do:
Quoted 8 lines
if (ifname[0] == '\0') strscpy(ifname, "wlan%d", sizeof(ifname)); ret = dev_alloc_name(pnetdev, ifname); if (ret < 0) { dev_err(pnetdev, "allocating device name failed.\n"); return NULL; }I know only root can set module parameters, but having one that contains a string used as a printf format seems dangerous at best.
Quoted 6 lines
Quoted 3 lines
Quoted 1 line
Isn't it best to let userspace rename the interfaces later on?
Yeah. I think you're right.
Sorry, but I don't understand if the code needs to be improved or not. Is the code shown by Dan not correct?
We should remove the ifname[] array and the module option completely.
regards,
dan carpenter
Quoted 10 lines · Dan Carpenter
Quoted 9 lines · Len Baker
Hi,
Quoted 6 lines · Dan Carpenter
Quoted 5 lines · David Laight
I know only root can set module parameters, but having one that contains a string used as a printf format seems dangerous at best.
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
Quoted 1 line
Isn't it best to let userspace rename the interfaces later on?
Yeah. I think you're right.
Sorry, but I don't understand if the code needs to be improved or not. Is the code shown by Dan not correct?
We should remove the ifname[] array and the module option completely.
Thanks for the clarification.
Regards,
Len