linux-kernel - Re: [PATCH v2] ALSA: pcm: oss: Fix race at SNDCTL_DSP

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

Message-ID: <87il83zmbs.wl-tiwai@suse.de>
Date:   Thu, 21 Sep 2023 15:29:27 +0200
From:   Takashi Iwai <tiwai@...e.de>
To:     Ma Ke <make_ruc2021@....com>
Cc:     perex@...ex.cz, tiwai@...e.com, Liam.Howlett@...cle.com,
        rppt@...nel.org, mgorman@...hsingularity.net, mhocko@...e.com,
        surenb@...gle.com, alsa-devel@...a-project.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] ALSA: pcm: oss: Fix race at SNDCTL_DSP_SETTRIGGER

On Thu, 21 Sep 2023 08:42:58 +0200,
Ma Ke wrote:
> 
> There is a small race window at snd_pcm_oss_set_trigger() that is
> called from OSS PCM SNDCTL_DSP_SETTRIGGER ioctl; namely the function
> calls snd_pcm_oss_make_ready() at first, then takes the params_lock
> mutex for the rest. When the stream is set up again by another thread
> between them, it leads to inconsistency, and may result in unexpected
> results such as NULL dereference of OSS buffer as a fuzzer spotted
> recently.
> The fix is simply to cover snd_pcm_oss_make_ready() call into the same
> params_lock mutex with snd_pcm_oss_make_ready_locked() variant.
> 
> Signed-off-by: Ma Ke <make_ruc2021@....com>
> ---
>  sound/core/oss/pcm_oss.c | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/sound/core/oss/pcm_oss.c b/sound/core/oss/pcm_oss.c
> index 728c211142d1..f6340a2fe52b 100644
> --- a/sound/core/oss/pcm_oss.c
> +++ b/sound/core/oss/pcm_oss.c
> @@ -2083,21 +2083,15 @@ static int snd_pcm_oss_set_trigger(struct snd_pcm_oss_file *pcm_oss_file, int tr
>  	psubstream = pcm_oss_file->streams[SNDRV_PCM_STREAM_PLAYBACK];
>  	csubstream = pcm_oss_file->streams[SNDRV_PCM_STREAM_CAPTURE];
>  
> -	if (psubstream) {
> -		err = snd_pcm_oss_make_ready(psubstream);
> -		if (err < 0)
> -			return err;
> -	}
> -	if (csubstream) {
> -		err = snd_pcm_oss_make_ready(csubstream);
> -		if (err < 0)
> -			return err;
> -	}
>        	if (psubstream) {
>        		runtime = psubstream->runtime;
>  		cmd = 0;
>  		if (mutex_lock_interruptible(&runtime->oss.params_lock))
>  			return -ERESTARTSYS;
> +		err = snd_pcm_oss_make_ready_locked(psubstream);
> +		if (err < 0)
> +			mutex_unlock(&runtime->oss.params_lock);
> +			return err;

This breaks totally;  you missed braces...  (Ditto for another place).


Takashi

>  		if (trigger & PCM_ENABLE_OUTPUT) {
>  			if (runtime->oss.trigger)
>  				goto _skip1;
> @@ -2128,6 +2122,10 @@ static int snd_pcm_oss_set_trigger(struct snd_pcm_oss_file *pcm_oss_file, int tr
>  		cmd = 0;
>  		if (mutex_lock_interruptible(&runtime->oss.params_lock))
>  			return -ERESTARTSYS;
> +		err = snd_pcm_oss_make_ready_locked(csubstream);
> +		if (err < 0)
> +			mutex_unlock(&runtime->oss.params_lock);
> +			return err;
>  		if (trigger & PCM_ENABLE_INPUT) {
>  			if (runtime->oss.trigger)
>  				goto _skip2;
> -- 
> 2.37.2
>