Re: [PATCH] ipmi: Clear smi_info->thread to prevent use-after-free during module unload

[Corey Minyard <minyard@acm.org>]

On 01/15/2018 01:58 AM, Masamitsu Yamazaki wrote:
> Subject:[PATCH] ipmi: Clear smi_info->thread to prevent use-after-free during module unload
> To: Corey Minyard <minyard@acm.org>
> To: openipmi-developer@lists.sourceforge.net
> Cc: linux-kernel@vger.kernel.org
> Cc: j-nomura@ce.jp.nec.com
> Cc: k-ueda@ct.jp.nec.com
> Cc: m-yamazaki@ah.jp.nec.com

In the future, please don't include the above, and please don't use a 
normal mailer to send the
email.  It came MIME encoded, so it's very hard to deal with.  Use "git 
send-email" to send, if
at all possible.

>
> During code inspection, I found an use-after-free possibility during unloading
> ipmi_si in the polling mode.

I'm curious, what exactly is this code inspection part of?  Are you 
reviewing all the code
in the kernel?  Are you having certain people own certain drivers?

> If start_new_msg() is called after kthread_stop(), the function will try to
> wake up non-existing kthread using the dangling pointer.
>
> Possible scenario is when a new internal message is generated after
> ipmi_unregister_smi()[*1] and remains after stop_timer_and_thread()
> in clenaup_one_si() [*2].
> Use-after-free could occur as follows depending on BMC replies.
>
>    cleanup_one_si
>      => ipmi_unregister_smi
>         [*1]
>      => stop_timer_and_thread
>         => kthread_stop(smi_info->thread)
>         [*2]
>      => poll
>         => smi_event_handler
>            => start_new_msg
>               => if (smi_info->thread)
>                      wake_up_process(smi_info->thread) <== use-after-free!!
>
> Although currently it seems no such message is generated in the polling mode,
> some changes might introduce that in thefuture. For example in the interrupt
> mode, disable_si_irq() does that at [*2].
>
> So let's prevent such a critical issue possibility now.

I don't have a problem with this, it's included for the next merge window.

Thanks,

-corey

>
> Signed-off-by: Yamazaki Masamitsu <m-yamazaki@ah.jp.nec.com>
>
>
> diff -Nurp a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
> --- a/drivers/char/ipmi/ipmi_si_intf.c	2018-01-09 13:40:14.624785104 +0900
> +++ b/drivers/char/ipmi/ipmi_si_intf.c	2018-01-09 13:44:39.174780570 +0900
> @@ -1938,8 +1938,10 @@ static void check_for_broken_irqs(struct
>   
>   static inline void stop_timer_and_thread(struct smi_info *smi_info)
>   {
> -	if (smi_info->thread != NULL)
> +	if (smi_info->thread != NULL) {
>   		kthread_stop(smi_info->thread);
> +		smi_info->thread = NULL;
> +	}
>   
>   	smi_info->timer_can_start = false;
>   	if (smi_info->timer_running)