netdev - 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 linux-cve-announce 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]

Message-ID: <CAFo99gb9Rqnc7HrPbN7UVyQwPv_a=fxAbe+FTtm+9GgsjG7StA@mail.gmail.com>
Date:	Mon, 15 Sep 2014 23:47:33 +0200
From:	Rickard Strandqvist <rickard_strandqvist@...ctrumdigital.se>
To:	David Laight <David.Laight@...lab.com>
Cc:	Marc Kleine-Budde <mkl@...gutronix.de>,
	Wolfgang Grandegger <wg@...ndegger.com>,
	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

2014-09-15 10:47 GMT+02:00 David Laight <David.Laight@...lab.com>:
> 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 netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html