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] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 15 Jan 2018 18:40:24 -0600
From:   Corey Minyard <minyard@....org>
To:     Masamitsu Yamazaki <m-yamazaki@...jp.nec.com>,
        "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>
Subject: Re: [PATCH] ipmi: Clear smi_info->thread to prevent use-after-free
 during module unload

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@....org>
> To: openipmi-developer@...ts.sourceforge.net
> Cc: linux-kernel@...r.kernel.org
> Cc: j-nomura@...jp.nec.com
> Cc: k-ueda@...jp.nec.com
> Cc: m-yamazaki@...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@...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)