[alsa-devel] [PATCH] ALSA: pcm: Fix possible inconsistent appl_ptr update via mmap

[Takashi Iwai]

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