Re: [PATCH v7 3/4] arm64: dts: nuvoton: npcm845: Add peripheral nodes

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] The `fiu0`, `fiu1`, and `fiu3` SPI flash controllers omit the required `memory` mapping region.
- [Low] The `pspi` (Peripheral SPI) controller is added but not documented in the commit message.

Pre-existing issues:
- [High] The IRQ name passed to `devm_request_irq` is allocated on the stack, leading to a dangling pointer in the IRQ subsystem.
- [High] The fan ISR calculates the module index using virtual IRQ numbers, leading to an out-of-bounds array access and memory corruption.
- [High] The USB device controller driver fails to save the allocated `ci_hdrc` pointer, causing a deterministic NULL pointer dereference on device removal.
- [High] The EDAC driver frees its main control structure on the probe error path while a device-managed interrupt is still registered and active.
--

--- Patch [3]: [PATCH v7 3/4] arm64: dts: nuvoton: npcm845: Add peripheral nodes ---
commit f606ce99eafc7d4c8e2d67d9d5119b1c25593a16
Author: Tomer Maimon <tmaimon77@gmail.com>
Subject: arm64: dts: nuvoton: npcm845: Add peripheral nodes

This commit extends the NPCM845 SoC DTSI by adding peripheral controller nodes
needed by the evaluation board and downstream platforms. It adds nodes for
Ethernet, USB, MMC, FIU, memory controller, RNG, ADC, PWM/FAN, I2C, and the
OP-TEE firmware.

> Extend the NPCM845 SoC DTSI with the peripheral controller nodes needed
> by the evaluation board and downstream platforms. Add the Ethernet MACs,
> USB device controllers and PHY, MMC controller, FIU controllers, memory
> controller, RNG, ADC, PWM/FAN controller, and I2C buses. Also add the
> OP-TEE firmware node needed to describe these blocks.

[Severity: Low]
The pspi (Peripheral SPI) controller node is added by this patch, but it
isn't mentioned in the commit message. Should it be included in this summary?

> diff --git a/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi b/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi
> index 0e5feabf2d718..7608dcf5489c4 100644
> --- a/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi
> +++ b/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi
[ ... ]
> +		mc: memory-controller@f0824000 {
> +			compatible = "nuvoton,npcm845-memory-controller";
> +			reg = <0x0 0xf0824000 0x0 0x1000>;
> +			interrupts = <GIC_SPI 25 IRQ_TYPE_LEVEL_HIGH>;
> +		};

[Severity: High]
This is a pre-existing issue, but adding this memory controller node exposes
a vulnerability in the EDAC driver (drivers/edac/npcm_edac.c).

In drivers/edac/npcm_edac.c:setup_irq(), a devres-managed interrupt is
registered and the hardware is unmasked:

    ret = devm_request_irq(&pdev->dev, irq, edac_ecc_isr, 0,
                           dev_name(&pdev->dev), mci);
    ...
    regmap_write(npcm_regmap, pdata->ctl_int_mask_master,
                 pdata->int_mask_master_non_ecc_mask);

If edac_mc_add_mc() fails later in edac_probe(), the code jumps to the error
path and frees the main control structure:

free_edac_mc:
    edac_mc_free(mci);
    return rc;

Since the interrupt remains registered and active until devres teardown,
could the hardware assert an ECC interrupt during this window, causing
edac_ecc_isr() to dereference the freed mci pointer?

[ ... ]
> +		udc0: usb@f0830000 {
> +			compatible = "nuvoton,npcm845-udc", "nuvoton,npcm750-udc";
> +			reg = <0x0 0xf0830000 0x0 0x1000
> +			       0x0 0xfffeb000 0x0 0x800>;
> +			interrupts = <GIC_SPI 51 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&clk NPCM8XX_CLK_SU>;
> +			clock-names = "clk_usb_bridge";
> +
> +			phys = <&udc0_phy>;
> +			phy_type = "utmi_wide";
> +			dr_mode = "peripheral";
> +			status = "disabled";
> +		};

[Severity: High]
This isn't a bug introduced by this patch, but adding these USB controller
nodes exposes an issue in drivers/usb/chipidea/ci_hdrc_npcm.c.

In drivers/usb/chipidea/ci_hdrc_npcm.c:npcm_udc_probe(), plat_ci is
successfully returned but never assigned to ci->ci:

    plat_ci = ci_hdrc_add_device(dev, pdev->resource, pdev->num_resources,
                                 &ci->pdata);
    if (IS_ERR(plat_ci)) { ... }

