Re: [Y2038] [musl] Re: [alsa-devel] [PATCH v7 8/9] ALSA: add new 32-bit layout for snd_pcm_mmap_status/control
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 wrote: > > > > > On Thu, 07 Oct 2021 13:48:44 +0200, Arnd Bergmann wrote: > > > > > > On Thu, Oct 7, 2021 at 12:53 PM Takashi Iwai 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
Re: [Y2038] [musl] Re: [alsa-devel] [PATCH v7 8/9] ALSA: add new 32-bit layout for snd_pcm_mmap_status/control
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 wrote: > > > > On Thu, 07 Oct 2021 13:48:44 +0200, Arnd Bergmann wrote: > > > > > On Thu, Oct 7, 2021 at 12:53 PM Takashi Iwai 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,
Re: [Y2038] [musl] Re: [alsa-devel] [PATCH v7 8/9] ALSA: add new 32-bit layout for snd_pcm_mmap_status/control
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 wrote: > > > On Thu, 07 Oct 2021 13:48:44 +0200, Arnd Bergmann wrote: > > > > On Thu, Oct 7, 2021 at 12:53 PM Takashi Iwai 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.
Re: [Y2038] [alsa-devel] [PATCH v7 8/9] ALSA: add new 32-bit layout for snd_pcm_mmap_status/control
On Thu, 07 Oct 2021 15:11:00 +0200, Arnd Bergmann wrote: > > On Thu, Oct 7, 2021 at 2:43 PM 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 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. Do we know how widely the __USE_TIME_BITS64 is deployed nowadays? Takashi ___ Y2038 mailing list Y2038@lists.linaro.org https://lists.linaro.org/mailman/listinfo/y2038
Re: [Y2038] [alsa-devel] [PATCH v7 8/9] ALSA: add new 32-bit layout for snd_pcm_mmap_status/control
On Thu, Oct 7, 2021 at 2:43 PM 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 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. Arnd [1] https://lore.kernel.org/all/29qbmju8de71e.2yzsh8iht5...@mforney.org/ ___ Y2038 mailing list Y2038@lists.linaro.org https://lists.linaro.org/mailman/listinfo/y2038
Re: [Y2038] [alsa-devel] [PATCH v7 8/9] ALSA: add new 32-bit layout for snd_pcm_mmap_status/control
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 wrote:
> > > On Wed, 06 Oct 2021 19:49:17 +0200, Michael Forney wrote:
> > > >
> > > > Arnd Bergmann 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_ */
> > > > > + __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
Re: [Y2038] [alsa-devel] [PATCH v7 8/9] ALSA: add new 32-bit layout for snd_pcm_mmap_status/control
On Thu, 07 Oct 2021 13:48:44 +0200,
Arnd Bergmann wrote:
>
> On Thu, Oct 7, 2021 at 12:53 PM Takashi Iwai wrote:
> > On Wed, 06 Oct 2021 19:49:17 +0200, Michael Forney wrote:
> > >
> > > Arnd Bergmann 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_
> > > > */
> > > > + __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.
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.
thanks,
Takashi
Re: [Y2038] [alsa-devel] [PATCH v7 8/9] ALSA: add new 32-bit layout for snd_pcm_mmap_status/control
On Thu, Oct 7, 2021 at 12:53 PM Takashi Iwai wrote:
> On Wed, 06 Oct 2021 19:49:17 +0200, Michael Forney wrote:
> >
> > Arnd Bergmann 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_ */
> > > + __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?
Arnd
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038
Re: [Y2038] [alsa-devel] [PATCH v7 8/9] ALSA: add new 32-bit layout for snd_pcm_mmap_status/control
On Wed, 06 Oct 2021 19:49:17 +0200,
Michael Forney wrote:
>
> Arnd Bergmann 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_ */
> > + __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).
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...
In anyway, care to send a formal fix patch?
Thanks for catching this!
Takashi
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038
