On Thu, 07 Oct 2021 18:51:58 +0200, Rich Felker wrote: > > On Thu, Oct 07, 2021 at 06:18:52PM +0200, Takashi Iwai wrote: > > On Thu, 07 Oct 2021 18:06:36 +0200, > > Rich Felker wrote: > > > > > > On Thu, Oct 07, 2021 at 05:33:19PM +0200, Takashi Iwai wrote: > > > > On Thu, 07 Oct 2021 15:11:00 +0200, > > > > Arnd Bergmann wrote: > > > > > > > > > > On Thu, Oct 7, 2021 at 2:43 PM Takashi Iwai <tiwai@xxxxxxx> wrote: > > > > > > On Thu, 07 Oct 2021 13:48:44 +0200, Arnd Bergmann wrote: > > > > > > > On Thu, Oct 7, 2021 at 12:53 PM Takashi Iwai <tiwai@xxxxxxx> wrote: > > > > > > > > On Wed, 06 Oct 2021 19:49:17 +0200, Michael Forney wrote: > > > > > > > > > > > > > > As far as I can tell, the broken interface will always result in > > > > > > > user space seeing a zero value for "avail_min". Can you > > > > > > > make a prediction what that would mean for actual > > > > > > > applications? Will they have no audio output, run into > > > > > > > a crash, or be able to use recover and appear to work normally > > > > > > > here? > > > > > > > > > > > > No, fortunately it's only about control->avail_min, and fiddling this > > > > > > value can't break severely (otherwise it'd be a security problem ;) > > > > > > > > > > > > In the buggy condition, it's always zero, and the kernel treated as if > > > > > > 1, i.e. wake up as soon as data is available, which is OK-ish for most > > > > > > applications. Apps usually don't care about the wake-up condition so > > > > > > much. There are subtle difference and may influence on the stability > > > > > > of stream processing, but the stability usually depends more strongly > > > > > > on the hardware and software configurations. > > > > > > > > > > > > That being said, the impact by this bug (from the application behavior > > > > > > POV) is likely quite small, but the contamination is large; as you > > > > > > pointed out, it's much larger than I thought. > > > > > > > > > > Ok, got it. > > > > > > > > > > > The definition in uapi/sound/asound.h is a bit cryptic, but IIUC, > > > > > > __snd_pcm_mmap_control64 is used for 64bit archs, right? If so, the > > > > > > problem rather hits more widely on 64bit archs silently. Then, the > > > > > > influence by this bug must be almost negligible, as we've had no bug > > > > > > report about the behavior change. > > > > > > > > > > While __snd_pcm_mmap_control64 is only used on 32-bit > > > > > architectures when 64-bit time_t is used. At the moment, this > > > > > means all users of musl-1.2.x libc, but not glibc. > > > > > > > > > > On 64-bit architectures, __snd_pcm_mmap_control and > > > > > __snd_pcm_mmap_control64 are meant to be identical, > > > > > and this is actually true regardless of the bug, since > > > > > __pad_before_uframe and __pad_after_uframe both > > > > > end up as zero-length arrays here. > > > > > > > > > > > We may just fix it in kernel and for new library with hoping that no > > > > > > one sees the actual problem. Or, we may provide a complete new set of > > > > > > mmap offsets and ioctl to cover both broken and fixed interfaces... > > > > > > The decision depends on how perfectly we'd like to address the bug. > > > > > > As of now, I'm inclined to go for the former, but I'm open for more > > > > > > opinions. > > > > > > > > > > Adding the musl list to Cc for additional testers, anyone interested > > > > > please see [1] for the original report. > > > > > > > > > > It would be good to hear from musl users that are already using > > > > > audio support with 32-bit applications on 64-bit kernels, which > > > > > is the case that has the problem today. Have you noticed any > > > > > problems with audio support here? If not, we can probably > > > > > "fix" the kernel here and make the existing binaries behave > > > > > the same way on 32-bit kernels. If there are applications that > > > > > don't work in that environment today, I think we need to instead > > > > > change the kernel to accept the currently broken format on > > > > > both 32-bit and 64-bit kernels, possibly introducing yet another > > > > > format that works as originally intended but requires a newly > > > > > built kernel. > > > > > > > > Thanks! > > > > > > > > And now, looking more deeply, I feel more desperate. > > > > > > > > This bug makes the expected padding gone on little-endian. > > > > On LE 32bit, the buggy definition is: > > > > > > > > char __pad1[0]; > > > > u32 appl_ptr; > > > > char __pad2[0]; // this should have been [4] > > > > char __pad3[0]; > > > > u32 avail_min; > > > > char __pad4[4]; > > > > > > > > When an application issues SYNC_PTR64 ioctl to submit appl_ptr and > > > > avail_min updates, 64bit kernel (in compat mode) reads directly as: > > > > > > > > u64 appl_ptr; > > > > u64 avail_min; > > > > > > > > Hence a bogus appl_ptr would be passed if avail_min != 0. > > > > And usually application sets non-zero avail_min. > > > > That is, the bug must hit more severely if the new API were really > > > > used. It wouldn't crash, but some weird streaming behavior can > > > > happen like noise, jumping or underruns. > > > > > > > > (Reading back avail_min=0 to user-space is rather harmless. Ditto for > > > > the case of BE, then at least there is no appl_ptr corruption.) > > > > > > > > This made me wonder which way to go: > > > > it's certainly possible to fix the new kernel to treat both buggy and > > > > sane formats (disabling compat mmap and re-define ioctls, having the > > > > code for old APIs). The problem is, however, in the case where the > > > > application needs to run on the older kernel that expects the buggy > > > > format. Then apps would still have to send in the old buggy format -- > > > > or maybe better in the older 32bit format that won't hit the bug > > > > above. It makes situation more complicated. > > > > > > Can't an ioctl number just be redefined so that, on old kernels with > > > the buggy one, newly built applications get told that mmap is not > > > available and use the unaffected non-mmap fallback? > > > > The problem is that the SYNC_PTR64 ioctl itself for non-mmap fallback > > is equally buggy due to this bug, too. So disabling mmap doesn't help > > alone. > > > > And, yes, we can redefine ioctl numbers. But, then, application would > > have to be bilingual, as well as the kernel; it'll have to switch back > > to old API when running on older kernel, while the same binary would > > need to run in a new API for a newer kernel. > > > > Maybe we can implement it in alsa-lib, if it really worth for it. > > In musl we already have ioctl struct conversion for running on > time32-only kernels. So it may be practical to convert this too if > needed. I guess we can work around without ioctl renumbering. The PCM API has a protocol version handshaking, and user-space is supposed to tell its API version to kernel. So the kernel can know in what version user-space is talking with. Below is the PoC fix in the kernel side (totally untested). The fix for alsa-lib will follow. thanks, Takashi --- diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h index 5859ca0a1439..dbdbf0c794d8 100644 --- a/include/uapi/sound/asound.h +++ b/include/uapi/sound/asound.h @@ -156,7 +156,7 @@ struct snd_hwdep_dsp_image { * * *****************************************************************************/ -#define SNDRV_PCM_VERSION SNDRV_PROTOCOL_VERSION(2, 0, 15) +#define SNDRV_PCM_VERSION SNDRV_PROTOCOL_VERSION(2, 0, 16) typedef unsigned long snd_pcm_uframes_t; typedef signed long snd_pcm_sframes_t; @@ -550,6 +550,7 @@ struct __snd_pcm_sync_ptr { } s; union { struct __snd_pcm_mmap_control control; + struct __snd_pcm_mmap_control control_api_2_0_15; /* no bug in 32bit mode */ unsigned char reserved[64]; } c; }; @@ -557,11 +558,15 @@ struct __snd_pcm_sync_ptr { #if defined(__BYTE_ORDER) ? __BYTE_ORDER == __BIG_ENDIAN : defined(__BIG_ENDIAN) typedef char __pad_before_uframe[sizeof(__u64) - sizeof(snd_pcm_uframes_t)]; typedef char __pad_after_uframe[0]; +typedef char __pad_before_u32[4]; +typedef char __pad_after_u32[0]; #endif #if defined(__BYTE_ORDER) ? __BYTE_ORDER == __LITTLE_ENDIAN : defined(__LITTLE_ENDIAN) typedef char __pad_before_uframe[0]; typedef char __pad_after_uframe[sizeof(__u64) - sizeof(snd_pcm_uframes_t)]; +typedef char __pad_before_u32[0]; +typedef char __pad_after_u32[4]; #endif struct __snd_pcm_mmap_status64 { @@ -579,13 +584,23 @@ struct __snd_pcm_mmap_status64 { struct __snd_pcm_mmap_control64 { __pad_before_uframe __pad1; snd_pcm_uframes_t appl_ptr; /* RW: appl ptr (0...boundary-1) */ - __pad_before_uframe __pad2; + __pad_after_uframe __pad2; __pad_before_uframe __pad3; snd_pcm_uframes_t avail_min; /* RW: min available frames for wakeup */ __pad_after_uframe __pad4; }; +/* buggy mmap control definition for 2.0.15 PCM API on 32bit mode */ +struct __snd_pcm_mmap_control64_api_2_0_15 { + __pad_before_u32 __pad1; + __u32 appl_ptr; + __pad_before_u32 __pad2; /* SiC! here is the bug */ + __pad_before_u32 __pad3; + __u32 avail_min; + __pad_after_uframe __pad4; +}; + struct __snd_pcm_sync_ptr64 { __u32 flags; __u32 pad1; @@ -595,6 +610,7 @@ struct __snd_pcm_sync_ptr64 { } s; union { struct __snd_pcm_mmap_control64 control; + struct __snd_pcm_mmap_control64_api_2_0_15 control_api_2_0_15; unsigned char reserved[64]; } c; }; diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 46c643db18eb..1f26dd2a2525 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -2958,8 +2958,19 @@ static int snd_pcm_delay(struct snd_pcm_substream *substream, return err; } +/* PCM 2.0.15 API definition had a bug in mmap control; it puts the avail_min + * at the wrong offset due to a typo in padding type. + * The bug hits only on 32bit, either on 32bit arch or in 32bit compat mode. + */ +static bool is_buggy_control(struct snd_pcm_file *pcm_file) +{ + return pcm_file->user_pversion == SNDRV_PROTOCOL_VERSION(2, 0, 15) && + (in_compat_syscall() || !IS_ENABLED(CONFIG_64BIT)); +} + static int snd_pcm_sync_ptr(struct snd_pcm_substream *substream, - struct snd_pcm_sync_ptr __user *_sync_ptr) + struct snd_pcm_sync_ptr __user *_sync_ptr, + bool buggy_control) { struct snd_pcm_runtime *runtime = substream->runtime; struct snd_pcm_sync_ptr sync_ptr; @@ -2970,8 +2981,17 @@ static int snd_pcm_sync_ptr(struct snd_pcm_substream *substream, memset(&sync_ptr, 0, sizeof(sync_ptr)); if (get_user(sync_ptr.flags, (unsigned __user *)&(_sync_ptr->flags))) return -EFAULT; - if (copy_from_user(&sync_ptr.c.control, &(_sync_ptr->c.control), sizeof(struct snd_pcm_mmap_control))) - return -EFAULT; + if (buggy_control) { + if (copy_from_user(&sync_ptr.c.control_api_2_0_15, + &(_sync_ptr->c.control_api_2_0_15), + sizeof(sync_ptr.c.control_api_2_0_15))) + return -EFAULT; + } else { + if (copy_from_user(&sync_ptr.c.control, + &(_sync_ptr->c.control), + sizeof(sync_ptr.c.control))) + return -EFAULT; + } status = runtime->status; control = runtime->control; if (sync_ptr.flags & SNDRV_PCM_SYNC_PTR_HWSYNC) { @@ -2981,19 +3001,34 @@ static int snd_pcm_sync_ptr(struct snd_pcm_substream *substream, } snd_pcm_stream_lock_irq(substream); if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_APPL)) { - err = pcm_lib_apply_appl_ptr(substream, - sync_ptr.c.control.appl_ptr); + if (buggy_control) { + err = pcm_lib_apply_appl_ptr(substream, + sync_ptr.c.control_api_2_0_15.appl_ptr); + } else { + err = pcm_lib_apply_appl_ptr(substream, + sync_ptr.c.control.appl_ptr); + } if (err < 0) { snd_pcm_stream_unlock_irq(substream); return err; } } else { - sync_ptr.c.control.appl_ptr = control->appl_ptr; + if (buggy_control) + sync_ptr.c.control_api_2_0_15.appl_ptr = control->appl_ptr; + else + sync_ptr.c.control.appl_ptr = control->appl_ptr; + } + if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_AVAIL_MIN)) { + if (buggy_control) + control->avail_min = sync_ptr.c.control_api_2_0_15.avail_min; + else + control->avail_min = sync_ptr.c.control.avail_min; + } else { + if (buggy_control) + sync_ptr.c.control_api_2_0_15.avail_min = control->avail_min; + else + sync_ptr.c.control.avail_min = control->avail_min; } - if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_AVAIL_MIN)) - control->avail_min = sync_ptr.c.control.avail_min; - else - sync_ptr.c.control.avail_min = control->avail_min; sync_ptr.s.status.state = status->state; sync_ptr.s.status.hw_ptr = status->hw_ptr; sync_ptr.s.status.tstamp = status->tstamp; @@ -3289,7 +3324,8 @@ static int snd_pcm_common_ioctl(struct file *file, case __SNDRV_PCM_IOCTL_SYNC_PTR32: return snd_pcm_ioctl_sync_ptr_compat(substream, arg); case __SNDRV_PCM_IOCTL_SYNC_PTR64: - return snd_pcm_sync_ptr(substream, arg); + return snd_pcm_sync_ptr(substream, arg, + is_buggy_control(pcm_file)); #ifdef CONFIG_SND_SUPPORT_OLD_API case SNDRV_PCM_IOCTL_HW_REFINE_OLD: return snd_pcm_hw_refine_old_user(substream, arg); @@ -3851,6 +3887,8 @@ static int snd_pcm_mmap(struct file *file, struct vm_area_struct *area) return -ENXIO; fallthrough; case SNDRV_PCM_MMAP_OFFSET_CONTROL_NEW: + if (is_buggy_control(pcm_file)) + return -ENXIO; if (!pcm_control_mmap_allowed(pcm_file)) return -ENXIO; return snd_pcm_mmap_control(substream, file, area);