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 <[email protected]>
> ---
> 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
>
_______________________________________________
Openipmi-developer mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
