Re: [PATCH] ALSA: pcm: Workaround for a wrong offset in SYNC_PTR compat ioctl

[Takashi Iwai <tiwai@suse.de>]

On Sun, 10 Oct 2021 12:10:53 +0200,
Arnd Bergmann wrote:
> 
> On Sun, Oct 10, 2021 at 9:55 AM Takashi Iwai <tiwai@suse.de> wrote:
> >
> > Michael Forney reported an incorrect padding type that was defined in
> > the commit 80fe7430c708 ("ALSA: add new 32-bit layout for
> > snd_pcm_mmap_status/control") for PCM control mmap data.
> > His analysis is correct, and this caused the misplacements of PCM
> > control data on 32bit arch and 32bit compat mode.
> >
> > The bug is that the __pad2 definition in __snd_pcm_mmap_control64
> > struct was wrongly with __pad_before_uframe, which should have been
> > __pad_after_uframe instead.  This struct is used in SYNC_PTR ioctl and
> > control mmap.  Basically this bug leads to two problems:
> >
> > - The offset of avail_min field becomes wrong, it's placed right after
> >   appl_ptr without padding on little-endian
> >
> > - When appl_ptr and avail_min are read as 64bit values in kernel side,
> >   the values become either zero or corrupted (mixed up)
> >
> > One good news is that, because both user-space and kernel
> > misunderstand the wrong offset, at least, 32bit application running on
> > 32bit kernel works as is.  Also, 64bit applications are unaffected
> > because the padding size is zero.  The remaining problem is the 32bit
> > compat mode; as mentioned in the above, avail_min is placed right
> > after appl_ptr on little-endian archs, 64bit kernel reads bogus values
> > for appl_ptr updates, which may lead to streaming bugs like jumping,
> > XRUN or whatever unexpected.
> > (However, we haven't heard any serious bug reports due to this over
> > years, so practically seen, it's fairly safe to assume that the impact
> > by this bug is limited.)
> >
> > Ideally speaking, we should correct the wrong mmap status control
> > definition.  But this would cause again incompatibility with the
> > existing binaries, and fixing it (e.g. by renumbering ioctls) would be
> > really messy.
> >
> > So, as of this patch, we only correct the behavior of 32bit compat
> > mode and keep the rest as is.  Namely, the SYNC_PTR ioctl is now
> > handled differently in compat mode to read/write the 32bit values at
> > the right offsets.  The control mmap of 32bit apps on 64bit kernels
> > has been already disabled (which is likely rather an overlook, but
> > this worked fine at this time :), so covering SYNC_PTR ioctl should
> > suffice as a fallback.
> >
> > Fixes: 80fe7430c708 ("ALSA: add new 32-bit layout for snd_pcm_mmap_status/control")
> > Reported-by: Michael Forney <mforney@mforney.org>
> > Cc: <stable@vger.kernel.org>
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: Rich Felker <dalias@libc.org>
> > Link: https://lore.kernel.org/r/29QBMJU8DE71E.2YZSH8IHT5HMH@mforney.org
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> 
> Reviewed-by: Arnd Bergmann <arnd@arndb.de>
> 
> This does look like it's the best way we can get out of the mess we're
> in for the kernel interface.
> 
> Do you have any ideas for how to test this properly to ensure that
> there are no further problems in these ioctls? Is there a testsuite
> for ALSA that can be run on a musl-enabled distro in both native
> and compat mode to exercise the ioctl and mmap interfaces?

There is no known test program, and it's what I planned to check in
the next week before merging :)


thanks,

Takashi