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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20180116080035.GA29613@53Xg1.linux.bs1.fc.nec.co.jp>
Date:   Tue, 16 Jan 2018 07:54:19 +0000
From:   Masamitsu Yamazaki <m-yamazaki@...jp.nec.com>
To:     "minyard@....org" <minyard@....org>,
        "openipmi-developer@...ts.sourceforge.net" 
        <openipmi-developer@...ts.sourceforge.net>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Junichi Nomura" <j-nomura@...jp.nec.com>,
        Kiyoshi Ueda <k-ueda@...jp.nec.com>,
        Masamitsu Yamazaki <m-yamazaki@...jp.nec.com>
Subject: Re: [PATCH] ipmi: Clear smi_info->thread to prevent use-after-free
 during module unload

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