[PATCH] ipmi: fix NULL pointer on smi_work

[PATCH] ipmi: fix NULL pointer on smi_work

[Breno Leitao]

I am getting the following crash on IPMI on linus' upstream. It tries to
double-add the same element to a list, and then get
a slab-use-after-free in handle_one_recv_msg.

Here is the decoded stack against commit cf38b2340c0e ("Merge tag
'soc-fixes-6.19-2' of git://git.kernel.org/pub/scm/linux/kernel/git/soc/soc")

	list_add double add: new=ffff888145b19000, prev=ffff888145b19000, next=ffff88810bb6d480.
	  WARNING: lib/list_debug.c:37 at __list_add_valid_or_report+0x10a/0x130, CPU#64: 0/408
	  Workqueue: events smi_work [ipmi_msghandler]
	  RIP: 0010:__list_add_valid_or_report (rw/compile/lib/list_debug.c:35)

	  deliver_response (rw/compile/./include/linux/list.h:158 rw/compile/./include/linux/list.h:191 rw/compile/drivers/char/ipmi/ipmi_msghandler.c:974) ipmi_msghandler
	  smi_work (rw/compile/drivers/char/ipmi/ipmi_msghandler.c:985 rw/compile/drivers/char/ipmi/ipmi_msghandler.c:999 rw/compile/drivers/char/ipmi/ipmi_msghandler.c:4853) ipmi_msghandler
	  ? process_scheduled_works (rw/compile/kernel/workqueue.c:3233 rw/compile/kernel/workqueue.c:3340)
	  process_scheduled_works (rw/compile/kernel/workqueue.c:? rw/compile/kernel/workqueue.c:3340)
	  worker_thread (rw/compile/./include/linux/list.h:381 rw/compile/kernel/workqueue.c:946 rw/compile/kernel/workqueue.c:3422)
	  kthread (rw/compile/kernel/kthread.c:465)
	  ? pr_cont_work (rw/compile/kernel/workqueue.c:3367)
	  ? kthread_blkcg (rw/compile/kernel/kthread.c:412)
	  ret_from_fork (rw/compile/arch/x86/kernel/process.c:164)
	  ? kthread_blkcg (rw/compile/kernel/kthread.c:412)
	  ret_from_fork_asm (rw/compile/arch/x86/entry/entry_64.S:256)

	list_add double add: new=ffff888145b19000, prev=ffff888145b19000, next=ffff88810bb6d480.
	  WARNING: lib/list_debug.c:37 at __list_add_valid_or_report+0x10a/0x130, CPU#64: 0/408
	  <double add hit again same stack>

	BUG: KASAN: slab-use-after-free in handle_one_recv_msg (rw/compile/drivers/char/ipmi/ipmi_msghandler.c:? rw/compile/drivers/char/ipmi/ipmi_msghandler.c:4761) ipmi_msghandler
	  T473136] Write of size 4 at addr ffff888145b19010 by task kworker/30:3/473136
	  handle_new_recv_msgs (rw/compile/drivers/char/ipmi/ipmi_msghandler.c:4788) ipmi_msghandler
	  ? get_smi_info (rw/compile/drivers/char/ipmi/ipmi_si_intf.c:918) ipmi_si
	  smi_work (rw/compile/drivers/char/ipmi/ipmi_msghandler.c:?) ipmi_msghandler
	  ? process_scheduled_works (rw/compile/kernel/workqueue.c:3233 rw/compile/kernel/workqueue.c:3340)
	  process_scheduled_works (rw/compile/kernel/workqueue.c:? rw/compile/kernel/workqueue.c:3340)
	  worker_thread (rw/compile/./include/linux/list.h:381 rw/compile/kernel/workqueue.c:946 rw/compile/kernel/workqueue.c:3422)
	  kthread (rw/compile/kernel/kthread.c:465)
	  ? rcu_is_watching (rw/compile/./include/linux/context_tracking.h:128 rw/compile/kernel/rcu/tree.c:751)
	  ? pr_cont_work (rw/compile/kernel/workqueue.c:3367)
	  ? kthread_blkcg (rw/compile/kernel/kthread.c:412)
	  ret_from_fork (rw/compile/arch/x86/kernel/process.c:164)
	  ? kthread_blkcg (rw/compile/kernel/kthread.c:412)
	  ret_from_fork_asm (rw/compile/arch/x86/entry/entry_64.S:256)

	Allocated by task 6379:
	  kasan_save_track (rw/compile/mm/kasan/common.c:58 rw/compile/mm/kasan/common.c:78)
	  __kasan_kmalloc (rw/compile/mm/kasan/common.c:419)
	  __kmalloc_cache_noprof (rw/compile/mm/slub.c:5781)
	  kernfs_fop_open.llvm.1481521202032378051 (rw/compile/./include/linux/slab.h:957 rw/compile/./include/linux/slab.h:1094 rw/compile/fs/kernfs/file.c:641)
	  do_dentry_open (rw/compile/fs/open.c:963)
	  vfs_open (rw/compile/fs/open.c:1095)
	  path_openat (rw/compile/fs/namei.c:4638 rw/compile/fs/namei.c:4796)
	  do_filp_open (rw/compile/fs/namei.c:4823)
	  do_sys_openat2 (rw/compile/./include/linux/err.h:78 rw/compile/./include/linux/file.h:177 rw/compile/fs/open.c:1430)
	  __x64_sys_openat (rw/compile/fs/open.c:1447)
	  do_syscall_64 (rw/compile/arch/x86/entry/syscall_64.c:?)
	  entry_SYSCALL_64_after_hwframe (rw/compile/arch/x86/entry/entry_64.S:131)

	Freed by task 6379:
	  kasan_save_track (rw/compile/mm/kasan/common.c:58 rw/compile/mm/kasan/common.c:78)
	  kasan_save_free_info (rw/compile/mm/kasan/generic.c:587)
	  __kasan_slab_free (rw/compile/mm/kasan/common.c:287)
	  kfree (rw/compile/mm/slub.c:6674 rw/compile/mm/slub.c:6882)
	  kernfs_fop_release.llvm.1481521202032378051 (rw/compile/fs/kernfs/file.c:788)
	  __fput (rw/compile/fs/file_table.c:469)
	  fput_close_sync (rw/compile/fs/file_table.c:574)
	  __x64_sys_close (rw/compile/fs/open.c:1575 rw/compile/fs/open.c:1558 rw/compile/fs/open.c:1558)
	  do_syscall_64 (rw/compile/arch/x86/entry/syscall_64.c:?)
	  entry_SYSCALL_64_after_hwframe (rw/compile/arch/x86/entry/entry_64.S:131)

	  BUG: kernel NULL pointer dereference, address: 0000000000000000
	  #PF: supervisor instruction fetch in kernel mode
	  #PF: error_code(0x0010) - not-present page
	  PGD 1d14bb067 P4D 1d14bb067 PUD 67c50d067 PMD 0
	  Oops: Oops: 0010 [#1] SMP DEBUG_PAGEALLOC KASAN
	  Hardware name: Quanta North Dome MP/North Dome MP, BIOS F09C_3B14.sign 04/12/2023
	  Workqueue: events smi_work [ipmi_msghandler]

The next patch contains the issue I found and a possible fix.

Signed-off-by: Breno Leitao <leitao@debian.org>
---
Breno Leitao (1):
      ipmi: Fix use-after-free and list corruption on sender error

 drivers/char/ipmi/ipmi_msghandler.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)
