Re: [PATCH v2 3/4] firmware: raspberrypi: Add reboot mode support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Use-After-Free in devm_reboot_mode_release due to incorrect devres registration.
- [Medium] The reboot mode magic value is sent to the firmware in CPU byte order, which breaks on big-endian kernels.
- [Low] RASPBERRYPI_FIRMWARE selects REBOOT_MODE without depending on POWER_RESET, causing a linker error.
--

commit c3a66741771139fdb5972557da035f4b4fb75ce1
Author: Gregor Herburger <gregor.herburger@linutronix.de>

firmware: raspberrypi: Add reboot mode support

The Raspberry Pi firmware has a tryboot mode where it tries to boot from
another partition. This can be used to create a A/B update schema. Add
support for this by registering a reboot mode driver.

> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> --- a/drivers/firmware/Kconfig
> +++ b/drivers/firmware/Kconfig
> @@ -115,6 +115,7 @@ config ISCSI_IBFT
>  config RASPBERRYPI_FIRMWARE
>  	tristate "Raspberry Pi Firmware Driver"
>  	depends on BCM2835_MBOX
> +	select REBOOT_MODE

[Severity: Low]
Since REBOOT_MODE implicitly depends on POWER_RESET, does selecting it here
without also adding `depends on POWER_RESET` cause unmet direct dependency
warnings and subsequent linker errors when POWER_RESET is disabled?

>  	help
>  	  This option enables support for communicating with the firmware on the
>  	  Raspberry Pi.

[ ... ]

> diff --git a/drivers/firmware/raspberrypi.c b/drivers/firmware/raspberrypi.c
> --- a/drivers/firmware/raspberrypi.c
> +++ b/drivers/firmware/raspberrypi.c

[ ... ]

> @@ -273,10 +275,25 @@ static void devm_rpi_firmware_put(void *data)
>  	rpi_firmware_put(fw);
>  }
>  
> +static int rpi_firmware_reboot_mode_write(struct reboot_mode_driver *reboot,
> +					  unsigned int magic)
> +{
> +	struct rpi_firmware *fw = container_of(reboot, struct rpi_firmware,
> +					       reboot_mode);
> +	int ret = 0;
> +
> +	if (magic)
> +		ret = rpi_firmware_property(fw, RPI_FIRMWARE_SET_REBOOT_FLAGS,
> +					    &magic, sizeof(magic));

[Severity: Medium]
The `magic` parameter is passed by reference directly to the firmware here.

Since the firmware expects a little-endian 32-bit value, does this cause
the reboot mode flag to be misinterpreted on big-endian kernels?

Should this value be converted with `cpu_to_le32()` before sending it?

> +
> +	return ret;
> +}

[ ... ]

> @@ -306,6 +323,12 @@ static int rpi_firmware_probe(struct platform_device *pdev)
>  	rpi_register_hwmon_driver(dev, fw);
>  	rpi_register_clk_driver(dev);
>  
> +	fw->reboot_mode.dev = dev;
> +	fw->reboot_mode.write = rpi_firmware_reboot_mode_write;
> +	ret = devm_reboot_mode_register(dev, &fw->reboot_mode);

[Severity: Critical]
Does unbinding the driver lead to a use-after-free in the devres cleanup
phase?

The `fw` structure is allocated and managed by a kref, which gets dropped in
`rpi_firmware_remove()` when the driver unbinds, freeing the `fw` memory.

However, `devm_reboot_mode_register()` ties the cleanup of the reboot mode
driver to the device's devres lifecycle. Since devres cleanup happens after
the remove function returns, `devm_reboot_mode_release()` will attempt to
access `&fw->reboot_mode` after the memory has already been freed.

> +	if (ret)
> +		dev_err(dev, "Failed to register reboot mode: %d\n", ret);
> +
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260630-rpi-tryboot-v2-0-f68d2dc6aa27@linutronix.de?part=3