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