---
base-commit: cf38b2340c0e60ef695b7137440a4d187ed49c88
change-id: 20260127-ipmi-03bae4a027bd

Best regards,
--  
Breno Leitao <leitao@debian.org>

[PATCH] ipmi: Fix use-after-free and list corruption on sender error

[Breno Leitao]

When the SMI sender returns an error, smi_work() delivers an error
response but then jumps back to restart without cleaning up properly:

1. intf->curr_msg is not cleared, so no new message is pulled
2. newmsg still points to the message, causing sender() to be called
   again with the same message
3. If sender() fails again, deliver_err_response() is called with
   the same recv_msg that was already queued for delivery

This causes list_add corruption ("list_add double add") because the
recv_msg is added to the user_msgs list twice. Subsequently, the
corrupted list leads to use-after-free when the memory is freed and
reused, and eventually a NULL pointer dereference when accessing
recv_msg->done.

The buggy sequence:

  sender() fails
    -> deliver_err_response(recv_msg)  // recv_msg queued for delivery
    -> goto restart                    // curr_msg not cleared!
  sender() fails again (same message!)
    -> deliver_err_response(recv_msg)  // tries to queue same recv_msg
    -> LIST CORRUPTION

Fix by introducing a send_failed flag that signals when sender()
returns an error. At the restart label, inside the existing spinlock
critical section, check this flag and clear curr_msg if set. This
ensures:

