Message-Id: <20221210090157.793547-1-mailhol.vincent@wanadoo.fr>
Date:   Sat, 10 Dec 2022 18:01:48 +0900
From:   Vincent Mailhol <mailhol.vincent@...adoo.fr>
To:     Marc Kleine-Budde <mkl@...gutronix.de>, linux-can@...r.kernel.org
Cc:     Oliver Neukum <oneukum@...e.com>,
        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>,
        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,
        Vincent Mailhol <mailhol.vincent@...adoo.fr>
Subject: [PATCH v2 0/9] can: usb: remove all usb_set_intfdata(intf, NULL) in drivers' disconnect()

Many of the can usb drivers set their driver's priv data to NULL in
their disconnect function using below pattern:

	struct driver_priv *priv = usb_get_intfdata(intf);

	usb_set_intfdata(intf, NULL);

	if (priv)
                /* ... */

The pattern comes from other drivers which have a secondary interface
and for which disconnect() may be called twice. However, usb can
drivers do not have such secondary interface.

On the contrary, if a driver set the driver's priv data to NULL before
all actions relying on the interface-data pointer complete, there is a
risk of NULL pointer dereference. Typically, this is the case if there
are outstanding urbs which have not yet completed when entering
disconnect().

Finally, even if there is a valid reason to set the driver's priv
data, it would still be useless to do it in usb_driver::disconnect()
because the core sets the driver's data to NULL in [1] (and this
operation is done in within locked context, so that race conditions
should not occur).

The first seven patches fix all drivers which set their usb_interface
to NULL while outstanding URB might still exists. There is one patch
per driver in order to add the relevant "Fixes:" tag to each of them.

The eighth patch removes the check toward the driver data being
NULL. This just reduces the kernel size so no fixes tag here and all
changes are done in bulk in one single patch.

Finally, the last patch removes in bulk the remaining benign calls to
usb_set_intfdata(intf, NULL) in etas_es58x and peak_usb.

N.B. some other usb drivers outside of the can tree also have the same
issue, but this is out of scope of this series.

[1] function usb_unbind_interface() from drivers/usb/core/driver.c
Link: https://elixir.bootlin.com/linux/v6.0/source/drivers/usb/core/driver.c#L497
---
* Changelog *

v1 -> v2

  * add explanation in the cover letter on the origin of this pattern
    and why it does not apply to can usb drivers.

  * v1 claimed that usb_set_intfdata(intf, NULL) sets the
    usb_interface to NULL. This is incorrect, it is the pointer to
    driver's private data which set to NULL. Fix this point of
    confusion in commit message.

  * add a patch to clean up the useless check on the driver's private
    data being NULL.

Vincent Mailhol (9):
  can: ems_usb: ems_usb_disconnect(): fix NULL pointer dereference
  can: esd_usb: esd_usb_disconnect(): fix NULL pointer dereference
  can: gs_usb: gs_usb_disconnect(): fix NULL pointer dereference
  can: kvaser_usb: kvaser_usb_disconnect(): fix NULL pointer dereference
  can: mcba_usb: mcba_usb_disconnect(): fix NULL pointer dereference
  can: ucan: ucan_disconnect(): fix NULL pointer dereference
  can: usb_8dev: usb_8dev_disconnect(): fix NULL pointer dereference
  can: usb: remove useless check on driver data
  can: etas_es58x and peak_usb: remove useless call to
    usb_set_intfdata()

 drivers/net/can/usb/ems_usb.c                  | 16 ++++++----------
 drivers/net/can/usb/esd_usb.c                  | 18 +++++++-----------
 drivers/net/can/usb/etas_es58x/es58x_core.c    |  1 -
 drivers/net/can/usb/gs_usb.c                   |  7 -------
 .../net/can/usb/kvaser_usb/kvaser_usb_core.c   |  9 +--------
 drivers/net/can/usb/mcba_usb.c                 |  2 --
 drivers/net/can/usb/peak_usb/pcan_usb_core.c   |  2 --
 drivers/net/can/usb/ucan.c                     |  8 ++------
 drivers/net/can/usb/usb_8dev.c                 | 13 ++++---------
 9 files changed, 20 insertions(+), 56 deletions(-)

-- 
2.37.4

Powered by blists - more mailing lists