Re: [PATCH 4/4] ipmi: bt-bmc: Read LPC address and SerIRQ from device tree

[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 of `bt_bmc` structure on driver unbind due to `devm` lifetime mismatch.
- [High] Integer underflow in `bt_bmc_read` when `count` is 0 leads to copying up to 256 bytes of kernel buffer into user space, potentially corrupting user memory.
--

commit 8f320638f39284e640c5ee9882a67005b5620ea1
Author: Yu-Che Hsieh <yc_hsieh@aspeedtech.com>
ipmi: bt-bmc: Read LPC address and SerIRQ from device tree

This commit updates the driver to allow the host interface parameters, such
as the LPC IO address and SerIRQ number, to be described by firmware via the
device tree. It falls back to default values for these properties when they
are not present.

> diff --git a/drivers/char/ipmi/bt-bmc.c b/drivers/char/ipmi/bt-bmc.c
> index 486ecc0b68159..6e1f941e63db8 100644
> --- a/drivers/char/ipmi/bt-bmc.c
> +++ b/drivers/char/ipmi/bt-bmc.c
> @@ -65,6 +65,12 @@ struct bt_bmc {
>  	struct timer_list	poll_timer;
>  	struct mutex		mutex;
>  	atomic_t		open_count;
> +	u32			io_addr;
> +
> +	struct {
> +		u32 id;
> +		u32 type;
> +	} sirq;
>  };

[Severity: High]
This isn't a bug introduced by this patch, but does bt_bmc_read() have an
integer underflow when the read count is 0?

If a user space process calls read() with a count of 0, the size calculation
might underflow:

drivers/char/ipmi/bt-bmc.c:bt_bmc_read() {
    ...
	if (len + 1 > count)
		len = count - 1;
    ...
}

If count is 0, could len = count - 1 underflow and become 255 when cast to
u8, causing the subsequent code to perform a 256-byte out-of-bounds copy to
user space?

[ ... ]

> @@ -429,6 +435,33 @@ static int bt_bmc_probe(struct platform_device *pdev)
>  	mutex_init(&bt_bmc->mutex);
>  	init_waitqueue_head(&bt_bmc->queue);

[Severity: High]
This is a pre-existing issue, but could there be a use-after-free of the
bt_bmc structure on driver unbind?

The structure is allocated using devm_kzalloc() in bt_bmc_probe(), tying
its lifetime to the device instead of the file descriptor's lifetime.

When the driver is unbound, bt_bmc_remove() unregisters the misc device but
does not revoke existing file descriptors or wake up waitqueues before
returning:

drivers/char/ipmi/bt-bmc.c:bt_bmc_remove() {
	struct bt_bmc *bt_bmc = dev_get_drvdata(&pdev->dev);

	misc_deregister(&bt_bmc->miscdev);
    ...
}

After bt_bmc_remove() returns, the devres framework frees the bt_bmc
structure. If a reader is blocked in bt_bmc_read():

drivers/char/ipmi/bt-bmc.c:bt_bmc_read() {
    ...
	if (wait_event_interruptible(bt_bmc->queue,
				     bt_inb(bt_bmc, BT_CTRL) & BT_CTRL_H2B_ATN))
    ...
}

Will this blocked reader wake up and access the freed waitqueue and bt_bmc
structure?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260629-aspeed-bt-bmc-multichannel-v1-0-fc23ee337f7a@aspeedtech.com?part=4