| 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
| ||
|
Date: Thu, 8 Dec 2022 18:00:46 +0900
From: Vincent MAILHOL <mailhol.vincent@...adoo.fr>
To: 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 Mon. 5 Dec. 2022 at 17:39, Oliver Neukum <oneukum@...e.com> wrote:
> On 03.12.22 14:31, Vincent Mailhol wrote:
> > The core sets the usb_interface to NULL in [1]. Also setting it to
> > NULL in usb_driver::disconnects() is at best useless, at worse risky.
>
> Hi,
>
> I am afraid there is a major issue with your series of patches.
> The drivers you are removing this from often have a subsequent check
> for the data they got from usb_get_intfdata() being NULL.
ACK, but I do not see the connection.
> That pattern is taken from drivers like btusb or CDC-ACM
Where does CDC-ACM set *his* interface to NULL? Looking at:
https://elixir.bootlin.com/linux/v6.0/source/drivers/usb/class/cdc-acm.c#L1531
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.
> 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
> 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.
By the way, I did check all the drivers:
* ems_usb: intf is only used in ems_usb_probe() and
ems_usb_disconnect() functions.
* esd_usb: intf is only used in the esd_usb_probe(),
esd_usb_probe_one_net() (which is part of probing),
esd_usb_disconnect() and a couple of sysfs functions (which only
use intf to get a pointer to struct esd_usb).
* gs_usb: intf is used several time but only to retrive struct
usb_device. This seems useless, I will sent this patch to remove
it:
https://lore.kernel.org/linux-can/20221208081142.16936-3-mailhol.vincent@wanadoo.fr/
Aside of that, intf is only used in gs_usb_probe(),
gs_make_candev() (which is part of probing) and
gs_usb_disconnect() functions.
* kvaser_usb: intf is only used in kvaser_usb_probe() and
kvaser_usb_disconnect() functions.
* mcba_usb: intf is only used in mcba_usb_probe() and
mcba_usb_disconnect() functions.
* ucan: intf is only used in ucan_probe() and
ucan_disconnect(). struct ucan_priv also has a pointer to intf but
it is never used. I sent this patch to remove it:
https://lore.kernel.org/linux-can/20221208081142.16936-2-mailhol.vincent@wanadoo.fr/
* usb_8dev: intf is only used in usb_8dev_probe() and
usb_8dev_disconnect().
With no significant use of intf outside of the probe() and
disconnect(), there is definitely no such "use intf as a flag" in any
of these drivers.
> You need to check for that in every driver
> you remove this code from and if you decide that it can safely be removed,
What makes you assume that I didn't check this in the first place? Or
do you see something I missed?
> 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) {
> unregister_netdev(dev->netdev);
How is the if (dev) check related? There is no correlation between
setting intf to NULL and dev not being NULL.
I think dev is never NULL, but I did not assess that dev could not be NULL.
> Either it can be called a second time, then you need to leave it
> as is,
Really?! The first thing disconnect() does is calling
usb_get_intfdata(intf) which dereferences intf without checking if it
is NULL, c.f.:
https://elixir.bootlin.com/linux/v6.0/source/include/linux/usb.h#L265
Then it sets intf to NULL.
The second time you call disconnect(), the usb_get_intfdata(intf)
would be a NULL pointer dereference.
> or the check for NULL is superfluous. But only removing setting
> the pointer to NULL never makes sense.
Yours sincerely,
Vincent Mailhol
Powered by blists - more mailing lists