diff mbox series

[v2] ALSA: pcm: oss: Fix race at SNDCTL_DSP_SETTRIGGER

Message ID 20230921064258.3582115-1-make_ruc2021@163.com (mailing list archive)
State Superseded
Headers show
Series [v2] ALSA: pcm: oss: Fix race at SNDCTL_DSP_SETTRIGGER | expand

Commit Message

Ma Ke Sept. 21, 2023, 6:42 a.m. UTC 
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@163.com>
---
 sound/core/oss/pcm_oss.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

Comments

Takashi Iwai Sept. 21, 2023, 1:29 p.m. UTC | #1 
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@163.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
>
kernel test robot Sept. 21, 2023, 3:20 p.m. UTC | #2 
Hi Ma,

kernel test robot noticed the following build warnings:

[auto build test WARNING on tiwai-sound/for-next]
[also build test WARNING on tiwai-sound/for-linus linus/master v6.6-rc2 next-20230921]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Ma-Ke/ALSA-pcm-oss-Fix-race-at-SNDCTL_DSP_SETTRIGGER/20230921-215828
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next
patch link:    https://lore.kernel.org/r/20230921064258.3582115-1-make_ruc2021%40163.com
patch subject: [PATCH v2] ALSA: pcm: oss: Fix race at SNDCTL_DSP_SETTRIGGER
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20230921/202309212328.2UOE4Raf-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230921/202309212328.2UOE4Raf-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309212328.2UOE4Raf-lkp@intel.com/

All warnings (new ones prefixed by >>):

   sound/core/oss/pcm_oss.c: In function 'snd_pcm_oss_set_trigger':
>> sound/core/oss/pcm_oss.c:2092:17: warning: this 'if' clause does not guard... [-Wmisleading-indentation]
    2092 |                 if (err < 0)
         |                 ^~
   sound/core/oss/pcm_oss.c:2094:25: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the 'if'
    2094 |                         return err;
         |                         ^~~~~~
   sound/core/oss/pcm_oss.c:2126:17: warning: this 'if' clause does not guard... [-Wmisleading-indentation]
    2126 |                 if (err < 0)
         |                 ^~
   sound/core/oss/pcm_oss.c:2128:25: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the 'if'
    2128 |                         return err;
         |                         ^~~~~~


vim +/if +2092 sound/core/oss/pcm_oss.c

  2072	
  2073	static int snd_pcm_oss_set_trigger(struct snd_pcm_oss_file *pcm_oss_file, int trigger)
  2074	{
  2075		struct snd_pcm_runtime *runtime;
  2076		struct snd_pcm_substream *psubstream = NULL, *csubstream = NULL;
  2077		int err, cmd;
  2078	
  2079	#ifdef OSS_DEBUG
  2080		pr_debug("pcm_oss: trigger = 0x%x\n", trigger);
  2081	#endif
  2082		
  2083		psubstream = pcm_oss_file->streams[SNDRV_PCM_STREAM_PLAYBACK];
  2084		csubstream = pcm_oss_file->streams[SNDRV_PCM_STREAM_CAPTURE];
  2085	
  2086	      	if (psubstream) {
  2087	      		runtime = psubstream->runtime;
  2088			cmd = 0;
  2089			if (mutex_lock_interruptible(&runtime->oss.params_lock))
  2090				return -ERESTARTSYS;
  2091			err = snd_pcm_oss_make_ready_locked(psubstream);
> 2092			if (err < 0)
  2093				mutex_unlock(&runtime->oss.params_lock);
  2094				return err;
  2095			if (trigger & PCM_ENABLE_OUTPUT) {
  2096				if (runtime->oss.trigger)
  2097					goto _skip1;
  2098				if (atomic_read(&psubstream->mmap_count))
  2099					snd_pcm_oss_simulate_fill(psubstream,
  2100							get_hw_ptr_period(runtime));
  2101				runtime->oss.trigger = 1;
  2102				runtime->start_threshold = 1;
  2103				cmd = SNDRV_PCM_IOCTL_START;
  2104			} else {
  2105				if (!runtime->oss.trigger)
  2106					goto _skip1;
  2107				runtime->oss.trigger = 0;
  2108				runtime->start_threshold = runtime->boundary;
  2109				cmd = SNDRV_PCM_IOCTL_DROP;
  2110				runtime->oss.prepare = 1;
  2111			}
  2112	 _skip1:
  2113			mutex_unlock(&runtime->oss.params_lock);
  2114			if (cmd) {
  2115				err = snd_pcm_kernel_ioctl(psubstream, cmd, NULL);
  2116				if (err < 0)
  2117					return err;
  2118			}
  2119		}
  2120		if (csubstream) {
  2121	      		runtime = csubstream->runtime;
  2122			cmd = 0;
  2123			if (mutex_lock_interruptible(&runtime->oss.params_lock))
  2124				return -ERESTARTSYS;
  2125			err = snd_pcm_oss_make_ready_locked(csubstream);
  2126			if (err < 0)
  2127				mutex_unlock(&runtime->oss.params_lock);
  2128				return err;
  2129			if (trigger & PCM_ENABLE_INPUT) {
  2130				if (runtime->oss.trigger)
  2131					goto _skip2;
  2132				runtime->oss.trigger = 1;
  2133				runtime->start_threshold = 1;
  2134				cmd = SNDRV_PCM_IOCTL_START;
  2135			} else {
  2136				if (!runtime->oss.trigger)
  2137					goto _skip2;
  2138				runtime->oss.trigger = 0;
  2139				runtime->start_threshold = runtime->boundary;
  2140				cmd = SNDRV_PCM_IOCTL_DROP;
  2141				runtime->oss.prepare = 1;
  2142			}
  2143	 _skip2:
  2144			mutex_unlock(&runtime->oss.params_lock);
  2145			if (cmd) {
  2146				err = snd_pcm_kernel_ioctl(csubstream, cmd, NULL);
  2147				if (err < 0)
  2148					return err;
  2149			}
  2150		}
  2151		return 0;
  2152	}
  2153
diff mbox series

Patch

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