linux-kernel - Re: [PATCH] ALSA: Use del_timer

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]

Message-ID: <9330bab0-1fbf-95a7-8d97-af522762f470@roeck-us.net>
Date:   Wed, 26 Oct 2022 21:04:22 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     Steven Rostedt <rostedt@...dmis.org>,
        LKML <linux-kernel@...r.kernel.org>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Stephen Boyd <sboyd@...nel.org>,
        Jaroslav Kysela <perex@...ex.cz>,
        Takashi Iwai <tiwai@...e.com>, alsa-devel@...a-project.org
Subject: Re: [PATCH] ALSA: Use del_timer_sync() before freeing timer

On 10/26/22 20:12, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@...dmis.org>
> 
> The current code for freeing the emux timer is extremely dangerous:
> 
>    CPU0				CPU1
>    ----				----
> snd_emux_timer_callback()
> 			    snd_emux_free()
> 			      spin_lock(&emu->voice_lock)
> 			      del_timer(&emu->tlist); <-- returns immediately
> 			      spin_unlock(&emu->voice_lock);
> 			      [..]
> 			      kfree(emu);
> 
>    spin_lock(&emu->voice_lock);
> 
>   [BOOM!]
> 
> Instead just use del_timer_sync() which will wait for the timer to finish
> before continuing. No need to check if the timer is active or not when
> doing so.
> 
> This doesn't fix the race of a possible re-arming of the timer, but at
> least it won't use the data that has just been freed.
> 
> Cc: stable@...r.kernel.org
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Steven Rostedt (Google) <rostedt@...dmis.org>

Reviewed-by: Guenter Roeck <linux@...ck-us.net>

> ---
>   sound/synth/emux/emux.c | 5 +----
>   1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/sound/synth/emux/emux.c b/sound/synth/emux/emux.c
> index 5ed8e36d2e04..a2ee78809cfb 100644
> --- a/sound/synth/emux/emux.c
> +++ b/sound/synth/emux/emux.c
> @@ -131,10 +131,7 @@ int snd_emux_free(struct snd_emux *emu)
>   	if (! emu)
>   		return -EINVAL;
>   
> -	spin_lock_irqsave(&emu->voice_lock, flags);
> -	if (emu->timer_active)
> -		del_timer(&emu->tlist);
> -	spin_unlock_irqrestore(&emu->voice_lock, flags);
> +	del_timer_sync(&emu->tlist);
>   
>   	snd_emux_proc_free(emu);
>   	snd_emux_delete_virmidi(emu);