Re: [alsa-devel] [PATCH v7 8/9] ALSA: add new 32-bit layout for snd_pcm_mmap_status/control

[Takashi Iwai <tiwai@suse.de>]

On Thu, 07 Oct 2021 14:43:53 +0200,
Takashi Iwai 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:
> > > >
> > > > Arnd Bergmann <arnd@arndb.de> wrote:
> > > > > +#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];
> > > > > +#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)];
> > > > > +#endif
> > > > > +
> > > > > +struct __snd_pcm_mmap_status64 {
> > > > > +   __s32 state;                    /* RO: state - SNDRV_PCM_STATE_XXXX */
> > > > > +   __u32 pad1;                     /* Needed for 64 bit alignment */
> > > > > +   __pad_before_uframe __pad1;
> > > > > +   snd_pcm_uframes_t hw_ptr;       /* RO: hw ptr (0...boundary-1) */
> > > > > +   __pad_after_uframe __pad2;
> > > > > +   struct __snd_timespec64 tstamp; /* Timestamp */
> > > > > +   __s32 suspended_state;          /* RO: suspended stream state */
> > > > > +   __u32 pad3;                     /* Needed for 64 bit alignment */
> > > > > +   struct __snd_timespec64 audio_tstamp; /* sample counter or wall clock */
> > > > > +};
> > > > > +
> > > > > +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;
> > > >
> > > > I was looking through this header and happened to notice that this
> > > > padding is wrong. I believe it should be __pad_after_uframe here.
> > > >
> > > > I'm not sure of the implications of this typo, but I suspect it
> > > > breaks something on 32-bit systems with 64-bit time (regardless of
> > > > the endianness, since it changes the offset of avail_min).
> > 
> > Thanks a lot for the report! Yes, this is definitely broken in some ways.
> > 
> > > Right, that's the expected breakage.  It seems that the 64bit time on
> > > 32bit arch is still rare, so we haven't heard a regression by that, so
> > > far...
> > 
> > It might actually be worse: on a native 32-bit kernel, both user space
> > and kernel see the same broken definition with a 64-bit time_t, which
> > would end up actually making it work as expected. However, in
> > compat mode, the layout seen on the 32-bit user space is now
> > different from what the 64-bit kernel has, which would in turn not
> > work, in both the SNDRV_PCM_IOCTL_SYNC_PTR ioctl and in
> > the mmap() interface.
> > 
> > Fixing the layout to look like the way we had intended would make
> > newly compiled applications work in compat mode, but would break
> > applications built against the old header on new kernels and also
> > newly built applications on old kernels.
> > 
> > I still hope I missed something and it's not quite that bad, but I
> > fear the best we can do in this case make the broken interface
> > the normative one and fixing compat mode to write
> > mmap_control64->avail_min in the wrong location for
> > SNDRV_PCM_IOCTL_SYNC_PTR, as well as disabling
> > the mmap() interface again for compat tasks.
> >
> > 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.
> 
> 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.

Erm, scratch this part: on 64bit arch, both __pad_before_uframe and
__pad_after_uframe is 0-size, so the bug doesn't hit.  It's only about
32bit arch.

> 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.


Takashi