Re: [PATCH RFC 2/2] ipmi: kcs_bmc_aspeed: Support multiple LPC controller instances

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] If `kcs_bmc_add_device()` fails, the `kcs_bmc` device remains on the global `kcs_bmc_devices` list, causing a use-after-free when devres cleans up.
- [High] The driver requests a shared IRQ before initializing the `kcs_bmc->lock` spinlock, which can cause a crash if a shared interrupt fires early.
- [High] The IPMI chardev frees its private data before all file descriptors are closed, leading to a use-after-free on driver unbind.
--

--- Patch [2]: [PATCH RFC 2/2] ipmi: kcs_bmc_aspeed: Support multiple LPC controller instances ---
commit 528fa21291ddfb0afac162723f5334b19c0f6d20
Author: Yu-Che Hsieh <yc_hsieh@aspeedtech.com>

ipmi: kcs_bmc_aspeed: Support multiple LPC controller instances

This commit introduces support for multiple physical LPC controllers by
splitting the KCS channel representation into a driver-private channel for
register access and a globally unique channel for the chardev exposed to
userspace. It derives the global channel number using an optional "lpcN"
alias on the LPC controller node.

> diff --git a/drivers/char/ipmi/kcs_bmc_aspeed.c b/drivers/char/ipmi/kcs_bmc_aspeed.c
> index a13a3470c17a4..591c11a445039 100644
> --- a/drivers/char/ipmi/kcs_bmc_aspeed.c
> +++ b/drivers/char/ipmi/kcs_bmc_aspeed.c
[ ... ]
> @@ -590,9 +605,11 @@ static int aspeed_kcs_probe(struct platform_device *pdev)
>  	if (!priv)
>  		return -ENOMEM;
>  
> +	priv->channel = channel;
> +
>  	kcs_bmc = &priv->kcs_bmc;
>  	kcs_bmc->dev = &pdev->dev;
> -	kcs_bmc->channel = channel;
> +	kcs_bmc->channel = bank * KCS_CHANNEL_MAX + channel;
>  	kcs_bmc->ioreg = ast_kcs_bmc_ioregs[channel - 1];
>  	kcs_bmc->ops = &aspeed_kcs_ops;
>  

[Severity: High]
This isn't a bug introduced by this patch, but does a failure in
kcs_bmc_add_device() leave a dangling pointer on the global list?

If any driver's add_device callback fails, kcs_bmc_add_device() returns an
error without unlinking kcs_bmc->entry from the kcs_bmc_devices list.
This error propagates back to aspeed_kcs_probe(), which returns it:

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

The devres framework will then free the priv memory containing the kcs_bmc
object, leaving a dangling pointer in the global kcs_bmc_devices list.
Could this lead to a use-after-free during subsequent driver registrations?

[Severity: High]
This is also a pre-existing issue, but is it safe to request a shared IRQ
before the spinlock is initialized?

In aspeed_kcs_probe(), aspeed_kcs_config_downstream_irq() is called before
kcs_bmc_add_device():

	/* Host to BMC IRQ */
	rc = aspeed_kcs_config_downstream_irq(kcs_bmc, pdev);
	if (rc)
		return rc;

aspeed_kcs_config_downstream_irq() uses devm_request_irq() with IRQF_SHARED,
which activates the aspeed_kcs_irq() handler. The handler immediately tries
to acquire kcs_bmc->lock.

However, kcs_bmc->lock isn't initialized until kcs_bmc_add_device() is
called later in the probe sequence. If a shared device on the same IRQ line
fires an interrupt in this window, could the handler dereference an
uninitialized spinlock?

[Severity: High]
This is another pre-existing issue in the IPMI chardev code, but does
kcs_bmc_ipmi_remove_device() free the private data before all file
descriptors are closed?

In drivers/char/ipmi/kcs_bmc_cdev_ipmi.c, kcs_bmc_ipmi_remove_device()
unregisters the miscdev and explicitly frees the priv struct:

	misc_deregister(&priv->miscdev);
	kcs_bmc_disable_device(priv->client.dev, &priv->client);
	devm_kfree(kcs_bmc->dev, priv->kbuffer);
	// ...
	devm_kfree(kcs_bmc->dev, priv);

Since misc_deregister() doesn't close existing file descriptors or wait for
active file operations to complete, could a user process that is sleeping in
kcs_bmc_ipmi_read() access the freed priv->mutex and priv->kbuffer when it
wakes up?

	wait_event_interruptible(priv->queue,
				 priv->data_in_avail);

	mutex_lock(&priv->mutex);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723-upstream_kcs_multiple_lpc-v1-0-d918b5270b86@aspeedtech.com?part=2