[PATCH] ipmi: ssif_bmc: keep device alive across remove

[PATCH] ipmi: ssif_bmc: keep device alive across remove

[Qing Ming]

misc_deregister() prevents new opens but does not close existing files.
ssif_bmc_ctx is allocated with devm_kzalloc(), so I2C device removal
releases the object while an open /dev/ipmi-ssif-host file can still reach
it through poll and release.

Keeping /dev/ipmi-ssif-host open across I2C removal and then polling the
old file triggers:

  BUG: KASAN: slab-use-after-free in do_raw_spin_lock
  ssif_bmc_poll

Allocate ssif_bmc_ctx explicitly and hold a reference for each successful
open. Mark the device dead before deregistering the miscdevice, wake
blocked operations, report a hangup to poll, and make empty reads and new
writes return -ENODEV after removal. Unregister the I2C target before
deleting the response timer so a target callback cannot rearm the timer
during teardown.

If I2C target registration fails after the miscdevice is visible, mark the
device dead and deregister it before dropping the probe reference. This
keeps any file that raced with the probe failure valid until its release.

Fixes: dd2bc5cc9e25 ("ipmi: ssif_bmc: Add SSIF BMC driver")
Cc: stable@vger.kernel.org
Signed-off-by: Qing Ming <a0yami@mailbox.org>
---
 drivers/char/ipmi/ssif_bmc.c | 77 ++++++++++++++++++++++++++++++------
 1 file changed, 65 insertions(+), 12 deletions(-)

diff --git a/drivers/char/ipmi/ssif_bmc.c b/drivers/char/ipmi/ssif_bmc.c
index 6036897725f3..84616fa2c8fd 100644
--- a/drivers/char/ipmi/ssif_bmc.c
+++ b/drivers/char/ipmi/ssif_bmc.c
@@ -7,12 +7,14 @@
  */
 
 #include <linux/i2c.h>
+#include <linux/kref.h>
 #include <linux/miscdevice.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/poll.h>
 #include <linux/sched.h>
+#include <linux/slab.h>
 #include <linux/mutex.h>
 #include <linux/spinlock.h>
 #include <linux/timer.h>
