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

[Zheng Hacker]

Yunsheng Lin <linyunsheng@xxxxxxxxxx> 于2023年3月13日周一 11:32写道：
>
> On 2023/3/13 11:02, Zheng Hacker wrote:
> > Yunsheng Lin <linyunsheng@xxxxxxxxxx> 于2023年3月13日周一 09:15写道：
> >>
> >> 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.
>

Hi Yunsheng,

I'm still not sure. I'll look at code more carefully and see if there
is more proof about it.
And as I'm not familiar with the related code, let's see how Sergey thnks.

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

>
> 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?

Yes, I'll append this msg to commit msg later.

> >
> >>
> >>> +
> >>>       /* Stop PTP Clock driver */
> >>>       if (info->ccc_gac)
> >>>               ravb_ptp_stop(ndev);
> >>>
> > .
> >

Best regards,
Zheng