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

[Takashi Iwai]

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
>