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@suse.de 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@suse.de 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);