lkml.org 
[lkml]   [2023]   [Oct]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRE: [PATCH v4] net: ravb: Fix possible UAF bug in ravb_remove
    Date
    Hi,

    > From: Behme Dirk (CM/ESO2), Sent: Friday, October 13, 2023 3:05 PM
    >
    > 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?

    Thank you very much for your detailed explanation. I understood it.
    ravb_tx_timeout_work() still has races between ethtool ops for instance.
    So, I'll make a patch for it by early next week. However, IIUC, using
    rtnl_lock() in ravb_tx_timeout_work() is possible to cause deadlock from
    cancel_work_sync() in ravb_close(). So, I'll use rtnl_trylock() instead.

    Best regards,
    Yoshihiro Shimoda

    > 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%25
    > 2Fnetdev%2Fnet.git%2Fcommit%2F%3Fid%3D3971442870713de527684398416970cf025b4f89&data=05%7C01%7Cyoshihiro.shimoda.uh%4
    > 0renesas.com%7C466e046b20b548b264f808dbcbb255f6%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C638327739033548199%7CUn
    > known%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=HkA8f5a
    > gawjXMvAGkaE6tELaSpjpbIn7M3mU5xbDTD0%3D&reserved=0
    > > [2]
    > https://lore.kernel.org/netdev/20230727164820.48c9e685
    > %40kernel.org%2F&data=05%7C01%7Cyoshihiro.shimoda.uh%40renesas.com%7C466e046b20b548b264f808dbcbb255f6%7C53d82571da19
    > 47e49cb4625a166a4a2a%7C0%7C0%7C638327739033548199%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTi
    > I6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=cGvnA8WqxM%2FUDa%2FNS2OBztr1IWgjCX4IzBYXe1LGkZU%3D&reserved=0
    > > [3]
    > https://lore.kernel.org/netdev/607f4fe4-5a59-39dd-71c2
    > -0cf769b48187%40omp.ru%2F&data=05%7C01%7Cyoshihiro.shimoda.uh%40renesas.com%7C466e046b20b548b264f808dbcbb255f6%7C53d
    > 82571da1947e49cb4625a166a4a2a%7C0%7C0%7C638327739033548199%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luM
    > zIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=OWwBKy%2Fdckgo3clPPfn2hxE4H6ToyqdcbhPhGoqoo30%3D&reserved=0
    > > [4]
    > https://lore.kernel.org/netdev/OSYPR01MB53341CFDBB49A3
    > BA41A6752CD8F9A%40OSYPR01MB5334.jpnprd01.prod.outlook.com%2F&data=05%7C01%7Cyoshihiro.shimoda.uh%40renesas.com%7C466
    > e046b20b548b264f808dbcbb255f6%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C638327739033548199%7CUnknown%7CTWFpbGZsb3
    > d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=Jfypf10jiUfTqWUAukjnPzIQp
    > urx7m0ETF5N2Toq8wE%3D&reserved=0
    > >
    > > 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
    > ======================================================================

    \
     
     \ /
      Last update: 2023-10-13 10:33    [W:3.890 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site