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

[David Laight]

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