Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux

ipmi: Remove smi_msg from waiting_rcv_msgs list before handle_one_recv_msg()

Commit 7ea0ed2b5be8 ("ipmi: Make the message handler easier to use for
SMI interfaces") changed handle_new_recv_msgs() to call handle_one_recv_msg()
for a smi_msg while the smi_msg is still connected to waiting_rcv_msgs list.
That could lead to following list corruption problems:

1) low-level function treats smi_msg as not connected to list

handle_one_recv_msg() could end up calling smi_send(), which
assumes the msg is not connected to list.

For example, the following sequence could corrupt list by
doing list_add_tail() for the entry still connected to other list.

handle_new_recv_msgs()
msg = list_entry(waiting_rcv_msgs)
handle_one_recv_msg(msg)
handle_ipmb_get_msg_cmd(msg)
smi_send(msg)
spin_lock(xmit_msgs_lock)
list_add_tail(msg)
spin_unlock(xmit_msgs_lock)

2) race between multiple handle_new_recv_msgs() instances

handle_new_recv_msgs() once releases waiting_rcv_msgs_lock before calling
handle_one_recv_msg() then retakes the lock and list_del() it.

If others call handle_new_recv_msgs() during the window shown below
list_del() will be done twice for the same smi_msg.

handle_new_recv_msgs()
spin_lock(waiting_rcv_msgs_lock)
msg = list_entry(waiting_rcv_msgs)
spin_unlock(waiting_rcv_msgs_lock)
|
| handle_one_recv_msg(msg)
|
spin_lock(waiting_rcv_msgs_lock)
list_del(msg)
spin_unlock(waiting_rcv_msgs_lock)

Fixes: 7ea0ed2b5be8 ("ipmi: Make the message handler easier to use for SMI interfaces")
Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
[Added a comment to describe why this works.]
Signed-off-by: Corey Minyard <cminyard@mvista.com>
Cc: stable@vger.kernel.org # 3.19
Tested-by: Ye Feng <yefeng.yl@alibaba-inc.com>

authored by

Junichi Nomura and committed by
Corey Minyard
ae4ea9a2 dc03c0f9

+6 -2
+6 -2
drivers/char/ipmi/ipmi_msghandler.c
··· 3820 3820 while (!list_empty(&intf->waiting_rcv_msgs)) { 3821 3821 smi_msg = list_entry(intf->waiting_rcv_msgs.next, 3822 3822 struct ipmi_smi_msg, link); 3823 + list_del(&smi_msg->link); 3823 3824 if (!run_to_completion) 3824 3825 spin_unlock_irqrestore(&intf->waiting_rcv_msgs_lock, 3825 3826 flags); ··· 3830 3829 if (rv > 0) { 3831 3830 /* 3832 3831 * To preserve message order, quit if we 3833 - * can't handle a message. 3832 + * can't handle a message. Add the message 3833 + * back at the head, this is safe because this 3834 + * tasklet is the only thing that pulls the 3835 + * messages. 3834 3836 */ 3837 + list_add(&smi_msg->link, &intf->waiting_rcv_msgs); 3835 3838 break; 3836 3839 } else { 3837 - list_del(&smi_msg->link); 3838 3840 if (rv == 0) 3839 3841 /* Message handled */ 3840 3842 ipmi_free_smi_msg(smi_msg);