Re: [PATCH v3 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:

Pre-existing issues:
- [High] Uninitialized spinlock access in interrupt handler.
- [High] Hardware channel remains enabled when `kcs_bmc_add_device()` fails.
- [Medium] OBE timer race condition allows infinite background polling.
- [Medium] Unchecked dereference of `pdev->dev.of_node`.
--

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

This commit adds a 5th KCS channel for the ASPEED AST2600 to support
KCS 4 over PCIe. It configures the appropriate registers and restricts
usage of this 5th channel to compatible AST2600 controllers.

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

[ ... ]

> @@ -452,6 +479,10 @@ static void aspeed_kcs_irq_mask_update(struct kcs_bmc_device *kcs_bmc, u8 mask,
>  			regmap_update_bits(priv->map, LPC_HICRB, LPC_HICRB_IBFIE4,
>  					   enable * LPC_HICRB_IBFIE4);
>  			return;
> +		case 5:
> +			regmap_update_bits(priv->map, PCIE_LPC_HICRB, LPC_HICRB_IBFIE4,
> +					   enable * LPC_HICRB_IBFIE4);
> +			return;
>  		default:
>  			pr_warn("%s: Unsupported channel: %d", __func__, kcs_bmc->channel);
>  			return;

[Severity: Medium]
This is a pre-existing issue, but since we are updating channel 5 here, I
wanted to ask about the OBE timer logic earlier in this function.

When disabling the OBE event, aspeed_kcs_irq_mask_update() calls
timer_delete(&priv->obe.timer). Because timer_delete() is asynchronous, if a
concurrent timer handler (aspeed_kcs_check_obe) is executing on another CPU,
it might observe the hardware OBF status is still set and call mod_timer().

Could this result in the timer endlessly re-arming itself and polling the
hardware indefinitely in the background? Might timer_delete_sync() or a shared
shutdown flag be needed here to prevent that?

[ ... ]

> @@ -575,11 +607,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;
> +	}
> +
>  	nr_addrs = aspeed_kcs_of_get_io_address(pdev, addrs);
>  	if (nr_addrs < 0)
>  		return nr_addrs;
>  
> -	np = pdev->dev.of_node;

[Severity: Medium]
This isn't a bug introduced by this patch, but I noticed pdev->dev.of_node is
used here in aspeed_kcs_probe().

Earlier in aspeed_kcs_probe(), we have this assignment:

aspeed_kcs_probe() {
    ...
    np = pdev->dev.of_node->parent;
    ...

Is there a scenario where this driver could be manually bound via sysfs by a
root user without a valid Device Tree node? If that happens, would
dereferencing pdev->dev.of_node->parent cause a kernel panic?

[Severity: High]
This is a pre-existing issue, but I have a question about the probe error
path further down in aspeed_kcs_probe().

During probe, the driver enables the hardware channel and then registers the
device:

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

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

If kcs_bmc_add_device() fails, the hardware remains active on the host bus.
If the device asserts an interrupt in this state, could it result in an
unhandled interrupt storm since the channel is never explicitly disabled on
this error path?

[Severity: High]
This isn't a bug introduced by this patch, but looking at the sequence above
in aspeed_kcs_probe(), there appears to be a window where the hardware is
fully enabled to assert IRQs before kcs_bmc_add_device() is called.

If an interrupt fires immediately after aspeed_kcs_enable_channel(), the
registered handler aspeed_kcs_irq() will trigger and call
kcs_bmc_handle_event(), which attempts to take kcs_bmc->lock.

However, kcs_bmc->lock is initialized inside kcs_bmc_add_device():

kcs_bmc_add_device() {
    ...
    spin_lock_init(&kcs_bmc->lock);
    ...
}

Does this mean the interrupt handler could operate on an uninitialized
spinlock if the hardware asserts an IRQ before kcs_bmc_add_device() finishes?

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