On Sun, 10 Oct 2021 17:17:39 +0200,
Takashi Iwai wrote:
>
> On Sun, 10 Oct 2021 12:10:53 +0200,
> Arnd Bergmann wrote:
> >
> > On Sun, Oct 10, 2021 at 9:55 AM Takashi Iwai <tiwai@xxxxxxx> 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@xxxxxxxxxxx>
> > > Cc: <stable@xxxxxxxxxxxxxxx>
> > > Cc: Arnd Bergmann <arnd@xxxxxxxx>
> > > Cc: Rich Felker <dalias@xxxxxxxx>
> > > Link: https://lore.kernel.org/r/29QBMJU8DE71E.2YZSH8IHT5HMH@xxxxxxxxxxx
> > > Signed-off-by: Takashi Iwai <tiwai@xxxxxxx>
> >
> > Reviewed-by: Arnd Bergmann <arnd@xxxxxxxx>
> >
> > 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 :)
I checked a small code to write PCM data (that was used for testing
the low-latency I/O on USB-audio -- attached below) together with
salsa-lib [*] (devel branch, in which I hacked to enforce 64bit
timespec), and confirmed that 32bit compat mode indeed leading to the
bogus appl_ptr updates in the current kernel. And, my patch was
confirmed to work as well.
So the fix was queued to for-linus branch.
Takashi
[*] https://github.com/tiwai/salsa-lib
Attachment:
aiotest.c
Description: Binary data