- The smi_msg is always freed after sender error
- curr_msg is cleared so the next iteration pulls a new message
- No stale pointers remain that could cause use-after-free
- Only one lock acquisition per iteration (avoids extra lock/unlock
  in the error path)

Fixes: 9cf93a8fa9513 ("ipmi: Allow an SMI sender to return an error")
Signed-off-by: Breno Leitao <leitao@debian.org>
---
 drivers/char/ipmi/ipmi_msghandler.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
index 3f48fc6ab596d..81259c93261fb 100644
--- a/drivers/char/ipmi/ipmi_msghandler.c
+++ b/drivers/char/ipmi/ipmi_msghandler.c
@@ -4816,6 +4816,7 @@ static void smi_work(struct work_struct *t)
 	int run_to_completion = READ_ONCE(intf->run_to_completion);
 	struct ipmi_smi_msg *newmsg = NULL;
 	struct ipmi_recv_msg *msg, *msg2;
+	bool send_failed = false;
 	int cc;
 
 	/*
@@ -4828,6 +4829,16 @@ static void smi_work(struct work_struct *t)
 restart:
 	if (!run_to_completion)
 		spin_lock_irqsave(&intf->xmit_msgs_lock, flags);
+	if (send_failed) {
+		/*
+		 * Sender failed, clear curr_msg so we can pull a new
+		 * message. Do not clear it unconditionally as a message
+		 * may be in flight from a previous run.
+		 */
+		intf->curr_msg = NULL;
+		send_failed = false;
+	}
+	newmsg = NULL;
 	if (intf->curr_msg == NULL && !intf->in_shutdown) {
 		struct list_head *entry = NULL;
 
@@ -4852,8 +4863,14 @@ static void smi_work(struct work_struct *t)
 			if (newmsg->recv_msg)
 				deliver_err_response(intf,
 						     newmsg->recv_msg, cc);
-			else
-				ipmi_free_smi_msg(newmsg);
+			/*
+			 * Sender returned error, the lower layer did not
+			 * take ownership of the message. The transaction
+			 * is abandoned - the user has been notified via
+			 * deliver_err_response() above.
+			 */
+			ipmi_free_smi_msg(newmsg);
+			send_failed = true;
 			goto restart;
 		}
 	}

-- 
2.47.3

Re: [PATCH] ipmi: Fix use-after-free and list corruption on sender error

[Corey Minyard]

On Tue, Jan 27, 2026 at 01:57:59AM -0800, Breno Leitao wrote:
> When the SMI sender returns an error, smi_work() delivers an error
> response but then jumps back to restart without cleaning up properly:
> 
> 1. intf->curr_msg is not cleared, so no new message is pulled
> 2. newmsg still points to the message, causing sender() to be called
>    again with the same message
> 3. If sender() fails again, deliver_err_response() is called with
>    the same recv_msg that was already queued for delivery

Yes, this is indeed a problem and your analysis is correct.  It looks
like it designed with this in mind and never properly completed.

However, there are some problems with your fix:

* You leave the message in intf->curr_msg after it has been freed, which
  can result in a use after free or other incorrect behavior.  It might
  be ok in this case, but it's a bad idea in general.

* The send_failed flags is unnecessary, you could just check for
  newmsg.

