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

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

[Takashi Iwai]

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
> 

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

[kernel test robot]

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	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki