* [PATCH] 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.
@ 2023-09-20 2:35 Ma Ke
2023-09-20 7:05 ` Takashi Iwai
0 siblings, 1 reply; 2+ messages in thread
From: Ma Ke @ 2023-09-20 2:35 UTC (permalink / raw)
To: perex, tiwai, mhocko, akpm, make_ruc2021, mgorman, 42.hyeyoo,
surenb, alsa-devel
Cc: linux-kernel
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 | 16 ++++++----------
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/sound/core/oss/pcm_oss.c b/sound/core/oss/pcm_oss.c
index 728c211142d1..9a830aeeba63 100644
--- a/sound/core/oss/pcm_oss.c
+++ b/sound/core/oss/pcm_oss.c
@@ -2083,21 +2083,14 @@ 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)
+ goto _skip1;
if (trigger & PCM_ENABLE_OUTPUT) {
if (runtime->oss.trigger)
goto _skip1;
@@ -2128,6 +2121,9 @@ 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)
+ goto _skip2;
if (trigger & PCM_ENABLE_INPUT) {
if (runtime->oss.trigger)
goto _skip2;
--
2.37.2
^ permalink raw reply related [flat|nested] 2+ messages in thread* Re: [PATCH] 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.
2023-09-20 2:35 [PATCH] 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 Ma Ke
@ 2023-09-20 7:05 ` Takashi Iwai
0 siblings, 0 replies; 2+ messages in thread
From: Takashi Iwai @ 2023-09-20 7:05 UTC (permalink / raw)
To: Ma Ke
Cc: perex, tiwai, mhocko, akpm, mgorman, 42.hyeyoo, surenb,
alsa-devel, linux-kernel
On Wed, 20 Sep 2023 04:35:36 +0200,
Ma Ke wrote:
>
> 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>
The patch subject needs to be fixed. The subject line can't be that
long. It must be concise. Put more text in the patch description
instead.
About the code change: the error handling is different after the
patch. The current code returns an error immediately while your patch
does "goto _skip1" etc, which doesn't abort but continues.
thanks,
Takashi
> ---
> sound/core/oss/pcm_oss.c | 16 ++++++----------
> 1 file changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/sound/core/oss/pcm_oss.c b/sound/core/oss/pcm_oss.c
> index 728c211142d1..9a830aeeba63 100644
> --- a/sound/core/oss/pcm_oss.c
> +++ b/sound/core/oss/pcm_oss.c
> @@ -2083,21 +2083,14 @@ 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)
> + goto _skip1;
> if (trigger & PCM_ENABLE_OUTPUT) {
> if (runtime->oss.trigger)
> goto _skip1;
> @@ -2128,6 +2121,9 @@ 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)
> + goto _skip2;
> if (trigger & PCM_ENABLE_INPUT) {
> if (runtime->oss.trigger)
> goto _skip2;
> --
> 2.37.2
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2023-09-21 13:24 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-09-20 2:35 [PATCH] 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 Ma Ke
2023-09-20 7:05 ` Takashi Iwai
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).