Re: [PATCH RESEND v3 3/4] firmware: raspberrypi: Add reboot mode supporty

On Thu, Jul 30, 2026 at 11:11:44AM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] fw->cl.dev becomes a dangling pointer if the firmware device is destroyed while a consumer holds a reference to fw.
> - [High] fw can be accessed after it is freed because drvdata is not cleared before freeing fw during driver unbind or probe failure.
> --
Yes they are real issues, I started to work on a patch for this. But as these
are pre-existing issues i will send a separate patch for this issues.

> 
> --- Patch [3]: [PATCH RESEND v3 3/4] firmware: raspberrypi: Add reboot mode support ---
> commit 7622e7ef47694e51ed746363fd587b5883406973
> Author: Gregor Herburger <[email protected]>
> 
> firmware: raspberrypi: Add reboot mode support
> 
> The Raspberry Pi firmware has a tryboot mode where it tries to boot from
> another partition. This commit enables this by registering a reboot mode
> driver. It also replaces the manual rpi_firmware_put() with a
> devres-managed call to safely register the reboot mode.
> 
> > diff --git a/drivers/firmware/raspberrypi.c b/drivers/firmware/raspberrypi.c
> > index 0aa322e9a2e73..2a0c40b8052e5 100644
> > --- a/drivers/firmware/raspberrypi.c
> > +++ b/drivers/firmware/raspberrypi.c
> [ ... ]
> > @@ -302,9 +331,14 @@ static int rpi_firmware_probe(struct platform_device *pdev)
> >  
> >  	platform_set_drvdata(pdev, fw);
> >  
> > +	ret = devm_add_action_or_reset(dev, devm_rpi_firmware_put, fw);
> > +	if (ret)
> > +		return ret;
> 
> [Severity: High]
> This is a pre-existing issue, but can fw be accessed after it is freed
> because drvdata is not explicitly cleared?
> 
> During unbind, devm_rpi_firmware_put is executed by devres, which drops the
> kref and frees fw. However, the driver core does not clear drvdata until
> device_unbind_cleanup completes (which happens after devres actions).
> 
> If rpi_firmware_get() is called concurrently by another thread, would it
> fetch the uncleared but freed fw pointer via platform_get_drvdata(pdev) and
> call kref_get_unless_zero(&fw->consumers), resulting in a use-after-free?
> 
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but wouldn't fw->cl.dev become a
> dangling pointer if the firmware device is destroyed while a consumer holds a
> reference to fw?
> 
> In rpi_firmware_get(), the consumer retrieves the firmware context and
> increments its kref (fw->consumers), but then drops the reference to the
> platform device itself:
> 
> drivers/firmware/raspberrypi.c:rpi_firmware_get() {
>     ...
>     put_device(&pdev->dev);
>     ...
> }
> 
> If the device is subsequently unregistered (e.g., via DT overlay removal), the
> device struct is freed, but the consumer still holds a reference to fw.
> 
> Any subsequent failing mailbox transaction will call dev_err(fw->cl.dev, ...)
> in rpi_firmware_transaction() and dereference the freed device pointer.
> 
> >  	rpi_firmware_print_firmware_revision(fw);
> >  	rpi_register_hwmon_driver(dev, fw);
> >  	rpi_register_clk_driver(dev);
> > +	rpi_register_reboot_mode(dev, fw);
> >  
> >  	return 0;
> >  }
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3

-- 
Gregor Herburger
Linutronix GmbH | Bahnhofstrasse 3 | D-88690 Uhldingen-Mühlhofen
Phone: +49 7556 25 999 35; Fax.: +49 7556 25 999 99

Hinweise zum Datenschutz finden Sie hier (Informations on data privacy 
can be found here): https://linutronix.de/legal/data-protection.php

Linutronix GmbH | Firmensitz (Registered Office): Uhldingen-Mühlhofen | 
Registergericht (Registration Court): Amtsgericht Freiburg i.Br., HRB700 
806 | Geschäftsführer (Managing Directors): Dr. Wilfried Wessner, 
Katharina Kopp, Alexander Gieringer