| Message ID | 20221210090157.793547-2-mailhol.vincent@wanadoo.fr (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | can: usb: remove all usb_set_intfdata(intf, NULL) in drivers' disconnect() | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Series ignored based on subject |
On Sat, Dec 10, 2022 at 06:01:49PM +0900, Vincent Mailhol wrote: > ems_usb sets the driver's priv data to NULL before waiting for the > completion of outsdanding urbs. This can results in NULL pointer > dereference, c.f. [1] and [2]. Please stop making hand-wavy claims like this. There is no risk for a NULL-pointer deference here, and if you think otherwise you need to explain how that can happen in detail for each driver. > Remove the call to usb_set_intfdata(intf, NULL). The core will take > care of setting it to NULL after ems_usb_disconnect() at [3]. > > [1] c/27ef17849779 ("usb: add usb_set_intfdata() documentation") > Link: https://git.kernel.org/gregkh/usb/c/27ef17849779 The claim in this commit is not correct either. > [2] thread about usb_set_intfdata() on linux-usb mailing. > Link: https://lore.kernel.org/linux-usb/Y4OD70GD4KnoRk0k@rowland.harvard.edu/ > > [3] 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 > > Fixes: 702171adeed3 ("ems_usb: Added support for EMS CPC-USB/ARM7 CAN/USB interface") > Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr> > --- > drivers/net/can/usb/ems_usb.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c > index 050c0b49938a..c64cb40ac8de 100644 > --- a/drivers/net/can/usb/ems_usb.c > +++ b/drivers/net/can/usb/ems_usb.c > @@ -1062,8 +1062,6 @@ static void ems_usb_disconnect(struct usb_interface *intf) > { > struct ems_usb *dev = usb_get_intfdata(intf); The interface data pointer is only used in this function so there is no risk for any NULL pointer dereference here. I only checked one of the other drivers you patch, but I'm pretty sure all of your claims about fixing NULL-pointer dereferences in this series are equally bogus. > > - usb_set_intfdata(intf, NULL); > - > if (dev) { > unregister_netdev(dev->netdev); Johan
On Tue. 10 déc. 2022 à 20:02, Johan Hovold <johan@kernel.org> wrote: > On Sat, Dec 10, 2022 at 06:01:49PM +0900, Vincent Mailhol wrote: > > ems_usb sets the driver's priv data to NULL before waiting for the > > completion of outsdanding urbs. This can results in NULL pointer > > dereference, c.f. [1] and [2]. > > Please stop making hand-wavy claims like this. There is no risk for a > NULL-pointer deference here, and if you think otherwise you need to > explain how that can happen in detail for each driver. Understood. *My* mistake comes from this message from Alan [1]: | But if a driver does make the call, it should be careful to | ensure that the call happens _after_ the driver is finished | using the interface-data pointer. For example, after all | outstanding URBs have completed, if the completion handlers | will need to call usb_get_intfdata(). I did not pay enough attention to the "if the completion handlers will need to call usb_get_intfdata()" part and jumped into the incorrect conclusion that any use of usb_set_intfdata(intf, NULL) before URB completion was erroneous. My deep apologies for all the noise. Please forget this series and one more time, thank you for your patience. [1] https://lore.kernel.org/linux-usb/Y4OD70GD4KnoRk0k@rowland.harvard.edu/
diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c index 050c0b49938a..c64cb40ac8de 100644 --- a/drivers/net/can/usb/ems_usb.c +++ b/drivers/net/can/usb/ems_usb.c @@ -1062,8 +1062,6 @@ static void ems_usb_disconnect(struct usb_interface *intf) { struct ems_usb *dev = usb_get_intfdata(intf); - usb_set_intfdata(intf, NULL); - if (dev) { unregister_netdev(dev->netdev);
ems_usb sets the driver's priv data to NULL before waiting for the completion of outsdanding urbs. This can results in NULL pointer dereference, c.f. [1] and [2]. Remove the call to usb_set_intfdata(intf, NULL). The core will take care of setting it to NULL after ems_usb_disconnect() at [3]. [1] c/27ef17849779 ("usb: add usb_set_intfdata() documentation") Link: https://git.kernel.org/gregkh/usb/c/27ef17849779 [2] thread about usb_set_intfdata() on linux-usb mailing. Link: https://lore.kernel.org/linux-usb/Y4OD70GD4KnoRk0k@rowland.harvard.edu/ [3] 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 Fixes: 702171adeed3 ("ems_usb: Added support for EMS CPC-USB/ARM7 CAN/USB interface") Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr> --- drivers/net/can/usb/ems_usb.c | 2 -- 1 file changed, 2 deletions(-)