* Doing the lock/unlock in error handling is not a big deal.
  That should be an exceptional case.  Adding the check every
  time in the normal flow is probably more expensive in the long run.

I'll send out a patch for this.  I also want to change the locks
and run to completion check, it's hurting my eyes the way it is now.

Thank you for the report, I really appreaciate it.

-corey

> 
> This causes list_add corruption ("list_add double add") because the
> recv_msg is added to the user_msgs list twice. Subsequently, the
> corrupted list leads to use-after-free when the memory is freed and
> reused, and eventually a NULL pointer dereference when accessing
> recv_msg->done.
> 
> The buggy sequence:
> 
>   sender() fails
>     -> deliver_err_response(recv_msg)  // recv_msg queued for delivery
>     -> goto restart                    // curr_msg not cleared!
>   sender() fails again (same message!)
>     -> deliver_err_response(recv_msg)  // tries to queue same recv_msg
>     -> LIST CORRUPTION
> 
> Fix by introducing a send_failed flag that signals when sender()
> returns an error. At the restart label, inside the existing spinlock
> critical section, check this flag and clear curr_msg if set. This
> ensures:
> 
> - The smi_msg is always freed after sender error
> - curr_msg is cleared so the next iteration pulls a new message
> - No stale pointers remain that could cause use-after-free
> - Only one lock acquisition per iteration (avoids extra lock/unlock
>   in the error path)
> 
> Fixes: 9cf93a8fa9513 ("ipmi: Allow an SMI sender to return an error")
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
>  drivers/char/ipmi/ipmi_msghandler.c | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
> index 3f48fc6ab596d..81259c93261fb 100644
> --- a/drivers/char/ipmi/ipmi_msghandler.c
> +++ b/drivers/char/ipmi/ipmi_msghandler.c
> @@ -4816,6 +4816,7 @@ static void smi_work(struct work_struct *t)
>  	int run_to_completion = READ_ONCE(intf->run_to_completion);
>  	struct ipmi_smi_msg *newmsg = NULL;
>  	struct ipmi_recv_msg *msg, *msg2;
> +	bool send_failed = false;
>  	int cc;
>  
>  	/*
> @@ -4828,6 +4829,16 @@ static void smi_work(struct work_struct *t)
>  restart:
>  	if (!run_to_completion)
>  		spin_lock_irqsave(&intf->xmit_msgs_lock, flags);
> +	if (send_failed) {
> +		/*
> +		 * Sender failed, clear curr_msg so we can pull a new
> +		 * message. Do not clear it unconditionally as a message
> +		 * may be in flight from a previous run.
> +		 */
> +		intf->curr_msg = NULL;
> +		send_failed = false;
> +	}
> +	newmsg = NULL;
>  	if (intf->curr_msg == NULL && !intf->in_shutdown) {
>  		struct list_head *entry = NULL;
>  
> @@ -4852,8 +4863,14 @@ static void smi_work(struct work_struct *t)
>  			if (newmsg->recv_msg)
>  				deliver_err_response(intf,
>  						     newmsg->recv_msg, cc);
> -			else
> -				ipmi_free_smi_msg(newmsg);
> +			/*
> +			 * Sender returned error, the lower layer did not
> +			 * take ownership of the message. The transaction
> +			 * is abandoned - the user has been notified via
> +			 * deliver_err_response() above.
> +			 */
> +			ipmi_free_smi_msg(newmsg);
> +			send_failed = true;
>  			goto restart;
>  		}
>  	}
> 
> -- 
> 2.47.3
> 

(no subject)

[Corey Minyard]

Fix a bug in error handling in the IPMI driver, reported by Breno
Leitao.

Also move the xmit_msgs_lock lock to a function and move the run to
completion check there so the if check isn't on every one, making
things hard to read.

Passes the IPMI test suite.  Adding a test for this would be very
difficult, unfortunately.

Thanks Breno and others who obviously worked on this.

-corey

[PATCH 1/2] ipmi: Fix use-after-free and list corruption on sender error

[Corey Minyard]

The analysis from Breno:

When the SMI sender returns an error, smi_work() delivers an error
response but then jumps back to restart without cleaning up properly:

