Re: [PATCH net v3] net: ravb: Fix possible UAF bug in ravb_remove

[Sergey Shtylyov]

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@xxxxxxx>
>>>> Reviewed-by: Sergey Shtylyov <s.shtylyov@xxxxxx>
>>>> ---
>>>> 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@xxxxxxxxxx>
>>>
>>> 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