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

[Corey Minyard <corey@minyard.net>]

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
>