1. intf->curr_msg is not cleared, so no new message is pulled
2. newmsg still points to the message, causing sender() to be called
   again with the same message
3. If sender() fails again, deliver_err_response() is called with
   the same recv_msg that was already queued for delivery

This causes list_add corruption ("list_add double add") because the
recv_msg is added to the user_msgs list twice. Subsequently, the
corrupted list leads to use-after-free when the memory is freed and
reused, and eventually a NULL pointer dereference when accessing
recv_msg->done.

The buggy sequence:

  sender() fails
    -> deliver_err_response(recv_msg)  // recv_msg queued for delivery
    -> goto restart                    // curr_msg not cleared!
  sender() fails again (same message!)
    -> deliver_err_response(recv_msg)  // tries to queue same recv_msg
    -> LIST CORRUPTION

Fix this by freeing the message and setting it to NULL on a send error.
Also, always free the newmsg on a send error, otherwise it will leak.

Reported-by: Breno Leitao <leitao@debian.org>
Fixes: 9cf93a8fa9513 ("ipmi: Allow an SMI sender to return an error")
Signed-off-by: Corey Minyard <corey@minyard.net>
---
 drivers/char/ipmi/ipmi_msghandler.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
index 3f48fc6ab596..a590a67294e2 100644
--- a/drivers/char/ipmi/ipmi_msghandler.c
+++ b/drivers/char/ipmi/ipmi_msghandler.c
@@ -4852,8 +4852,15 @@ static void smi_work(struct work_struct *t)
 			if (newmsg->recv_msg)
 				deliver_err_response(intf,
 						     newmsg->recv_msg, cc);
-			else
-				ipmi_free_smi_msg(newmsg);
+			if (!run_to_completion)
+				spin_lock_irqsave(&intf->xmit_msgs_lock,
+						  flags);
+			intf->curr_msg = NULL;
+			if (!run_to_completion)
+				spin_unlock_irqrestore(&intf->xmit_msgs_lock,
+						       flags);
+			ipmi_free_smi_msg(newmsg);
+			newmsg = NULL;
 			goto restart;
 		}
 	}
-- 
2.43.0

[PATCH 2/2] ipmi: Consolidate the run to completion checking for xmit msgs lock

[Corey Minyard]

It made things hard to read, move the check to a function.

Signed-off-by: Corey Minyard <corey@minyard.net>
---
 drivers/char/ipmi/ipmi_msghandler.c | 40 ++++++++++++++++-------------
 1 file changed, 22 insertions(+), 18 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
