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

[Rickard Strandqvist]

2014-09-15 10:47 GMT+02:00 David Laight <David.Laight@xxxxxxxxxx>:
> 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


Hi all

I have been more and more careful so I did not introduce the type of
error that can arise by blindly replacing strncpy to strlcpy.
But this is one of the most apparent where there will not be a problem.

I liked the variant:
char name[sizeof(netdev->name)];

But dislike and do not understand what the point would be with memcpy variant.


Kind regards
Rickard Strandqvist
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/