linux-kernel - Re: [PATCH 0/8] can: usb: remove all usb_set_intfdata(intf, NULL) in drivers' disconnect()

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:   Thu, 8 Dec 2022 11:55:06 +0100
From:   Oliver Neukum <oneukum@...e.com>
To:     Vincent MAILHOL <mailhol.vincent@...adoo.fr>,
        Oliver Neukum <oneukum@...e.com>
Cc:     Marc Kleine-Budde <mkl@...gutronix.de>, linux-can@...r.kernel.org,
        Wolfgang Grandegger <wg@...ndegger.com>,
        "David S . Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Frank Jungclaus <frank.jungclaus@....eu>, socketcan@....eu,
        Yasushi SHOJI <yashi@...cecubics.com>,
        Stefan Mätje <stefan.maetje@....eu>,
        Hangyu Hua <hbh25y@...il.com>,
        Oliver Hartkopp <socketcan@...tkopp.net>,
        Peter Fink <pfink@...ist-es.de>,
        Jeroen Hofstee <jhofstee@...tronenergy.com>,
        Christoph Möhring <cmoehring@...ist-es.de>,
        John Whittington <git@...engineering.co.uk>,
        Vasanth Sadhasivan <vasanth.sadhasivan@...sara.com>,
        Jimmy Assarsson <extja@...ser.com>,
        Anssi Hannula <anssi.hannula@...wise.fi>,
        Pavel Skripkin <paskripkin@...il.com>,
        Stephane Grosjean <s.grosjean@...k-system.com>,
        Wolfram Sang <wsa+renesas@...g-engineering.com>,
        "Gustavo A . R . Silva" <gustavoars@...nel.org>,
        Julia Lawall <Julia.Lawall@...ia.fr>,
        Dongliang Mu <dzm91@...t.edu.cn>,
        Sebastian Haas <haas@...-wuensche.com>,
        Maximilian Schneider <max@...neidersoft.net>,
        Daniel Berglund <db@...ser.com>,
        Olivier Sobrie <olivier@...rie.be>,
        Remigiusz Kołłątaj 
        <remigiusz.kollataj@...ica.com>,
        Jakob Unterwurzacher <jakob.unterwurzacher@...obroma-systems.com>,
        Martin Elshuber <martin.elshuber@...obroma-systems.com>,
        Philipp Tomsich <philipp.tomsich@...obroma-systems.com>,
        Bernd Krumboeck <b.krumboeck@...il.com>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        Alan Stern <stern@...land.harvard.edu>,
        linux-usb@...r.kernel.org
Subject: Re: [PATCH 0/8] can: usb: remove all usb_set_intfdata(intf, NULL) in
 drivers' disconnect()

On 08.12.22 10:00, Vincent MAILHOL wrote:
> On Mon. 5 Dec. 2022 at 17:39, Oliver Neukum <oneukum@...e.com> wrote:
>> On 03.12.22 14:31, Vincent Mailhol wrote:

Good Morning!

> ACK, but I do not see the connection.
Well, useless checks are bad. In particular, we should always
make it clear whether a pointer may or may not be NULL.
That is, I have no problem with what you were trying to do
with your patch set. It is a good idea and possibly slightly
overdue. The problem is the method.

> I can see that cdc-acm sets acm->control and acm->data to NULL in his
> disconnect(), but it doesn't set its own usb_interface to NULL.

You don't have to, but you can. I was explaining the two patterns for doing so.

>> which claim secondary interfaces disconnect() will be called a second time
>> for.
> 
> Are you saying that the disconnect() of those CAN USB drivers is being
> called twice? I do not see this in the source code. The only caller of
> usb_driver::disconnect() I can see is:
> 
>    https://elixir.bootlin.com/linux/v6.0/source/drivers/usb/core/driver.c#L458

If they use usb_claim_interface(), yes it is called twice. Once per
interface. That is in the case of ACM once for the originally probed
interface and a second time for the claimed interface.
But not necessarily in that order, as you can be kicked off an interface
via sysfs. Yet you need to cease operations as soon as you are disconnected
from any interface. That is annoying because it means you cannot use a
refcount. From that stems the widespread use of intfdata as a flag.

>> In addition, a driver can use setting intfdata to NULL as a flag
>> for disconnect() having proceeded to a point where certain things
>> can no longer be safely done.
> 
> Any reference that a driver can do that? This pattern seems racy.

Technically that is exactly what drivers that use usb_claim_interface()
do. You free everything at the first call and use intfdata as a flag
to prevent a double free.
The race is prevented by usbcore locking, which guarantees that probe()
and disconnect() have mutual exclusion.
If you use intfdata in sysfs, yes additional locking is needed.

> What makes you assume that I didn't check this in the first place? Or
> do you see something I missed?

That you did not put it into the changelogs.
That reads like the drivers are doing something obsolete or stupid.
They do not. They copied something that is necessary only under
some circumstances.

And that you did not remove the checks.

>> which is likely, then please also remove checks like this:
>>
>>          struct ems_usb *dev = usb_get_intfdata(intf);
>>
>>          usb_set_intfdata(intf, NULL);
>>
>>          if (dev) {

Here. If you have a driver that uses usb_claim_interface().
You need this check or you unregister an already unregistered
netdev.

The way this disconnect() method is coded is extremely defensive.
Most drivers do not need this check. But it is never
wrong in the strict sense.

Hence doing a mass removal with a change log that does
not say that this driver is using only a single interface
hence the check can be dropped to reduce code size
is not good.

	Regards
		Oliver