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

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
>