Messages in this thread | | | Date | Fri, 13 Oct 2023 08:04:45 +0200 | Subject | RE: [PATCH v4] net: ravb: Fix possible UAF bug in ravb_remove | From | "Behme Dirk (CM/ESO2)" <> |
| |
Hi,
On 12.10.2023 10:39, Yoshihiro Shimoda wrote: > Hello Behme, > >> From: Behme Dirk (CM/ESO2), Sent: Tuesday, October 10, 2023 9:59 PM >> >> On 26.07.2023 05:19, Jakub Kicinski wrote: >> ... >>> The fact that ravb_tx_timeout_work doesn't take any locks seems much >>> more suspicious. >> Does anybody plan to look into this, too? > > I believe my fixed patch [1] resolved this issue too.
I'm not an expert of this driver nor the network stack, so sorry if I'm totally wrong here ;) But somehow this answer confuses me. Let me explain:
What you did with [1] is to stop/cancel the workqueue in ravb_close(). That's fine. But that is at driver's close time.
What's about driver's normal runtime? What I understood is that ravb_tx_timeout_work() might run totally asynchronously to the rest of the driver. And therefore needs locking/protection/synchronization if it uses resources of the driver which are used elsewhere in the driver, too.
I think this is exactly what is described with:
> --- > Simplest fix I can think of is to take a reference on the netdev before > scheduling the work, and then check if it's still registered in the work > itself. Wrap the timeout work in rtnl_lock() to avoid any races there. > ---
So, where is above done? Not at close time, but at normal run time of the driver?
Best regards
Dirk
> Sergey suggested to add cancel_work_sync() into the ravb_close () [3]. > And I investigated calltrace, and then the ravb_close() is under rtnl_lock() [4] > like below: > ----------------------------------------------------------------------- > ravb_remove() calls unregister_netdev(). > -> unregister_netdev() calls rtnl_lock() and unregister_netdevice(). > --> unregiter_netdevice_queue() > ---> unregiter_netdevice_many() > ----> unregiter_netdevice_many_notify(). > -----> dev_close_many() > ------> __dev_close_many() > -------> ops->ndo_stop() > > ravb_close() calls phy_stop() > -> phy_state_machine() with PHY_HALTED > --> phy_link_down() > ---> phy_link_change() > ----> netif_carrier_off() > ----------------------------------------------------------------------- > > So, during cancel_work_sync() is waiting for canceling the workqueue in ravb_close(), > it's under rtnl_lock() so that no additional locks are needed in ravb_tx_timeout_work(). > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=3971442870713de527684398416970cf025b4f89 > [2] https://lore.kernel.org/netdev/20230727164820.48c9e685@kernel.org/ > [3] https://lore.kernel.org/netdev/607f4fe4-5a59-39dd-71c2-0cf769b48187@omp.ru/ > [4] https://lore.kernel.org/netdev/OSYPR01MB53341CFDBB49A3BA41A6752CD8F9A@OSYPR01MB5334.jpnprd01.prod.outlook.com/ > > Best regards, > Yoshihiro Shimoda > >> Best regards >> >> Dirk
-- ====================================================================== Dirk Behme Robert Bosch Car Multimedia GmbH CM/ESO2 Phone: +49 5121 49-3274 Dirk Behme Fax: +49 711 811 5053274 PO Box 77 77 77 mailto:dirk.behme@de.bosch.com D-31132 Hildesheim - Germany
Bosch Group, Car Multimedia (CM) Engineering SW Operating Systems 2 (ESO2)
Robert Bosch Car Multimedia GmbH - Ein Unternehmen der Bosch Gruppe Sitz: Hildesheim Registergericht: Amtsgericht Hildesheim HRB 201334 Aufsichtsratsvorsitzender: Dr. Dirk Hoheisel Geschäftsführung: Dr. Steffen Berns; Dr. Sven Ost, Jörg Pollak, Dr. Walter Schirm ======================================================================
| |