netdev - Re: [PATCH] driver: usb: nullify dangling pointer in cdc_ncm

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]

Date:   Wed, 20 Apr 2022 11:45:49 +0200
From:   Oliver Neukum <oneukum@...e.com>
To:     Johan Hovold <johan@...nel.org>, Oliver Neukum <oneukum@...e.com>
CC:     Andy Shevchenko <andy.shevchenko@...il.com>,
        Oleksij Rempel <linux@...pel-privat.de>,
        Dongliang Mu <dzm91@...t.edu.cn>,
        Oliver Neukum <oliver@...kum.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Dongliang Mu <mudongliangabcd@...il.com>,
        syzbot+eabbf2aaa999cc507108@...kaller.appspotmail.com,
        USB <linux-usb@...r.kernel.org>, netdev <netdev@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] driver: usb: nullify dangling pointer in cdc_ncm_free



On 20.04.22 08:56, Johan Hovold wrote:
>
>>  
>> @@ -1214,7 +1214,7 @@ static const struct driver_info hawking_uf200_info = {
>>  static const struct driver_info ax88772_info = {
>>  	.description = "ASIX AX88772 USB 2.0 Ethernet",
>>  	.bind = ax88772_bind,
>> -	.unbind = ax88772_unbind,
>> +	.unbind = ax88772_disable,
> These should all be
>
> 	.disable = ...
Thanks, noted.
>
> but you probably need to split the callback and keep unbind as well for
> the actual clean up (freeing resources etc).
That is the driver the problematic commit was requested for.
I am looking into it.
>
>> -	if (dev->driver_info->unbind)
>> -		dev->driver_info->unbind(dev, intf);
>> +	if (dev->driver_info->disable)
>> +		dev->driver_info->disable(dev, intf);
>>  
>>  	net = dev->net;
>>  	unregister_netdev (net);
>> @@ -1651,6 +1651,9 @@ void usbnet_disconnect (struct usb_interface *intf)
>>  
>>  	usb_scuttle_anchored_urbs(&dev->deferred);
>>  
>> +	if (dev->driver_info->unbind)
>> +		dev->driver_info->unbind (dev, intf);
>> +
>>  	usb_kill_urb(dev->interrupt);
> Don't you need to quiesce all I/O, including stopping the interrupt URB,
> before unbind?
If I do that, how do I prevent people from relaunching the URB between
kill and unbind? Do I need to poison it?

    Regards
        Oliver