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

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

[Masamitsu Yamazaki]

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

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

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.

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)

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

[Corey Minyard]

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)

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

[Masamitsu Yamazaki]

Hi Corey,

On 2018/01/16 9:40, Corey Minyard wrote:
> On 01/15/2018 01:58 AM, Masamitsu Yamazaki wrote:
>> 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?

Sorry for the less explanation.

I am not reviewing all codes.
I just found it when I had investigating and reviewing the patch for
commit 4f7f5551a760(ipmi: Stop timers before cleaning up the module)
as it has similar function traces.

And because I noticed it is just a possibility for now, I had wating
for the patch above to be included in the mainline.

Thanks,
Masamitsu Yamazaki


>> 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)