Re: [PATCH v2 1/2] ipmi: kcs_bmc_aspeed: g6: Add KCS Channel 4 over PCIe

[sashiko-bot@kernel.org]

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

New issues:
- [High] Missing PCIE_LPC_ODR4 in switch statement prevents upstream IRQ triggering for channel 5.

Pre-existing issues:
- [Critical] Returning an error from probe frees the kcs_bmc object while it remains linked in the global kcs_bmc_devices list.
- [High] The KCS hardware channel is enabled and the IRQ is registered before the interrupt handler's spinlock is initialized.
- [High] Hardware channel is left enabled on the probe error path if kcs_bmc_add_device fails.
--

--- Patch [1]: [PATCH v2 1/2] ipmi: kcs_bmc_aspeed: g6: Add KCS Channel 4 over PCIe ---
commit 41d8f781b8898d768896e3f02a1b1877d0c1b5ff
Author: Grégoire Layet <gregoire.layet@9elements.com>

ipmi: kcs_bmc_aspeed: g6: Add KCS Channel 4 over PCIe
    
This patch creates a 5th KCS channel to match against the KCS 4 over PCIe
addresses. It exposes a KCS-compatible interface over PCIe for the
ASPEED AST2600.

> diff --git a/drivers/char/ipmi/kcs_bmc_aspeed.c b/drivers/char/ipmi/kcs_bmc_aspeed.c
> index a13a3470c17a4..4d93af2455b8c 100644
> --- a/drivers/char/ipmi/kcs_bmc_aspeed.c
> +++ b/drivers/char/ipmi/kcs_bmc_aspeed.c

[ ... ]

> @@ -191,6 +197,9 @@ static void aspeed_kcs_outb(struct kcs_bmc_device *kcs_bmc, u32 reg, u8 data)
>  	case 4:
>  		regmap_update_bits(priv->map, LPC_HICRC, LPC_HICRC_IRQXE4, LPC_HICRC_IRQXE4);
>  		break;
> +	case 5:
> +		regmap_update_bits(priv->map, PCIE_LPC_HICRC, LPC_HICRC_IRQXE4, LPC_HICRC_IRQXE4);
> +		break;

[Severity: High]
Will this upstream IRQ triggering for channel 5 ever be reached?

Looking earlier in aspeed_kcs_outb(), there is a switch statement
validating the ODR registers:

aspeed_kcs_outb() {
    ...
    switch (reg) {
    case LPC_ODR1:
    case LPC_ODR2:
    case LPC_ODR3:
    case LPC_ODR4:
        break;
    default:
        return;
    }
    ...
}

Since PCIE_LPC_ODR4 is missing from this switch statement, won't any write
to the channel 5 ODR register return early and prevent this new code from
executing?

>  	default:
>  		break;
>  	}

[ ... ]

> @@ -575,11 +605,16 @@ static int aspeed_kcs_probe(struct platform_device *pdev)
>  	if (channel < 0)
>  		return channel;
>  
> +	np = pdev->dev.of_node;
> +	if (channel == 5 && !of_device_is_compatible(np, "aspeed,ast2600-kcs-bmc")) {
> +		dev_err(&pdev->dev, "LPC over PCIE KCS4 only supported on ast2600\n");
> +		return -ENODEV;
> +	}
> +

[Severity: Critical]
This isn't a bug introduced by this patch, but does returning an error from
probe leave a freed object linked in the global list?

If kcs_bmc_add_device() fails later in aspeed_kcs_probe() after linking the
device:

aspeed_kcs_probe() {
    ...
    rc = kcs_bmc_add_device(&priv->kcs_bmc);
    if (rc) {
        dev_warn(&pdev->dev, "Failed to register channel %d: %d\n", ...);
        return rc;
    }
    ...
}

Since kcs_bmc_add_device() leaves the kcs_bmc object in the global
kcs_bmc_devices list on certain errors, and devres will free the priv
structure containing kcs_bmc when we return, could this cause a
use-after-free during subsequent list iterations?

[Severity: High]
This is also a pre-existing issue, but is it safe to enable the hardware
channel before the interrupt handler's spinlock is initialized?

In aspeed_kcs_probe(), the device interrupts are enabled and can fire
immediately:

aspeed_kcs_probe() {
    ...
    aspeed_kcs_irq_mask_update(...);
    aspeed_kcs_enable_channel(kcs_bmc, true);

    rc = kcs_bmc_add_device(&priv->kcs_bmc);
    ...
}

However, kcs_bmc->lock is only initialized later inside
kcs_bmc_add_device(). If an interrupt occurs in this window, will the
handler attempt to acquire an uninitialized lock?

[Severity: High]
This is another pre-existing issue, but is the hardware channel left enabled
if the probe fails here?

In the same error path for kcs_bmc_add_device() shown above, the driver
returns without disabling the channel.

Since the IRQ is shared, could unhandled interrupts from this active device
eventually cause the core IRQ subsystem to disable the shared IRQ line?

>  	nr_addrs = aspeed_kcs_of_get_io_address(pdev, addrs);
>  	if (nr_addrs < 0)
>  		return nr_addrs;
>  
> -	np = pdev->dev.of_node;
>  	rc = of_property_read_u32_array(np, "aspeed,lpc-interrupts", upstream_irq, 2);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1784732161.git.gregoire.layet@9elements.com?part=1