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