linux-kernel - RE: [PATCH v4] net: ravb: Fix possible UAF bug in ravb

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <93811ef2-8df9-de21-3f8d-296400ec2ba5@de.bosch.com>
Date:   Fri, 13 Oct 2023 08:04:45 +0200
From:   "Behme Dirk (CM/ESO2)" <dirk.behme@...bosch.com>
To:     Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>
CC:     "s.shtylyov@....ru" <s.shtylyov@....ru>,
        "lee@...nel.org" <lee@...nel.org>,
        "linyunsheng@...wei.com" <linyunsheng@...wei.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "richardcochran@...il.com" <richardcochran@...il.com>,
        "p.zabel@...gutronix.de" <p.zabel@...gutronix.de>,
        "geert+renesas@...der.be" <geert+renesas@...der.be>,
        "magnus.damm@...il.com" <magnus.damm@...il.com>,
        Biju Das <biju.das.jz@...renesas.com>,
        "wsa+renesas@...g-engineering.com" <wsa+renesas@...g-engineering.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-renesas-soc@...r.kernel.org" 
        <linux-renesas-soc@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "hackerzheng666@...il.com" <hackerzheng666@...il.com>,
        "1395428693sheep@...il.com" <1395428693sheep@...il.com>,
        "alex000young@...il.com" <alex000young@...il.com>,
        Zheng Wang <zyytlz.wz@....com>,
        Jakub Kicinski <kuba@...nel.org>
Subject: RE: [PATCH v4] net: ravb: Fix possible UAF bug in ravb_remove

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@...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
======================================================================