[alsa-devel] [PATCH] ALSA: pcm: Fix possible inconsistent appl_ptr update via mmap
The ALSA PCM core refers to the appl_ptr value stored on the mmapped page that is shared between kernel and user-space. Although the reference is performed in the PCM stream lock, it doesn't guarantee the atomic access when the value gets updated concurrently from the user-space on another CPU.
In most of codes, this is no big problem, but still there are a few places that may result in slight inconsistencies because they access runtime->control->appl_ptr multiple times; that is, the second read might be a different value from the first value. It can be even backward or jumping, as we have no control for it. Hence, the calculation may give an unexpected value. Luckily, there is no security vulnerability by that, as far as I've checked. But still we should address it.
This patch tries to reduce such possible cases. The fix is simple -- we just read once, store it to a local variable and use it for the rest calculations.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/core/pcm_lib.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index 631cd598ba6c..cda258d91490 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -65,15 +65,16 @@ void snd_pcm_playback_silence(struct snd_pcm_substream *substream, snd_pcm_ufram
if (runtime->silence_size < runtime->boundary) { snd_pcm_sframes_t noise_dist, n; - if (runtime->silence_start != runtime->control->appl_ptr) { - n = runtime->control->appl_ptr - runtime->silence_start; + snd_pcm_uframes_t appl_ptr = runtime->control->appl_ptr; + if (runtime->silence_start != appl_ptr) { + n = appl_ptr - runtime->silence_start; if (n < 0) n += runtime->boundary; if ((snd_pcm_uframes_t)n < runtime->silence_filled) runtime->silence_filled -= n; else runtime->silence_filled = 0; - runtime->silence_start = runtime->control->appl_ptr; + runtime->silence_start = appl_ptr; } if (runtime->silence_filled >= runtime->buffer_size) return; @@ -2224,7 +2225,9 @@ snd_pcm_sframes_t __snd_pcm_lib_xfer(struct snd_pcm_substream *substream, continue; /* draining */ } frames = size > avail ? avail : size; - cont = runtime->buffer_size - runtime->control->appl_ptr % runtime->buffer_size; + appl_ptr = runtime->control->appl_ptr; + appl_ofs = appl_ptr % runtime->buffer_size; + cont = runtime->buffer_size - appl_ofs; if (frames > cont) frames = cont; if (snd_BUG_ON(!frames)) { @@ -2232,8 +2235,6 @@ snd_pcm_sframes_t __snd_pcm_lib_xfer(struct snd_pcm_substream *substream, snd_pcm_stream_unlock_irq(substream); return -EINVAL; } - appl_ptr = runtime->control->appl_ptr; - appl_ofs = appl_ptr % runtime->buffer_size; snd_pcm_stream_unlock_irq(substream); err = writer(substream, appl_ofs, data, offset, frames, transfer);
Hi,
On Jun 17 2017 05:40, Takashi Iwai wrote:
The ALSA PCM core refers to the appl_ptr value stored on the mmapped page that is shared between kernel and user-space. Although the reference is performed in the PCM stream lock, it doesn't guarantee the atomic access when the value gets updated concurrently from the user-space on another CPU.
In most of codes, this is no big problem, but still there are a few places that may result in slight inconsistencies because they access runtime->control->appl_ptr multiple times; that is, the second read might be a different value from the first value. It can be even backward or jumping, as we have no control for it. Hence, the calculation may give an unexpected value. Luckily, there is no security vulnerability by that, as far as I've checked. But still we should address it.
This patch tries to reduce such possible cases. The fix is simple -- we just read once, store it to a local variable and use it for the rest calculations.
Signed-off-by: Takashi Iwai tiwai@suse.de
sound/core/pcm_lib.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
This looks good to me by itself.
Reviewed-by: Takashi Sakamoto o-takashi@sakamocchi.jp
This function, 'snd_pcm_playback_silence()' can be called without acquiring stream lock by 'snd_pcm_reset()' when runtime of PCM substream is configured to have a larger value than zero to its 'silence_size' member. When a task or any IRQ handler operates the runtime and another task operates the reset in the same time, both of them got expected result.
I'll post a patch to take 'snd_pcm_reset()' to use 'snd_pcm_action_lock_irq()' instead of 'snd_pcm_action_nonatomic()'. How do you think about it?
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index 631cd598ba6c..cda258d91490 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -65,15 +65,16 @@ void snd_pcm_playback_silence(struct snd_pcm_substream *substream, snd_pcm_ufram
if (runtime->silence_size < runtime->boundary) { snd_pcm_sframes_t noise_dist, n;
if (runtime->silence_start != runtime->control->appl_ptr) {n = runtime->control->appl_ptr - runtime->silence_start;
snd_pcm_uframes_t appl_ptr = runtime->control->appl_ptr;if (runtime->silence_start != appl_ptr) {n = appl_ptr - runtime->silence_start; if (n < 0) n += runtime->boundary; if ((snd_pcm_uframes_t)n < runtime->silence_filled) runtime->silence_filled -= n; else runtime->silence_filled = 0;
runtime->silence_start = runtime->control->appl_ptr;
runtime->silence_start = appl_ptr;@@ -2224,7 +2225,9 @@ snd_pcm_sframes_t __snd_pcm_lib_xfer(struct snd_pcm_substream *substream, continue; /* draining */ } frames = size > avail ? avail : size;
cont = runtime->buffer_size - runtime->control->appl_ptr % runtime->buffer_size;
appl_ptr = runtime->control->appl_ptr;appl_ofs = appl_ptr % runtime->buffer_size;cont = runtime->buffer_size - appl_ofs;@@ -2232,8 +2235,6 @@ snd_pcm_sframes_t __snd_pcm_lib_xfer(struct snd_pcm_substream *substream, snd_pcm_stream_unlock_irq(substream); return -EINVAL; }
appl_ptr = runtime->control->appl_ptr;appl_ofs = appl_ptr % runtime->buffer_size;
Regards
Takashi Sakamoto
On Sun, 18 Jun 2017 12:49:24 +0200, Takashi Sakamoto wrote:
Hi,
On Jun 17 2017 05:40, Takashi Iwai wrote:
The ALSA PCM core refers to the appl_ptr value stored on the mmapped page that is shared between kernel and user-space. Although the reference is performed in the PCM stream lock, it doesn't guarantee the atomic access when the value gets updated concurrently from the user-space on another CPU.
In most of codes, this is no big problem, but still there are a few places that may result in slight inconsistencies because they access runtime->control->appl_ptr multiple times; that is, the second read might be a different value from the first value. It can be even backward or jumping, as we have no control for it. Hence, the calculation may give an unexpected value. Luckily, there is no security vulnerability by that, as far as I've checked. But still we should address it.
This patch tries to reduce such possible cases. The fix is simple -- we just read once, store it to a local variable and use it for the rest calculations.
Signed-off-by: Takashi Iwai tiwai@suse.de
sound/core/pcm_lib.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
This looks good to me by itself.
Reviewed-by: Takashi Sakamoto o-takashi@sakamocchi.jp
This function, 'snd_pcm_playback_silence()' can be called without acquiring stream lock by 'snd_pcm_reset()' when runtime of PCM substream is configured to have a larger value than zero to its 'silence_size' member. When a task or any IRQ handler operates the runtime and another task operates the reset in the same time, both of them got expected result.
Good catch. The whole snd_pcm_reset() behavior is a bit whacky.
I'll post a patch to take 'snd_pcm_reset()' to use 'snd_pcm_action_lock_irq()' instead of 'snd_pcm_action_nonatomic()'. How do you think about it?
Conceptually seen, that's not right. snd_pcm_reset() calls PCM ioctl ops, and this is supposed to be always non-atomic (it's a kind of ioctl wrapper, after all). So the correct fix would be simply to wrap only the snd_pcm_playback_silence() call with the stream lock.
thanks,
Takashi
On Jun 19 2017 02:23, Takashi Iwai wrote:
I'll post a patch to take 'snd_pcm_reset()' to use 'snd_pcm_action_lock_irq()' instead of 'snd_pcm_action_nonatomic()'. How do you think about it?
Conceptually seen, that's not right. snd_pcm_reset() calls PCM ioctl ops, and this is supposed to be always non-atomic (it's a kind of ioctl wrapper, after all). So the correct fix would be simply to wrap only the snd_pcm_playback_silence() call with the stream lock.
I once investigated for this idea. But I was discouraged because of several resons:
1. There's a semantic contradiction. 'snd_pcm_action_nonatomic()' includes atomic operation. The idea perhaps puzzles developers. 2. In 'snd_pcm_pre_reset()', 'snd_pcm_do_reset()' and 'snd_pcm_post_reset()', parameters of runtime for PCM substream are also changed or referred. They can be changed in other functions concurrently, thus should be locked. 3. Currently there're a few drivers which implements the local ioctl. All of them handle the callback without calling task scheduler. It's reasonable against the cost to have an additional restraint just for context with the reset command.
Well, this is a revised version.
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 7e8f3656b695..c74bee099155 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -1593,7 +1593,11 @@ static int snd_pcm_pre_reset(struct snd_pcm_substream *substream, int state) static int snd_pcm_do_reset(struct snd_pcm_substream *substream, int state) { struct snd_pcm_runtime *runtime = substream->runtime; - int err = substream->ops->ioctl(substream, SNDRV_PCM_IOCTL1_RESET, NULL); + int err; + + snd_pcm_stream_unlock_irq(substream); + err = substream->ops->ioctl(substream, SNDRV_PCM_IOCTL1_RESET, NULL); + snd_pcm_stream_lock_irq(substream); if (err < 0) return err; runtime->hw_ptr_base = 0; @@ -1621,7 +1625,7 @@ static const struct action_ops snd_pcm_action_reset = {
static int snd_pcm_reset(struct snd_pcm_substream *substream) { - return snd_pcm_action_nonatomic(&snd_pcm_action_reset, substream, 0); + return snd_pcm_action_lock_irq(&snd_pcm_action_reset, substream, 0); }
/*
There's still a contradiction about atomicity but as a whole better than before.
Regards
Takashi Sakamoto
On Mon, 19 Jun 2017 14:10:00 +0200, Takashi Sakamoto wrote:
On Jun 19 2017 02:23, Takashi Iwai wrote:
I'll post a patch to take 'snd_pcm_reset()' to use 'snd_pcm_action_lock_irq()' instead of 'snd_pcm_action_nonatomic()'. How do you think about it?
Conceptually seen, that's not right. snd_pcm_reset() calls PCM ioctl ops, and this is supposed to be always non-atomic (it's a kind of ioctl wrapper, after all). So the correct fix would be simply to wrap only the snd_pcm_playback_silence() call with the stream lock.
I once investigated for this idea. But I was discouraged because of several resons:
- There's a semantic contradiction. 'snd_pcm_action_nonatomic()'
includes atomic operation. The idea perhaps puzzles developers.
Oh no, that's a big misunderstanding. snd_pcm_action_nonatomic() doesn't mean to prohibit the critical section protected via a spinlock inside its execution code. Instead, it's the code you can't call *from* spinlocked context.
- In 'snd_pcm_pre_reset()', 'snd_pcm_do_reset()' and
'snd_pcm_post_reset()', parameters of runtime for PCM substream are also changed or referred. They can be changed in other functions concurrently, thus should be locked.
They are protected in terms of the mutex.
- Currently there're a few drivers which implements the local
ioctl. All of them handle the callback without calling task scheduler. It's reasonable against the cost to have an additional restraint just for context with the reset command.
But the local ioctl *is* always non-atomic, that's the reason I wrote "conceptually seen". If you don't want it to be non-atomic, you'd need to introduce yet another ops for doing that.
Well, this is a revised version.
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 7e8f3656b695..c74bee099155 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -1593,7 +1593,11 @@ static int snd_pcm_pre_reset(struct snd_pcm_substream *substream, int state) static int snd_pcm_do_reset(struct snd_pcm_substream *substream, int state) { struct snd_pcm_runtime *runtime = substream->runtime;
int err = substream->ops->ioctl(substream,SNDRV_PCM_IOCTL1_RESET, NULL);
int err;snd_pcm_stream_unlock_irq(substream);err = substream->ops->ioctl(substream, SNDRV_PCM_IOCTL1_RESET,NULL);
snd_pcm_stream_lock_irq(substream); if (err < 0) return err; runtime->hw_ptr_base = 0;@@ -1621,7 +1625,7 @@ static const struct action_ops snd_pcm_action_reset = {
static int snd_pcm_reset(struct snd_pcm_substream *substream) {
return snd_pcm_action_nonatomic(&snd_pcm_action_reset,substream, 0);
return snd_pcm_action_lock_irq(&snd_pcm_action_reset, substream, 0);}
/*
There's still a contradiction about atomicity but as a whole better than before.
No, this code is just wrong... The PCM locking doesn't work like that.
Imagine multiple PCM streams are linked. Then you'll be screwed up.
thanks,
Takashi
Takashi Iwai wrote:
The ALSA PCM core refers to the appl_ptr value stored on the mmapped page that is shared between kernel and user-space. Although the reference is performed in the PCM stream lock, it doesn't guarantee the atomic access when the value gets updated concurrently from the user-space on another CPU.
In most of codes, this is no big problem, but still there are a few places that may result in slight inconsistencies because they access runtime->control->appl_ptr multiple times; that is, the second read might be a different value from the first value. It can be even backward or jumping, as we have no control for it. Hence, the calculation may give an unexpected value. Luckily, there is no security vulnerability by that, as far as I've checked. But still we should address it.
This patch tries to reduce such possible cases. The fix is simple -- we just read once, store it to a local variable and use it for the rest calculations.
It is likely that the compiler's optimizer already merged multiple accesses and used a temporary register.
However, if the compiler can do this transformation, it can also do the reverse transformation. So this new code does not actually guarantee that there is a single atomic access. That can be enforced only with READ_ONCE():
if (runtime->silence_size < runtime->boundary) { snd_pcm_sframes_t noise_dist, n;
if (runtime->silence_start != runtime->control->appl_ptr) {n = runtime->control->appl_ptr - runtime->silence_start;
snd_pcm_uframes_t appl_ptr = runtime->control->appl_ptr;
snd_pcm_uframes_t appl_ptr = READ_ONCE(runtime->control->appl_ptr); etc.
Regards, Clemens
On Sun, 18 Jun 2017 20:03:34 +0200, Clemens Ladisch wrote:
Takashi Iwai wrote:
The ALSA PCM core refers to the appl_ptr value stored on the mmapped page that is shared between kernel and user-space. Although the reference is performed in the PCM stream lock, it doesn't guarantee the atomic access when the value gets updated concurrently from the user-space on another CPU.
In most of codes, this is no big problem, but still there are a few places that may result in slight inconsistencies because they access runtime->control->appl_ptr multiple times; that is, the second read might be a different value from the first value. It can be even backward or jumping, as we have no control for it. Hence, the calculation may give an unexpected value. Luckily, there is no security vulnerability by that, as far as I've checked. But still we should address it.
This patch tries to reduce such possible cases. The fix is simple -- we just read once, store it to a local variable and use it for the rest calculations.
It is likely that the compiler's optimizer already merged multiple accesses and used a temporary register.
However, if the compiler can do this transformation, it can also do the reverse transformation. So this new code does not actually guarantee that there is a single atomic access. That can be enforced only with READ_ONCE():
Good point, we need to assure by that. Will respin the patch.
thanks,
Takashi
if (runtime->silence_size < runtime->boundary) { snd_pcm_sframes_t noise_dist, n;
if (runtime->silence_start != runtime->control->appl_ptr) {n = runtime->control->appl_ptr - runtime->silence_start;
snd_pcm_uframes_t appl_ptr = runtime->control->appl_ptr;snd_pcm_uframes_t appl_ptr = READ_ONCE(runtime->control->appl_ptr);etc.
Regards, Clemens
participants (3)
-
Clemens Ladisch -
Takashi Iwai -
Takashi Sakamoto