Re: [PATCH] phy: renesas: rcar-gen3-usb2: Fix use-after-free in rcar_gen3_phy_usb2_remove due to race condition

在 2026/8/4 17:08, Biju Das 写道:
> 
> 
>> -----Original Message-----
>> From: Pei Xiao <[email protected]>
>> Sent: 04 August 2026 10:05
>> To: Biju Das <[email protected]>; Yoshihiro Shimoda <[email protected]>;
>> [email protected]; [email protected]; [email protected]; magnus.damm
>> <[email protected]>; [email protected]; [email protected]; linux-
>> [email protected]
>> Subject: Re: [PATCH] phy: renesas: rcar-gen3-usb2: Fix use-after-free in rcar_gen3_phy_usb2_remove due to
>> race condition
>>
>>
>>
>> 在 2026/8/4 16:34, Biju Das 写道:
>>> Hi Pei Xiao,
>>>
>>> Thanks for the patch.
>>>
>>>> -----Original Message-----
>>>> From: Pei Xiao <[email protected]>
>>>> Sent: 04 August 2026 09:02
>>>> Subject: [PATCH] phy: renesas: rcar-gen3-usb2: Fix use-after-free in
>>>> rcar_gen3_phy_usb2_remove due to race condition
>>>>
>>>> In rcar_gen3_phy_usb2_probe, &channel->work is bound with
>>>> rcar_gen3_phy_usb2_work. rcar_gen3_phy_usb2_irq can schedule this
>>>> work on system_wq via rcar_gen3_device_recognition(), and the role sysfs store can also schedule it
>> via rcar_gen3_init_for_host() / rcar_gen3_init_for_peri().
>>>>
>>>> If we remove the device, rcar_gen3_phy_usb2_remove makes cleanup and
>>>> the memory allocated for channel with devm_kzalloc() is released by
>>>> the devm cleanup after the remove callback returns, while the work
>>>> mentioned above may still be pending or running. The sequence of operations that may lead to a UAF bug
>> is as follows:
>>>>
>>>> CPU0                                      CPU1
>>>>
>>>>                                           | rcar_gen3_phy_usb2_irq
>>>>                                           | rcar_gen3_device_recognition
>>>>                                           | rcar_gen3_init_for_host
>>>>                                           | schedule_work(&ch->work)
>>>> rcar_gen3_phy_usb2_remove                 |
>>>> device_remove_file(&pdev->dev,            |
>>>>                    &dev_attr_role)        |
>>>> // remove returns                         |
>>>> // devm cleanup: free_irq,                |
>>>> // kfree(channel)                         |
>>>>                                           | rcar_gen3_phy_usb2_work
>>>>                                           | // use ch
>>>> (use-after-free)
>>>>
>>>> Fix it by disabling the OTG interrupts, so the IRQ handler cannot
>>>> schedule new work, and canceling the work before the remaining cleanup in rcar_gen3_phy_usb2_remove
>> and the devm release of channel.
>>>>
>>>> Fixes: c14f8a4032ef ("phy: rcar-gen3-usb2: fix mutex_lock calling in
>>>> interrupt")
>>>> Assisted-by: Codex:deepseek-v4-flash
>>>> Signed-off-by: Pei Xiao <[email protected]>
>>>> ---
>>>>  drivers/phy/renesas/phy-rcar-gen3-usb2.c | 10 +++++++++-
>>>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c
>>>> b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
>>>> index 9a45d840efeb..fa0e680a4b91 100644
>>>> --- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c
>>>> +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
>>>> @@ -1067,8 +1067,16 @@ static void rcar_gen3_phy_usb2_remove(struct platform_device *pdev)  {
>>>>  	struct rcar_gen3_chan *channel = platform_get_drvdata(pdev);
>>>>
>>>> -	if (channel->is_otg_channel)
>>>> +	if (channel->is_otg_channel) {
>>>> +		/* Disable OTG interrupts so the IRQ handler cannot
>>>> +		 * schedule new work.
>>>> +		 */
>>>> +		rcar_gen3_control_otg_irq(channel, 0);
>>>> +
>>>>  		device_remove_file(&pdev->dev, &dev_attr_role);
>>>> +
>>>> +		cancel_work_sync(&channel->work);
>>>
>>> What about pending wq that is still about execute after
>>> "device_remove_file(&pdev->dev, &dev_attr_role);" ?
>> Hi Biju,
>>   I don't understand what you mean. cancel_work_sync is exactly what catches this kind of pending work —
>> it either cancels the work that hasn't run yet, or waits for the one that is currently running to finish,
>> and it is placed right.
>>  Could you explain it in more detail?
> 
> Assume you removed the file, and before cancel_work_sync(), the WQ get scheduled
> will it result in UAF bug mentioned in the commit message.
> 
The rcar_gen3_phy_usb2_work only uses chan. If you call
device_remove_file(&pdev->dev, &dev_attr_role) before
cancel_work_sync(&channel->work), it does not lead to a UAF.

On the contrary, if you call cancel_work_sync(&channel->work) first, the
sysfs node has not been removed yet. When the sysfs node is written to
(via store) again, it will schedule the work once more, rendering the
cancel_work_sync call ineffective.
> Maybe??
> 
> rcar_gen3_control_otg_irq(channel, 0);
> cancel_work_sync(&channel->work);
> device_remove_file(&pdev->dev, &dev_attr_role);
> 
> Cheers,
> Biju
> 
> 
> 
> 
>>
>> Thanks！
>> Pei.
>>
>>   > after device_remove_file.
>>
>>> Cheers,
>>> Biju
>>>
>>>> +	}
>>>>  }
>>>>
>>>>  static int rcar_gen3_phy_usb2_suspend(struct device *dev)
>>>> --
>>>> 2.25.1
>>>>
>