Re: [PATCH 2/4] ipmi: bt-bmc: Track open state per device

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] The patch fails to achieve its stated goal of allowing multiple independent BT devices because it hardcodes the miscdevice name, causing a sysfs collision during probe.

Pre-existing issues:
- [High] A race condition between `bt_bmc_release` and `bt_bmc_open` can leave the BT device hardware incorrectly marked as busy, permanently blocking communication.
- [High] Unbinding the driver via sysfs while the miscdevice is held open leads to a Use-After-Free (UAF) and kernel panic.
- [High] Hardware interrupts are not disabled during device removal, leading to a potential unhandled interrupt storm if the IRQ line is shared.
- [High] In `bt_bmc_read()`, passing a `count` of 0 causes an integer underflow that leads to an out-of-bounds `copy_to_user()`, silently corrupting userspace memory.
--

commit a5620fa074169f5d84ab115271732484510fd6a0
Author: Yu-Che Hsieh <yc_hsieh@aspeedtech.com>

ipmi: bt-bmc: Track open state per device

The patch moves the open count from a global variable into the per-device
struct bt_bmc. This is intended to maintain the single-open restriction per
device while allowing multiple distinct BT devices to be opened independently.

> diff --git a/drivers/char/ipmi/bt-bmc.c b/drivers/char/ipmi/bt-bmc.c
> index f3c67272502fb..486ecc0b68159 100644
> --- a/drivers/char/ipmi/bt-bmc.c
> +++ b/drivers/char/ipmi/bt-bmc.c

[ ... ]

> @@ -152,12 +151,12 @@ static int bt_bmc_open(struct inode *inode, struct file *file)
>  {
>  	struct bt_bmc *bt_bmc = file_bt_bmc(file);
>  
> -	if (atomic_inc_return(&open_count) == 1) {
> +	if (atomic_inc_return(&bt_bmc->open_count) == 1) {
>  		clr_b_busy(bt_bmc);
>  		return 0;
>  	}

[Severity: High]
This is a pre-existing issue, but is there a race condition between
bt_bmc_open() and bt_bmc_release()?

If process A closes the device and process B opens it concurrently, could
they interleave like this:

Process A in bt_bmc_release():
    atomic_dec(&bt_bmc->open_count);

Process B in bt_bmc_open():
    if (atomic_inc_return(&bt_bmc->open_count) == 1) {
        clr_b_busy(bt_bmc);
        return 0;
    }

Process A continues in bt_bmc_release():
    set_b_busy(bt_bmc);

Could this sequence leave the newly opened device incorrectly marked as busy
in the hardware, permanently blocking communication?

[ ... ]

> @@ -313,7 +312,7 @@ static int bt_bmc_release(struct inode *inode, struct file *file)
>  {
>  	struct bt_bmc *bt_bmc = file_bt_bmc(file);
>  
> -	atomic_dec(&open_count);
> +	atomic_dec(&bt_bmc->open_count);
>  	set_b_busy(bt_bmc);
>  	return 0;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does this function lead to a
use-after-free if the driver is unbound while the device is held open?

When bt_bmc_remove() unbinds the driver, it deregisters the misc device:

bt_bmc_remove() {
    struct bt_bmc *bt_bmc = dev_get_drvdata(&pdev->dev);
    ...
    misc_deregister(&bt_bmc->miscdev);
}

Because misc_deregister() does not instantly revoke open file descriptors,
the bt_bmc structure can be freed by devres while userspace still has the
file open.

When the file is eventually closed, won't bt_bmc_release() access the freed
bt_bmc pointer to decrement open_count and update the hardware?


[Severity: High]
This is a pre-existing issue, but do we need to disable hardware interrupts
during device removal?

When configuring the IRQ, hardware interrupts are enabled:

bt_bmc_config_irq() {
    ...
    reg |= BT_CR1_IRQ_EN_H2B | BT_CR1_IRQ_EN_HBUSY;
    writel(reg, bt_bmc->base + BT_CR1);
    ...
}

However, bt_bmc_remove() doesn't seem to clear these interrupt enable bits
before the IRQ handler is freed by devres.

If the hardware asserts an interrupt after removal, could this result in an
unhandled interrupt storm that forces the kernel to disable the shared IRQ
line?

[ ... ]

> @@ -425,6 +424,8 @@ static int bt_bmc_probe(struct platform_device *pdev)
>  	if (IS_ERR(bt_bmc->base))
>  		return PTR_ERR(bt_bmc->base);
>  
> +	atomic_set(&bt_bmc->open_count, 0);
> +

[Severity: Medium]
Will this actually allow multiple independent BT devices to operate, as
intended by the commit message?

Later in bt_bmc_probe(), the misc device name is statically assigned:

bt_bmc_probe() {
    ...
    bt_bmc->miscdev.name = DEVICE_NAME;
    ...
    rc = misc_register(&bt_bmc->miscdev);
    ...
}

Because DEVICE_NAME is statically defined as "ipmi-bt-host", won't a second
device probe fail with an -EEXIST error from misc_register() due to sysfs
and /dev name collisions?


[Severity: High]
This isn't a bug introduced by this patch, but could passing a count of zero
to bt_bmc_read() result in an out-of-bounds copy?

Looking at bt_bmc_read():

bt_bmc_read() {
    ...
    if (len + 1 > count)
        len = count - 1;
    ...
}

If count is 0, len + 1 is 1, which is greater than 0, so len becomes 0 - 1.
Because len is an 8-bit unsigned integer, wouldn't this underflow to 255?

This could cause the loop to read 255 bytes from the hardware and copy 256
bytes to userspace using copy_to_user(), which might silently corrupt
memory.

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