linux-kernel - RE: [PATCH] net: can: usb: peak_usb: pcan_usb_core.c: Cleaning up missing null-terminate in conjunction with strncpy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 15 Sep 2014 08:47:18 +0000
From:	David Laight <David.Laight@...LAB.COM>
To:	'Marc Kleine-Budde' <mkl@...gutronix.de>,
	'Rickard Strandqvist' <rickard_strandqvist@...ctrumdigital.se>,
	Wolfgang Grandegger <wg@...ndegger.com>
CC:	Oliver Hartkopp <socketcan@...tkopp.net>,
	Stephane Grosjean <s.grosjean@...k-system.com>,
	Alexey Khoroshilov <khoroshilov@...ras.ru>,
	"Christopher R. Baker" <cbaker@....ri.cmu.edu>,
	"linux-can@...r.kernel.org" <linux-can@...r.kernel.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] net: can: usb: peak_usb: pcan_usb_core.c:  Cleaning up
 missing null-terminate in conjunction with strncpy

From: Marc Kleine-Budde [ 
> On 09/15/2014 10:28 AM, David Laight wrote:
> > From: Rickard Strandqvist
> > ...
> >> Replacing strncpy with strlcpy to avoid strings that lacks null terminate.
> > ...
> >> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.c
> >> b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
> >> index 644e6ab..d4fe8ac 100644
> >> --- a/drivers/net/can/usb/peak_usb/pcan_usb_core.c
> >> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
> >> @@ -830,7 +830,7 @@ static void peak_usb_disconnect(struct usb_interface *intf)
> >>  		char name[IFNAMSIZ];
> >>
> >>  		dev->state &= ~PCAN_USB_STATE_CONNECTED;
> >> -		strncpy(name, netdev->name, IFNAMSIZ);
> >> +		strlcpy(name, netdev->name, IFNAMSIZ);
> >>
> >>  		unregister_netdev(netdev);
> >>  		free_candev(netdev);
> >
> > Or:
> > 	char name[sizeof netdev->name];
> > 	memcpy(name, netdev->name, sizeof netdev->name);
> 
> I would be "sizeof(foo)" in kernel coding style,
But not in mine :-)
sizeof is an operator, not a function, it's argument can be (type).

> but let's have a look at the original code:
> 
>                 struct net_device *netdev = dev->netdev;
>                 char name[IFNAMSIZ];
> 
>                 dev->state &= ~PCAN_USB_STATE_CONNECTED;
>                 strncpy(name, netdev->name, IFNAMSIZ);
> 
>                 unregister_netdev(netdev);
>                 free_candev(netdev);
> 
>                 kfree(dev->cmd_buf);
>                 dev->next_siblings = NULL;
>                 if (dev->adapter->dev_free)
>                         dev->adapter->dev_free(dev);
> 
>                 dev_info(&intf->dev, "%s removed\n", name);
> 
> I think it's save to use:
> 
>     dev_info(&intf->dev, "%s removed\n", netdev_name(dev->netdev));
> 
> instead of doing the str?cpy() in the first place. But why not use:
> 
>     netdev_info(dev->netdev, "removed\n");
> 
> Is the USB device information lost when using netdev_info()?

My guess is it avoids a 'use after free' - but I'm not going to
dig that far.

Another issue with blindly replacing strncpy() with strlcpy()
(which doesn't affect the above) is when copying status to userspace.

	David