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 for Android: free password hash cracker in your pocket

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

Date:   Thu, 27 Oct 2022 08:43:32 +0200
From:   Takashi Iwai <tiwai@...e.de>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Guenter Roeck <linux@...ck-us.net>,
        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 Thu, 27 Oct 2022 05:12:36 +0200,
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>

Applied now with the fix of unused variable warning.


Thanks!

Takashi


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