[PATCH v2] ALSA: pcm: oss: Fix race at SNDCTL_DSP_SETTRIGGER
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; 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;
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;-- 2.37.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-a... 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@i...) 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@i...)
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
participants (3)
-
kernel test robot -
Ma Ke -
Takashi Iwai