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

[Arnd Bergmann]

On Sun, Oct 10, 2021 at 9:55 AM Takashi Iwai <tiwai at 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 at mforney.org>
> Cc: <stable at vger.kernel.org>
> Cc: Arnd Bergmann <arnd at arndb.de>
> Cc: Rich Felker <dalias at libc.org>
> Link: https://lore.kernel.org/r/29QBMJU8DE71E.2YZSH8IHT5HMH@mforney.org
> Signed-off-by: Takashi Iwai <tiwai at suse.de>

Reviewed-by: Arnd Bergmann <arnd at 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?

       Arnd