When the device is removed, npcm_udc_remove() passes the uninitialized ci->ci
pointer to ci_hdrc_remove_device(), which immediately dereferences it.

Could this lead to a NULL pointer dereference during device unbind?

[ ... ]
> +		fiu0: spi@fb000000 {
> +			compatible = "nuvoton,npcm845-fiu";
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			reg = <0x0 0xfb000000 0x0 0x1000>;
> +			reg-names = "control";
> +			clocks =  <&clk NPCM8XX_CLK_SPI0>;
> +			status = "disabled";
> +		};
> +
> +		fiu1: spi@fb002000 {
> +			compatible = "nuvoton,npcm845-fiu";
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			reg = <0x0 0xfb002000 0x0 0x1000>;
> +			reg-names = "control";
> +			clocks =  <&clk NPCM8XX_CLK_SPI1>;
> +			pinctrl-names = "default";
> +			pinctrl-0 = <&spi1_pins>;
> +			status = "disabled";
> +		};
> +
> +		fiu3: spi@c0000000 {
> +			compatible = "nuvoton,npcm845-fiu";
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			reg = <0x0 0xc0000000 0x0 0x1000>;
> +			reg-names = "control";
> +			clocks =  <&clk NPCM8XX_CLK_SPI3>;
> +			pinctrl-names = "default";
> +			pinctrl-0 = <&spi3_pins>;
> +			status = "disabled";
> +		};

[Severity: Medium]
Do these FIU controller nodes need a 'memory' mapping region?

Without the 'memory' register range, the SPI driver might fail to use
memory-mapped direct reads and drop to UMA (manual) mode, degrading
performance. The fiux node below includes it, but fiu0, fiu1, and fiu3
do not.

[ ... ]
> +			pwm_fan:pwm-fan-controller@103000 {
> +				compatible = "nuvoton,npcm845-pwm-fan";
> +				reg = <0x103000 0x3000>,
> +					<0x180000 0x8000>;
> +				reg-names = "pwm", "fan";
> +				clocks =  <&clk NPCM8XX_CLK_APB3>,
> +					<&clk NPCM8XX_CLK_APB4>;
> +				clock-names = "pwm","fan";
> +				interrupts = <GIC_SPI 96 IRQ_TYPE_LEVEL_HIGH>,
> +						<GIC_SPI 97 IRQ_TYPE_LEVEL_HIGH>,
> +						<GIC_SPI 98 IRQ_TYPE_LEVEL_HIGH>,
> +						<GIC_SPI 99 IRQ_TYPE_LEVEL_HIGH>,
> +						<GIC_SPI 100 IRQ_TYPE_LEVEL_HIGH>,
> +						<GIC_SPI 101 IRQ_TYPE_LEVEL_HIGH>,
> +						<GIC_SPI 102 IRQ_TYPE_LEVEL_HIGH>,
> +						<GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>;
[ ... ]
> +				status = "disabled";
> +			};

[Severity: High]
This isn't a bug introduced by this patch, but enabling this PWM/FAN
controller node triggers a dangling pointer bug in
drivers/hwmon/npcm750-pwm-fan.c.

In drivers/hwmon/npcm750-pwm-fan.c:npcm7xx_pwm_fan_probe(), the IRQ name is
allocated on the stack:

    char name[20];
    ...
    sprintf(name, "NPCM7XX-FAN-MD%d", i);
    ret = devm_request_irq(dev, data->fan_irq[i], npcm7xx_fan_isr, 0, name,
                           (void *)data);

Since devm_request_irq() stores the name pointer in the IRQ descriptor, the
stack frame destruction upon probe return leaves a dangling pointer.

Could this cause a use-after-free or panic when userspace reads
/proc/interrupts?

[Severity: High]
This is also a pre-existing issue, but there is another bug in
drivers/hwmon/npcm750-pwm-fan.c that this node exposes.

In drivers/hwmon/npcm750-pwm-fan.c:npcm7xx_fan_isr(), the module index is
derived by subtracting the first virtual IRQ from the incoming virtual IRQ:

    module = irq - data->fan_irq[0];
    spin_lock_irqsave(&data->fan_lock[module], flags);

Because the kernel's irqdomain does not guarantee that virtual IRQs are
contiguous or allocated in order, could this subtraction result in an
out-of-bounds array access and memory corruption when acquiring the spinlock?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708125352.1915040-1-tmaimon77@gmail.com?part=3