index a590a67294e2..030828cdb778 100644
--- a/drivers/char/ipmi/ipmi_msghandler.c
+++ b/drivers/char/ipmi/ipmi_msghandler.c
@@ -602,6 +602,20 @@ static int __ipmi_bmc_register(struct ipmi_smi *intf,
 static int __scan_channels(struct ipmi_smi *intf,
 				struct ipmi_device_id *id, bool rescan);
 
+static void ipmi_lock_xmit_msgs(struct ipmi_smi *intf, int run_to_completion,
+				unsigned long *flags)
+{
+	if (!run_to_completion)
+		spin_lock_irqsave(&intf->xmit_msgs_lock, *flags);
+}
+
+static void ipmi_unlock_xmit_msgs(struct ipmi_smi *intf, int run_to_completion,
+				  unsigned long *flags)
+{
+	if (!run_to_completion)
+		spin_unlock_irqrestore(&intf->xmit_msgs_lock, *flags);
+}
+
 static void free_ipmi_user(struct kref *ref)
 {
 	struct ipmi_user *user = container_of(ref, struct ipmi_user, refcount);
@@ -1878,11 +1892,9 @@ static void smi_send(struct ipmi_smi *intf,
 	int run_to_completion = READ_ONCE(intf->run_to_completion);
 	unsigned long flags = 0;
 
-	if (!run_to_completion)
-		spin_lock_irqsave(&intf->xmit_msgs_lock, flags);
+	ipmi_lock_xmit_msgs(intf, run_to_completion, &flags);
 	smi_msg = smi_add_send_msg(intf, smi_msg, priority);
-	if (!run_to_completion)
-		spin_unlock_irqrestore(&intf->xmit_msgs_lock, flags);
+	ipmi_unlock_xmit_msgs(intf, run_to_completion, &flags);
 
 	if (smi_msg)
 		handlers->sender(intf->send_info, smi_msg);
@@ -4826,8 +4838,7 @@ static void smi_work(struct work_struct *t)
 	 * message delivery.
 	 */
 restart:
-	if (!run_to_completion)
-		spin_lock_irqsave(&intf->xmit_msgs_lock, flags);
+	ipmi_lock_xmit_msgs(intf, run_to_completion, &flags);
 	if (intf->curr_msg == NULL && !intf->in_shutdown) {
 		struct list_head *entry = NULL;
 
@@ -4843,8 +4854,7 @@ static void smi_work(struct work_struct *t)
 			intf->curr_msg = newmsg;
 		}
 	}
-	if (!run_to_completion)
-		spin_unlock_irqrestore(&intf->xmit_msgs_lock, flags);
+	ipmi_unlock_xmit_msgs(intf, run_to_completion, &flags);
 
 	if (newmsg) {
 		cc = intf->handlers->sender(intf->send_info, newmsg);
@@ -4852,13 +4862,9 @@ static void smi_work(struct work_struct *t)
 			if (newmsg->recv_msg)
 				deliver_err_response(intf,
 						     newmsg->recv_msg, cc);
-			if (!run_to_completion)
-				spin_lock_irqsave(&intf->xmit_msgs_lock,
-						  flags);
+			ipmi_lock_xmit_msgs(intf, run_to_completion, &flags);
 			intf->curr_msg = NULL;
-			if (!run_to_completion)
-				spin_unlock_irqrestore(&intf->xmit_msgs_lock,
-						       flags);
+			ipmi_unlock_xmit_msgs(intf, run_to_completion, &flags);
 			ipmi_free_smi_msg(newmsg);
 			newmsg = NULL;
 			goto restart;
@@ -4928,16 +4934,14 @@ void ipmi_smi_msg_received(struct ipmi_smi *intf,
 		spin_unlock_irqrestore(&intf->waiting_rcv_msgs_lock,
 				       flags);
 
-	if (!run_to_completion)
-		spin_lock_irqsave(&intf->xmit_msgs_lock, flags);
+	ipmi_lock_xmit_msgs(intf, run_to_completion, &flags);
 	/*
 	 * We can get an asynchronous event or receive message in addition
 	 * to commands we send.
 	 */
 	if (msg == intf->curr_msg)
 		intf->curr_msg = NULL;
-	if (!run_to_completion)
-		spin_unlock_irqrestore(&intf->xmit_msgs_lock, flags);
+	ipmi_unlock_xmit_msgs(intf, run_to_completion, &flags);
 
 	if (run_to_completion)
 		smi_work(&intf->smi_work);
-- 
2.43.0

Re: [PATCH 1/2] ipmi: Fix use-after-free and list corruption on sender error

[Breno Leitao]

On Tue, Jan 27, 2026 at 07:54:39AM -0600, Corey Minyard wrote:
> The analysis from Breno:
> 
> When the SMI sender returns an error, smi_work() delivers an error
> response but then jumps back to restart without cleaning up properly:
> 
> 1. intf->curr_msg is not cleared, so no new message is pulled
> 2. newmsg still points to the message, causing sender() to be called
>    again with the same message
> 3. If sender() fails again, deliver_err_response() is called with
>    the same recv_msg that was already queued for delivery
> 
> This causes list_add corruption ("list_add double add") because the
> recv_msg is added to the user_msgs list twice. Subsequently, the
> corrupted list leads to use-after-free when the memory is freed and
> reused, and eventually a NULL pointer dereference when accessing
> recv_msg->done.
> 
> The buggy sequence:
> 
>   sender() fails
>     -> deliver_err_response(recv_msg)  // recv_msg queued for delivery
>     -> goto restart                    // curr_msg not cleared!
>   sender() fails again (same message!)
>     -> deliver_err_response(recv_msg)  // tries to queue same recv_msg
>     -> LIST CORRUPTION
> 
> Fix this by freeing the message and setting it to NULL on a send error.
> Also, always free the newmsg on a send error, otherwise it will leak.
> 
> Reported-by: Breno Leitao <leitao@debian.org>
> Fixes: 9cf93a8fa9513 ("ipmi: Allow an SMI sender to return an error")
> Signed-off-by: Corey Minyard <corey@minyard.net>

Reviewed-by: Breno Leitao <leitao@debian.org>

--breno

Re: [PATCH 2/2] ipmi: Consolidate the run to completion checking for xmit msgs lock

[Breno Leitao]

On Tue, Jan 27, 2026 at 07:54:40AM -0600, Corey Minyard wrote:
> It made things hard to read, move the check to a function.
> 
> Signed-off-by: Corey Minyard <corey@minyard.net>
> ---
>  drivers/char/ipmi/ipmi_msghandler.c | 40 ++++++++++++++++-------------
>  1 file changed, 22 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
> index a590a67294e2..030828cdb778 100644
> --- a/drivers/char/ipmi/ipmi_msghandler.c
> +++ b/drivers/char/ipmi/ipmi_msghandler.c
> @@ -602,6 +602,20 @@ static int __ipmi_bmc_register(struct ipmi_smi *intf,
>  static int __scan_channels(struct ipmi_smi *intf,
>  				struct ipmi_device_id *id, bool rescan);
>  
> +static void ipmi_lock_xmit_msgs(struct ipmi_smi *intf, int run_to_completion,
> +				unsigned long *flags)
> +{
> +	if (!run_to_completion)
> +		spin_lock_irqsave(&intf->xmit_msgs_lock, *flags);
> +}

I usually see the opposite construction in most cases. Something like:

	static void ipmi_lock_xmit_msgs(struct ipmi_smi *intf, int run_to_completion,
					unsigned long *flags)
	{
		if (run_to_completion)
			return;

		spin_lock_irqsave(&intf->xmit_msgs_lock, *flags);
	}

Thanks for doing this, this looks way better!
--breno

Re: [PATCH 2/2] ipmi: Consolidate the run to completion checking for xmit msgs lock

[Corey Minyard]

On Tue, Jan 27, 2026 at 06:41:48AM -0800, Breno Leitao wrote:
> On Tue, Jan 27, 2026 at 07:54:40AM -0600, Corey Minyard wrote:
> > It made things hard to read, move the check to a function.
> > 
> > Signed-off-by: Corey Minyard <corey@minyard.net>
> > ---
> >  drivers/char/ipmi/ipmi_msghandler.c | 40 ++++++++++++++++-------------
> >  1 file changed, 22 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
> > index a590a67294e2..030828cdb778 100644
> > --- a/drivers/char/ipmi/ipmi_msghandler.c
> > +++ b/drivers/char/ipmi/ipmi_msghandler.c
> > @@ -602,6 +602,20 @@ static int __ipmi_bmc_register(struct ipmi_smi *intf,
> >  static int __scan_channels(struct ipmi_smi *intf,
> >  				struct ipmi_device_id *id, bool rescan);
> >  
> > +static void ipmi_lock_xmit_msgs(struct ipmi_smi *intf, int run_to_completion,
> > +				unsigned long *flags)
> > +{
> > +	if (!run_to_completion)
> > +		spin_lock_irqsave(&intf->xmit_msgs_lock, *flags);
> > +}
> 
> I usually see the opposite construction in most cases. Something like:
> 
> 	static void ipmi_lock_xmit_msgs(struct ipmi_smi *intf, int run_to_completion,
> 					unsigned long *flags)
> 	{
> 		if (run_to_completion)
> 			return;
> 
> 		spin_lock_irqsave(&intf->xmit_msgs_lock, *flags);
> 	}

Yes, that's better, I've changed it.

> 
> Thanks for doing this, this looks way better!

No problem.  It was more for my own benefit :-).

-corey

> --breno

Re: [PATCH 1/2] ipmi: Fix use-after-free and list corruption on sender error

[Corey Minyard]

On Tue, Jan 27, 2026 at 06:40:04AM -0800, Breno Leitao wrote:
> On Tue, Jan 27, 2026 at 07:54:39AM -0600, Corey Minyard wrote:
> > The analysis from Breno:
> > 
> > When the SMI sender returns an error, smi_work() delivers an error
> > response but then jumps back to restart without cleaning up properly:
> > 
> > 1. intf->curr_msg is not cleared, so no new message is pulled
> > 2. newmsg still points to the message, causing sender() to be called
> >    again with the same message
> > 3. If sender() fails again, deliver_err_response() is called with
> >    the same recv_msg that was already queued for delivery
> > 
> > This causes list_add corruption ("list_add double add") because the
> > recv_msg is added to the user_msgs list twice. Subsequently, the
> > corrupted list leads to use-after-free when the memory is freed and
> > reused, and eventually a NULL pointer dereference when accessing
> > recv_msg->done.
> > 
> > The buggy sequence:
> > 
> >   sender() fails
> >     -> deliver_err_response(recv_msg)  // recv_msg queued for delivery
> >     -> goto restart                    // curr_msg not cleared!
> >   sender() fails again (same message!)
> >     -> deliver_err_response(recv_msg)  // tries to queue same recv_msg
> >     -> LIST CORRUPTION
> > 
> > Fix this by freeing the message and setting it to NULL on a send error.
> > Also, always free the newmsg on a send error, otherwise it will leak.
> > 
> > Reported-by: Breno Leitao <leitao@debian.org>
> > Fixes: 9cf93a8fa9513 ("ipmi: Allow an SMI sender to return an error")
> > Signed-off-by: Corey Minyard <corey@minyard.net>
> 
> Reviewed-by: Breno Leitao <leitao@debian.org>

Thanks, I'll put this into the next tree and send it up after a bit
if everything is ok there.

-corey

> 
> --breno

Re: [PATCH 2/2] ipmi: Consolidate the run to completion checking for xmit msgs lock

[Breno Leitao]

On Tue, Jan 27, 2026 at 08:46:06AM -0600, Corey Minyard wrote:
> On Tue, Jan 27, 2026 at 06:41:48AM -0800, Breno Leitao wrote:
> > On Tue, Jan 27, 2026 at 07:54:40AM -0600, Corey Minyard wrote:
> > > It made things hard to read, move the check to a function.
> > > 
> > > Signed-off-by: Corey Minyard <corey@minyard.net>
> > > ---
> > >  drivers/char/ipmi/ipmi_msghandler.c | 40 ++++++++++++++++-------------
> > >  1 file changed, 22 insertions(+), 18 deletions(-)
> > > 
> > > diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
> > > index a590a67294e2..030828cdb778 100644
> > > --- a/drivers/char/ipmi/ipmi_msghandler.c
> > > +++ b/drivers/char/ipmi/ipmi_msghandler.c
> > > @@ -602,6 +602,20 @@ static int __ipmi_bmc_register(struct ipmi_smi *intf,
> > >  static int __scan_channels(struct ipmi_smi *intf,
> > >  				struct ipmi_device_id *id, bool rescan);
> > >  
> > > +static void ipmi_lock_xmit_msgs(struct ipmi_smi *intf, int run_to_completion,
> > > +				unsigned long *flags)
> > > +{
> > > +	if (!run_to_completion)
> > > +		spin_lock_irqsave(&intf->xmit_msgs_lock, *flags);
> > > +}
> > 
> > I usually see the opposite construction in most cases. Something like:
> > 
> > 	static void ipmi_lock_xmit_msgs(struct ipmi_smi *intf, int run_to_completion,
> > 					unsigned long *flags)
> > 	{
> > 		if (run_to_completion)
> > 			return;
> > 
> > 		spin_lock_irqsave(&intf->xmit_msgs_lock, *flags);
> > 	}
> 
> Yes, that's better, I've changed it.

Thanks. feel free to add:

Reviewed-by: Breno Leitao <leitao@debian.org>

Thanks for the quick replies,
--breno