linux-kernel - Re: [PATCH net v3] 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: <10f72b0a-663b-7f71-06c2-7315bd0bf368@omp.ru>
Date:   Thu, 20 Jul 2023 00:04:50 +0300
From:   Sergey Shtylyov <s.shtylyov@....ru>
To:     Yunsheng Lin <linyunsheng@...wei.com>,
        Zheng Hacker <hackerzheng666@...il.com>
CC:     Zheng Wang <zyytlz.wz@....com>, <davem@...emloft.net>,
        <edumazet@...gle.com>, <kuba@...nel.org>, <pabeni@...hat.com>,
        <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <1395428693sheep@...il.com>, <alex000young@...il.com>
Subject: Re: [PATCH net v3] net: ravb: Fix possible UAF bug in ravb_remove

Hello!

On 3/13/23 6:32 AM, Yunsheng Lin wrote:
[...]

>>> On 2023/3/12 2:06, Zheng Wang wrote:
>>>> In ravb_probe, priv->work was bound with ravb_tx_timeout_work.
>>>> If timeout occurs, it will start the work. And if we call
>>>> ravb_remove without finishing the work, there may be a
>>>> use-after-free bug on ndev.
>>>>
>>>> Fix it by finishing the job before cleanup in ravb_remove.
>>>>
>>>> Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")
>>>> Signed-off-by: Zheng Wang <zyytlz.wz@....com>
>>>> Reviewed-by: Sergey Shtylyov <s.shtylyov@....ru>
>>>> ---
>>>> v3:
>>>> - fix typo in commit message
>>>> v2:
>>>> - stop dev_watchdog so that handle no more timeout work suggested by Yunsheng Lin,
>>>> add an empty line to make code clear suggested by Sergey Shtylyov
>>>> ---
>>>>  drivers/net/ethernet/renesas/ravb_main.c | 4 ++++
>>>>  1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>>>> index 0f54849a3823..eb63ea788e19 100644
>>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>>>> @@ -2892,6 +2892,10 @@ static int ravb_remove(struct platform_device *pdev)
>>>>       struct ravb_private *priv = netdev_priv(ndev);
>>>>       const struct ravb_hw_info *info = priv->info;
>>>>
>>>> +     netif_carrier_off(ndev);
>>>> +     netif_tx_disable(ndev);
>>>> +     cancel_work_sync(&priv->work);
>>>
>>> LGTM.
>>> Reviewed-by: Yunsheng Lin <linyunsheng@...wei.com>
>>>
>>> As noted by Sergey, ravb_remove() and ravb_close() may
>>> share the same handling, but may require some refactoring
>>> in order to do that. So for a fix, it seems the easiest
>>> way to just add the handling here.
>>
>> Dear Yunsheng,
>>
>> I think Sergey is right for I've seen other drivers' same handling
>> logic. Do you think we should try to move the cancel-work-related code
>> from ravb_remove to ravb_close funtion?
>> Appreciate for your precise advice.
> 
> As Sergey question "can ravb_remove() be called without ravb_close()
> having been called on the bound devices?"
> If I understand it correctly, I think ravb_remove() can be called
> without ravb_close() having been called on the bound devices. I am
> happy to be corrected if I am wrong.

   Yes, correct. It's ravb_remove() that calls unregister_netdev()
which results in calling ravb_close() on the opened devices...

> Yes, you can call *_close() directly in *_remove(), but that may
> require some refactoring and a lot of testing.

   No need to do that I think, as it's called anyways...

> Also, if you found the bug through some static analysis, it may
> be better to make it clear in the commit log and share some info
> about the static analysis, which I suppose it is a tool?

   Agreed. :-)

>> Best regards,
>> Zheng

MBR, Sergey