Re: [PATCH v2 4/4] arm64: dts: broadcom: bcm2712: Add reboot modes to firmware node

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Use-After-Free in devm_reboot_mode_unregister during driver unbind
- [High] Endianness bug in reboot mode magic value parsing
--

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

arm64: dts: broadcom: bcm2712: Add reboot modes to firmware node

The raspberry pi firmware driver allows the tryboot reboot mode. Add
this mode and normal boot mode to the node.

> diff --git a/arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b-base.dtsi b/arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b-base.dtsi
> index b7a6bc34ae1ab..67095c7ff770d 100644
> --- a/arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b-base.dtsi
> +++ b/arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b-base.dtsi
> @@ -49,6 +49,8 @@ firmware: rpi-firmware {
>  			compatible = "raspberrypi,bcm2835-firmware", "simple-mfd";
>  
>  			mboxes = <&mailbox>;
> +			mode-normal = <0>;
> +			mode-tryboot = <1>;

[Severity: High]
This is a pre-existing issue, but enabling the reboot mode in the device
tree exposes a use-after-free during driver unbind.

The reboot mode is registered using devres, but is embedded in a manually
refcounted struct:

drivers/firmware/raspberrypi.c:rpi_firmware_probe() {
    ...
    fw->reboot_mode.dev = dev;
    fw->reboot_mode.write = rpi_firmware_reboot_mode_write;
    ret = devm_reboot_mode_register(dev, &fw->reboot_mode);
    ...
}

When the driver is unbound, rpi_firmware_remove() drops the final reference
to fw, freeing it before devres cleanup runs:

drivers/firmware/raspberrypi.c:rpi_firmware_remove() {
    ...
    rpi_firmware_put(fw);
    ...
}

Will this cause memory corruption when the driver core later invokes
devm_reboot_mode_release() and accesses &fw->reboot_mode?


[Severity: High]
This is a pre-existing issue, but does this feature work correctly on
big-endian kernels?

The CPU-endian magic variable is passed by reference to the hardware
communication layer without cpu_to_le32() conversion:

drivers/firmware/raspberrypi.c:rpi_firmware_reboot_mode_write() {
    ...
    if (magic)
        ret = rpi_firmware_property(fw, RPI_FIRMWARE_SET_REBOOT_FLAGS,
                                    &magic, sizeof(magic));
    ...
}

Since the reboot-mode framework parses the device tree property into a
CPU-endian unsigned int, passing it directly to rpi_firmware_property() will
send the wrong value to the little-endian VideoCore firmware.

Should magic be converted to little-endian before it is sent to the firmware?

>  
>  			firmware_clocks: clocks {
>  				compatible = "raspberrypi,firmware-clocks";

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