@@ -78,6 +80,7 @@ enum ssif_state {
 };
 
 struct ssif_bmc_ctx {
+	struct kref            refcount;
 	struct i2c_client       *client;
 	struct miscdevice       miscdev;
 	int                     msg_idx;
@@ -101,6 +104,7 @@ struct ssif_bmc_ctx {
 	bool                    response_in_progress;
 	bool                    busy;
 	bool                    aborting;
+	bool                    dead;
 	/* Buffer for SSIF Transaction part*/
 	struct ssif_part_buffer part_buf;
 	struct ipmi_ssif_msg    response;
@@ -109,7 +113,15 @@ struct ssif_bmc_ctx {
 
 static inline struct ssif_bmc_ctx *to_ssif_bmc(struct file *file)
 {
-	return container_of(file->private_data, struct ssif_bmc_ctx, miscdev);
+	return file->private_data;
+}
+
+static void ssif_bmc_free(struct kref *refcount)
+{
+	struct ssif_bmc_ctx *ssif_bmc;
+
+	ssif_bmc = container_of(refcount, struct ssif_bmc_ctx, refcount);
+	kfree(ssif_bmc);
 }
 
 static const char *state_to_string(enum ssif_state state)
@@ -142,10 +154,15 @@ static ssize_t ssif_bmc_read(struct file *file, char __user *buf, size_t count,
 
 	spin_lock_irqsave(&ssif_bmc->lock, flags);
 	while (!ssif_bmc->request_available) {
+		if (ssif_bmc->dead) {
+			spin_unlock_irqrestore(&ssif_bmc->lock, flags);
+			return -ENODEV;
+		}
 		spin_unlock_irqrestore(&ssif_bmc->lock, flags);
 		if (file->f_flags & O_NONBLOCK)
 			return -EAGAIN;
 		ret = wait_event_interruptible(ssif_bmc->wait_queue,
+					       READ_ONCE(ssif_bmc->dead) ||
 					       ssif_bmc->request_available);
 		if (ret)
 			return ret;
@@ -195,15 +212,24 @@ static ssize_t ssif_bmc_write(struct file *file, const char __user *buf, size_t
 
 	spin_lock_irqsave(&ssif_bmc->lock, flags);
 	while (ssif_bmc->response_in_progress) {
+		if (ssif_bmc->dead) {
+			ret = -ENODEV;
+			goto exit;
+		}
 		spin_unlock_irqrestore(&ssif_bmc->lock, flags);
 		if (file->f_flags & O_NONBLOCK)
 			return -EAGAIN;
 		ret = wait_event_interruptible(ssif_bmc->wait_queue,
+					       READ_ONCE(ssif_bmc->dead) ||
 					       !ssif_bmc->response_in_progress);
 		if (ret)
 			return ret;
 		spin_lock_irqsave(&ssif_bmc->lock, flags);
 	}
+	if (ssif_bmc->dead) {
+		ret = -ENODEV;
+		goto exit;
+	}
 
 	/*
 	 * The write must complete before the response timeout fired, otherwise
@@ -235,14 +261,21 @@ static ssize_t ssif_bmc_write(struct file *file, const char __user *buf, size_t
 
 static int ssif_bmc_open(struct inode *inode, struct file *file)
 {
-	struct ssif_bmc_ctx *ssif_bmc = to_ssif_bmc(file);
+	struct ssif_bmc_ctx *ssif_bmc;
 	int ret = 0;
 
+	ssif_bmc = container_of(file->private_data, struct ssif_bmc_ctx,
+				miscdev);
 	spin_lock_irq(&ssif_bmc->lock);
-	if (!ssif_bmc->running)
+	if (ssif_bmc->dead) {
+		ret = -ENODEV;
+	} else if (!ssif_bmc->running) {
 		ssif_bmc->running = 1;
-	else
+		kref_get(&ssif_bmc->refcount);
+		file->private_data = ssif_bmc;
+	} else {
 		ret = -EBUSY;
+	}
 	spin_unlock_irq(&ssif_bmc->lock);
 
 	return ret;
@@ -259,6 +292,8 @@ static __poll_t ssif_bmc_poll(struct file *file, poll_table *wait)
 	/* The request is available, userspace application can get the request */
 	if (ssif_bmc->request_available)
 		mask |= EPOLLIN;
+	if (ssif_bmc->dead)
+		mask |= EPOLLERR | EPOLLHUP;
 
 	spin_unlock_irq(&ssif_bmc->lock);
 
@@ -272,6 +307,7 @@ static int ssif_bmc_release(struct inode *inode, struct file *file)
 	spin_lock_irq(&ssif_bmc->lock);
 	ssif_bmc->running = 0;
 	spin_unlock_irq(&ssif_bmc->lock);
+	kref_put(&ssif_bmc->refcount, ssif_bmc_free);
 
 	return 0;
 }
@@ -825,9 +861,10 @@ static int ssif_bmc_probe(struct i2c_client *client)
 	struct ssif_bmc_ctx *ssif_bmc;
 	int ret;
 
-	ssif_bmc = devm_kzalloc(&client->dev, sizeof(*ssif_bmc), GFP_KERNEL);
+	ssif_bmc = kzalloc_obj(struct ssif_bmc_ctx);
 	if (!ssif_bmc)
 		return -ENOMEM;
+	kref_init(&ssif_bmc->refcount);
 
 	spin_lock_init(&ssif_bmc->lock);
 
@@ -837,6 +874,8 @@ static int ssif_bmc_probe(struct i2c_client *client)
 	ssif_bmc->busy = false;
 	ssif_bmc->response_timer_inited = false;
 
+	ssif_bmc->client = client;
+	ssif_bmc->client->flags |= I2C_CLIENT_SLAVE;
 	/* Register misc device interface */
 	ssif_bmc->miscdev.minor = MISC_DYNAMIC_MINOR;
 	ssif_bmc->miscdev.name = DEVICE_NAME;
@@ -844,17 +883,25 @@ static int ssif_bmc_probe(struct i2c_client *client)
 	ssif_bmc->miscdev.parent = &client->dev;
 	ret = misc_register(&ssif_bmc->miscdev);
 	if (ret)
-		return ret;
-
-	ssif_bmc->client = client;
-	ssif_bmc->client->flags |= I2C_CLIENT_SLAVE;
+		goto err_put;
 
 	/* Register I2C slave */
 	i2c_set_clientdata(client, ssif_bmc);
 	ret = i2c_slave_register(client, ssif_bmc_cb);
 	if (ret)
-		misc_deregister(&ssif_bmc->miscdev);
+		goto err_misc;
+
+	return 0;
 
+err_misc:
+	spin_lock_irq(&ssif_bmc->lock);
+	ssif_bmc->dead = true;
+	spin_unlock_irq(&ssif_bmc->lock);
+	misc_deregister(&ssif_bmc->miscdev);
+	wake_up_all(&ssif_bmc->wait_queue);
+	i2c_set_clientdata(client, NULL);
+err_put:
+	kref_put(&ssif_bmc->refcount, ssif_bmc_free);
 	return ret;
 }
 
@@ -862,9 +909,15 @@ static void ssif_bmc_remove(struct i2c_client *client)
 {
 	struct ssif_bmc_ctx *ssif_bmc = i2c_get_clientdata(client);
 
-	timer_delete_sync(&ssif_bmc->response_timer);
-	i2c_slave_unregister(client);
+	spin_lock_irq(&ssif_bmc->lock);
+	ssif_bmc->dead = true;
+	spin_unlock_irq(&ssif_bmc->lock);
 	misc_deregister(&ssif_bmc->miscdev);
+	wake_up_all(&ssif_bmc->wait_queue);
+	i2c_slave_unregister(client);
+	timer_delete_sync(&ssif_bmc->response_timer);
+	i2c_set_clientdata(client, NULL);
+	kref_put(&ssif_bmc->refcount, ssif_bmc_free);
 }
 
 static const struct of_device_id ssif_bmc_match[] = {
-- 
2.53.0

Re: [PATCH] ipmi: ssif_bmc: keep device alive across remove

[Chad Aldreda]

On Wed, 5 Aug 2026, Qing Ming wrote:
> Allocate ssif_bmc_ctx explicitly and hold a reference for each successful
> open. Mark the device dead before deregistering the miscdevice, wake
> blocked operations, report a hangup to poll, and make empty reads and new
> writes return -ENODEV after removal.

Tested on mainline d48723711bae (v7.4-rc1-672), where this still applies,
in QEMU with KASAN, lockdep, kmemleak and DEBUG_OBJECTS_TIMERS. There is
no SSIF hardware involved: a test-only I2C adapter module with target
support binds the driver, sends single-part write requests and reads
back the responses the way a host would.

Without the patch, sysfs unbind, delete_device or removing the adapter
with /dev/ipmi-ssif-host open gives slab-use-after-free reports in
ssif_bmc_poll(), ssif_bmc_read(), ssif_bmc_write() and
ssif_bmc_release(). A read(), poll() or write() blocked across the
removal is not woken, and hits freed memory when it is signalled.

With the patch there are no KASAN, lockdep or kmemleak reports. After
each of the three removals, poll() returns POLLERR|POLLHUP, and read()
and write() return -ENODEV. Operations blocked across each removal wake
up: read(), and a write() blocked behind an unsent response, return
-ENODEV, and poll() returns POLLERR|POLLHUP.

Normal use still works: a second open gets -EBUSY, a blocking read() and
poll() are woken by a new request, the request arrives intact, and the
host reads back a response of the length written to the node (the stub
checks the length, not the bytes). These also ran clean:
an unbind with the response timer armed, an unbind and bind with the old
file still open (the old file gets -ENODEV and the new one works),
closing with an unread request, probe failures from target registration
and from misc_register(), and 200 unbind/bind cycles against six
processes opening, polling, reading and writing while requests are sent
and responses read. The same stress run on the unpatched kernel produces
KASAN reports.

Separate from this patch, and the same with or without it: removing the
device before any request has arrived gives a debugobjects WARN,
"ODEBUG: assert_init not available ... object type: timer_list".
remove() calls timer_delete_sync() on response_timer, which is only
set up in handle_request(); the call came in with 7fc3e2546cf3 ("ipmi:
ssif_bmc: cancel response timer on remove"). There is no KASAN report.
From reading the code, setting the timer up in probe would avoid it; I
have not tried that.

Not tested: real hardware (the stub delivers target events from
process context, not an interrupt), multi-part requests and responses,
PEC, epoll, the driver's KUnit tests, and module unload (the driver was
built in). The stress run used non-blocking file operations only.

The harness and this report were produced with an AI assistant (Claude
Opus 5.5), which also did the earlier runs, including the unpatched
comparison. I ran the final version on the patched kernel myself and
got the same results. The harness is available on request.

Tested-by: Chad Aldreda <ca@modelmiser.com>