mailing list of musl libc
 help / color / mirror / code / Atom feed
* [musl] Re: [alsa-devel] [PATCH v7 8/9] ALSA: add new 32-bit layout for snd_pcm_mmap_status/control
       [not found]         ` <s5hee8x9f92.wl-tiwai@suse.de>
@ 2021-10-07 13:11           ` Arnd Bergmann
  2021-10-07 15:33             ` Takashi Iwai
  0 siblings, 1 reply; 24+ messages in thread
From: Arnd Bergmann @ 2021-10-07 13:11 UTC (permalink / raw)
  To: Takashi Iwai
  Cc: Arnd Bergmann, Michael Forney, ALSA Development Mailing List,
	Takashi Iwai, Baolin Wang, y2038 Mailman List,
	Linux Kernel Mailing List, Mark Brown, Baolin Wang, musl

 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.

      Arnd

[1] https://lore.kernel.org/all/29QBMJU8DE71E.2YZSH8IHT5HMH@mforney.org/

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [musl] Re: [alsa-devel] [PATCH v7 8/9] ALSA: add new 32-bit layout for snd_pcm_mmap_status/control
  2021-10-07 13:11           ` [musl] Re: [alsa-devel] [PATCH v7 8/9] ALSA: add new 32-bit layout for snd_pcm_mmap_status/control Arnd Bergmann
@ 2021-10-07 15:33             ` Takashi Iwai
  2021-10-07 16:06               ` Rich Felker
  0 siblings, 1 reply; 24+ messages in thread
From: Takashi Iwai @ 2021-10-07 15:33 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Michael Forney, ALSA Development Mailing List, Takashi Iwai,
	Baolin Wang, y2038 Mailman List, Linux Kernel Mailing List,
	Mark Brown, Baolin Wang, musl

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.

Do we know how widely the __USE_TIME_BITS64 is deployed nowadays?


Takashi

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [musl] Re: [alsa-devel] [PATCH v7 8/9] ALSA: add new 32-bit layout for snd_pcm_mmap_status/control
  2021-10-07 15:33             ` Takashi Iwai
@ 2021-10-07 16:06               ` Rich Felker
  2021-10-07 16:18                 ` Takashi Iwai
  0 siblings, 1 reply; 24+ messages in thread
From: Rich Felker @ 2021-10-07 16:06 UTC (permalink / raw)
  To: Takashi Iwai
  Cc: Arnd Bergmann, Michael Forney, ALSA Development Mailing List,
	Takashi Iwai, Baolin Wang, y2038 Mailman List,
	Linux Kernel Mailing List, Mark Brown, Baolin Wang, musl

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?

> Do we know how widely the __USE_TIME_BITS64 is deployed nowadays?

Anyone using musl on 32-bit archs who's not >=2 years behind current.

Rich

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [musl] Re: [alsa-devel] [PATCH v7 8/9] ALSA: add new 32-bit layout for snd_pcm_mmap_status/control
  2021-10-07 16:06               ` Rich Felker
@ 2021-10-07 16:18                 ` Takashi Iwai
  2021-10-07 16:51                   ` Rich Felker
  0 siblings, 1 reply; 24+ messages in thread
From: Takashi Iwai @ 2021-10-07 16:18 UTC (permalink / raw)
  To: Rich Felker
  Cc: Arnd Bergmann, Michael Forney, ALSA Development Mailing List,
	Takashi Iwai, Baolin Wang, y2038 Mailman List,
	Linux Kernel Mailing List, Mark Brown, Baolin Wang, musl

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.

> > Do we know how widely the __USE_TIME_BITS64 is deployed nowadays?
> 
> Anyone using musl on 32-bit archs who's not >=2 years behind current.

OK.


Takashi

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [musl] Re: [alsa-devel] [PATCH v7 8/9] ALSA: add new 32-bit layout for snd_pcm_mmap_status/control
  2021-10-07 16:18                 ` Takashi Iwai
@ 2021-10-07 16:51                   ` Rich Felker
  2021-10-08  8:43                     ` Takashi Iwai
  0 siblings, 1 reply; 24+ messages in thread
From: Rich Felker @ 2021-10-07 16:51 UTC (permalink / raw)
  To: Takashi Iwai
  Cc: Arnd Bergmann, Michael Forney, ALSA Development Mailing List,
	Takashi Iwai, Baolin Wang, y2038 Mailman List,
	Linux Kernel Mailing List, Mark Brown, Baolin Wang, musl

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.

Rich

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [musl] Re: [alsa-devel] [PATCH v7 8/9] ALSA: add new 32-bit layout for snd_pcm_mmap_status/control
  2021-10-07 16:51                   ` Rich Felker
@ 2021-10-08  8:43                     ` Takashi Iwai
  2021-10-08  8:44                       ` Takashi Iwai
  2021-10-08  9:24                       ` Arnd Bergmann
  0 siblings, 2 replies; 24+ messages in thread
From: Takashi Iwai @ 2021-10-08  8:43 UTC (permalink / raw)
  To: Rich Felker
  Cc: Arnd Bergmann, Michael Forney, ALSA Development Mailing List,
	Takashi Iwai, Baolin Wang, y2038 Mailman List,
	Linux Kernel Mailing List, Mark Brown, Baolin Wang, musl

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);

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [musl] Re: [alsa-devel] [PATCH v7 8/9] ALSA: add new 32-bit layout for snd_pcm_mmap_status/control
  2021-10-08  8:43                     ` Takashi Iwai
@ 2021-10-08  8:44                       ` Takashi Iwai
  2021-10-08  9:24                       ` Arnd Bergmann
  1 sibling, 0 replies; 24+ messages in thread
From: Takashi Iwai @ 2021-10-08  8:44 UTC (permalink / raw)
  To: Rich Felker
  Cc: Arnd Bergmann, Michael Forney, ALSA Development Mailing List,
	Takashi Iwai, Baolin Wang, y2038 Mailman List,
	Linux Kernel Mailing List, Mark Brown, Baolin Wang, musl

On Fri, 08 Oct 2021 10:43:24 +0200,
Takashi Iwai wrote:
> 
> 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.

And below is the PoC fix for alsa-lib.


Takashi

---
diff --git a/include/sound/uapi/asound.h b/include/sound/uapi/asound.h
index 9fe3943f5fbb..ee91fe1f881f 100644
--- a/include/sound/uapi/asound.h
+++ b/include/sound/uapi/asound.h
@@ -154,7 +154,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;
@@ -541,6 +541,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;
 };
@@ -548,11 +549,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 {
@@ -570,13 +575,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;
@@ -586,6 +601,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/src/pcm/pcm_hw.c b/src/pcm/pcm_hw.c
index b3f9d1579d29..bb97b7ecf5ca 100644
--- a/src/pcm/pcm_hw.c
+++ b/src/pcm/pcm_hw.c
@@ -94,6 +94,7 @@ typedef struct {
 
 	volatile struct snd_pcm_mmap_status * mmap_status;
 	struct snd_pcm_mmap_control *mmap_control;
+	snd_pcm_uframes_t *avail_min_p;
 	bool mmap_status_fallbacked;
 	bool mmap_control_fallbacked;
 	struct snd_pcm_sync_ptr *sync_ptr;
@@ -507,7 +508,7 @@ static int snd_pcm_hw_sw_params(snd_pcm_t *pcm, snd_pcm_sw_params_t * params)
 	    params->silence_threshold == pcm->silence_threshold &&
 	    params->silence_size == pcm->silence_size &&
 	    old_period_event == hw->period_event) {
-		hw->mmap_control->avail_min = params->avail_min;
+		*hw->avail_min_p = params->avail_min;
 		err = issue_avail_min(hw);
 		goto out;
 	}
@@ -540,7 +541,7 @@ static int snd_pcm_hw_sw_params(snd_pcm_t *pcm, snd_pcm_sw_params_t * params)
 		}
 		pcm->tstamp_type = params->tstamp_type;
 	}
-	hw->mmap_control->avail_min = params->avail_min;
+	*hw->avail_min_p = params->avail_min;
 	if (hw->period_event != old_period_event) {
 		err = snd_pcm_hw_change_timer(pcm, old_period_event);
 		if (err < 0)
@@ -980,6 +981,14 @@ static bool map_control_data(snd_pcm_hw_t *hw,
 	}
 
 	hw->mmap_control = mmap_control;
+	hw->avail_min_p = &mmap_control->avail_min;
+#ifdef __SND_STRUCT_TIME64
+	if (hw->version == SNDRV_PROTOCOL_VERSION(2, 0, 15)) {
+		struct __snd_pcm_mmap_control64_api_2_0_15 *buggy_control =
+			(struct __snd_pcm_mmap_control64_api_2_0_15 *)mmap_control;
+		hw->avail_min_p = &buggy_control->avail_min;
+	}
+#endif
 
 	return fallbacked;
 }
@@ -1015,7 +1024,7 @@ static int map_status_and_control_data(snd_pcm_t *pcm, bool force_fallback)
 	if (!(pcm->mode & SND_PCM_APPEND)) {
 		/* Initialize the data. */
 		hw->mmap_control->appl_ptr = 0;
-		hw->mmap_control->avail_min = 1;
+		*hw->avail_min_p = 1;
 	}
 	snd_pcm_set_hw_ptr(pcm, &hw->mmap_status->hw_ptr, hw->fd,
 			   SNDRV_PCM_MMAP_OFFSET_STATUS +

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [musl] Re: [alsa-devel] [PATCH v7 8/9] ALSA: add new 32-bit layout for snd_pcm_mmap_status/control
  2021-10-08  8:43                     ` Takashi Iwai
  2021-10-08  8:44                       ` Takashi Iwai
@ 2021-10-08  9:24                       ` Arnd Bergmann
  2021-10-08 11:11                         ` Takashi Iwai
  2021-10-08 12:06                         ` Rich Felker
  1 sibling, 2 replies; 24+ messages in thread
From: Arnd Bergmann @ 2021-10-08  9:24 UTC (permalink / raw)
  To: musl
  Cc: Rich Felker, Arnd Bergmann, Michael Forney,
	ALSA Development Mailing List, Takashi Iwai, Baolin Wang,
	y2038 Mailman List, Linux Kernel Mailing List, Mark Brown,
	Baolin Wang

On Fri, Oct 8, 2021 at 10:43 AM Takashi Iwai <tiwai@suse.de> wrote:
> 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:
>
> @@ -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

I think these should remain unchanged, the complex expression was intentionally
done so the structures are laid out the same way on 64-bit
architectures, so that
the kernel can use the __SND_STRUCT_TIME64 path internally on both 32-bit
and 64-bit architectures.

> @@ -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;
> +       }

The problem I see with this is that it might break musl's ability to
emulate the new
interface on top of the old (time32) one for linux-4.x and older
kernels, as the conversion
function is no longer stateless but has to know the negotiated
interface version.

It's probably fine as long as we can be sure that the 2.0.16+ API
version only gets
negotiated if both the kernel and user sides support it, and musl only emulates
the 2.0.15 API version from the current kernels.

I've tried to understand this part of musl's convert_ioctl_struct(), but I just
can't figure out whether it does the conversion based the on the layout that
is currently used in the kernel, or based on the layout we should have been
using, and would use with the above fix. Rich, can you help me here?

       Arnd

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [musl] Re: [alsa-devel] [PATCH v7 8/9] ALSA: add new 32-bit layout for snd_pcm_mmap_status/control
  2021-10-08  9:24                       ` Arnd Bergmann
@ 2021-10-08 11:11                         ` Takashi Iwai
  2021-10-08 11:45                           ` Arnd Bergmann
  2021-10-08 12:07                           ` Rich Felker
  2021-10-08 12:06                         ` Rich Felker
  1 sibling, 2 replies; 24+ messages in thread
From: Takashi Iwai @ 2021-10-08 11:11 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: musl, Rich Felker, Michael Forney, ALSA Development Mailing List,
	Takashi Iwai, Baolin Wang, y2038 Mailman List,
	Linux Kernel Mailing List, Mark Brown, Baolin Wang

On Fri, 08 Oct 2021 11:24:39 +0200,
Arnd Bergmann wrote:
> 
> On Fri, Oct 8, 2021 at 10:43 AM Takashi Iwai <tiwai@suse.de> wrote:
> > 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:
> >
> > @@ -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
> 
> I think these should remain unchanged, the complex expression was intentionally
> done so the structures are laid out the same way on 64-bit
> architectures, so that
> the kernel can use the __SND_STRUCT_TIME64 path internally on both 32-bit
> and 64-bit architectures.

That was explicitly defined, but OK, this isn't necessarily defined
here.

> > @@ -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;
> > +       }
> 
> The problem I see with this is that it might break musl's ability to
> emulate the new
> interface on top of the old (time32) one for linux-4.x and older
> kernels, as the conversion
> function is no longer stateless but has to know the negotiated
> interface version.
> 
> It's probably fine as long as we can be sure that the 2.0.16+ API
> version only gets
> negotiated if both the kernel and user sides support it, and musl only emulates
> the 2.0.15 API version from the current kernels.
> 
> I've tried to understand this part of musl's convert_ioctl_struct(), but I just
> can't figure out whether it does the conversion based the on the layout that
> is currently used in the kernel, or based on the layout we should have been
> using, and would use with the above fix. Rich, can you help me here?

So, at this moment, I'm not sure whether we should correct the struct
at all.  This will lead to yet more breakage, and basically the struct
itself *works* -- the only bug is in 32bit compat handling in the
kernel (again).

The below is a revised kernel patch (again untested), just correcting
the behavior of 32bit compat mode.  32bit apps on 32bit kernel work
fine as is, as well as 64bit apps on 64bit kernel.


Takashi

--- a/sound/core/pcm_compat.c
+++ b/sound/core/pcm_compat.c
@@ -468,6 +468,75 @@ static int snd_pcm_ioctl_sync_ptr_x32(struct snd_pcm_substream *substream,
 }
 #endif /* CONFIG_X86_X32 */
 
+#ifdef __BIG_ENDIAN
+typedef char __pad_before_u32[4];
+typedef char __pad_after_u32[0];
+#else
+typedef char __pad_before_u32[0];
+typedef char __pad_after_u32[4];
+#endif
+
+/* 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, hitting only on 32bit.
+ * Workaround for incorrect read/write is needed only in 32bit compat mode.
+ */
+struct __snd_pcm_mmap_control64_buggy {
+	__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;
+};
+
+static int snd_pcm_ioctl_sync_ptr_buggy(struct snd_pcm_substream *substream,
+					struct snd_pcm_sync_ptr __user *_sync_ptr)
+{
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	struct snd_pcm_sync_ptr sync_ptr;
+	struct __snd_pcm_mmap_control64_buggy *sync_cp;
+	volatile struct snd_pcm_mmap_status *status;
+	volatile struct snd_pcm_mmap_control *control;
+	int err;
+
+	memset(&sync_ptr, 0, sizeof(sync_ptr));
+	sync_cp = (struct __snd_pcm_mmap_control64_buggy *)&sync_ptr.c.control;
+	if (get_user(sync_ptr.flags, (unsigned __user *)&(_sync_ptr->flags)))
+		return -EFAULT;
+	if (copy_from_user(sync_cp, &(_sync_ptr->c.control), sizeof(*sync_cp)))
+		return -EFAULT;
+	status = runtime->status;
+	control = runtime->control;
+	if (sync_ptr.flags & SNDRV_PCM_SYNC_PTR_HWSYNC) {
+		err = snd_pcm_hwsync(substream);
+		if (err < 0)
+			return err;
+	}
+	snd_pcm_stream_lock_irq(substream);
+	if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_APPL)) {
+		err = pcm_lib_apply_appl_ptr(substream, sync_cp->appl_ptr);
+		if (err < 0) {
+			snd_pcm_stream_unlock_irq(substream);
+			return err;
+		}
+	} else {
+		sync_cp->appl_ptr = control->appl_ptr;
+	}
+	if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_AVAIL_MIN))
+		control->avail_min = sync_cp->avail_min;
+	else
+		sync_cp->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;
+	sync_ptr.s.status.suspended_state = status->suspended_state;
+	sync_ptr.s.status.audio_tstamp = status->audio_tstamp;
+	snd_pcm_stream_unlock_irq(substream);
+	if (copy_to_user(_sync_ptr, &sync_ptr, sizeof(sync_ptr)))
+		return -EFAULT;
+	return 0;
+}
+
 /*
  */
 enum {
@@ -537,7 +606,7 @@ static long snd_pcm_ioctl_compat(struct file *file, unsigned int cmd, unsigned l
 		if (in_x32_syscall())
 			return snd_pcm_ioctl_sync_ptr_x32(substream, argp);
 #endif /* CONFIG_X86_X32 */
-		return snd_pcm_common_ioctl(file, substream, cmd, argp);
+		return snd_pcm_ioctl_sync_ptr_buggy(substream, argp);
 	case SNDRV_PCM_IOCTL_HW_REFINE32:
 		return snd_pcm_ioctl_hw_params_compat(substream, 1, argp);
 	case SNDRV_PCM_IOCTL_HW_PARAMS32:

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [musl] Re: [alsa-devel] [PATCH v7 8/9] ALSA: add new 32-bit layout for snd_pcm_mmap_status/control
  2021-10-08 11:11                         ` Takashi Iwai
@ 2021-10-08 11:45                           ` Arnd Bergmann
  2021-10-08 11:53                             ` Takashi Iwai
  2021-10-08 12:07                           ` Rich Felker
  1 sibling, 1 reply; 24+ messages in thread
From: Arnd Bergmann @ 2021-10-08 11:45 UTC (permalink / raw)
  To: musl
  Cc: Arnd Bergmann, Rich Felker, Michael Forney,
	ALSA Development Mailing List, Takashi Iwai, Baolin Wang,
	y2038 Mailman List, Linux Kernel Mailing List, Mark Brown,
	Baolin Wang

On Fri, Oct 8, 2021 at 1:11 PM Takashi Iwai <tiwai@suse.de> wrote:
> On Fri, 08 Oct 2021 11:24:39 +0200, Arnd Bergmann wrote:

> >
> > I've tried to understand this part of musl's convert_ioctl_struct(), but I just
> > can't figure out whether it does the conversion based the on the layout that
> > is currently used in the kernel, or based on the layout we should have been
> > using, and would use with the above fix. Rich, can you help me here?
>
> So, at this moment, I'm not sure whether we should correct the struct
> at all.  This will lead to yet more breakage, and basically the struct
> itself *works* -- the only bug is in 32bit compat handling in the
> kernel (again).

I'm still unsure if the musl fallback code is correct or not.

> The below is a revised kernel patch (again untested), just correcting
> the behavior of 32bit compat mode.  32bit apps on 32bit kernel work
> fine as is, as well as 64bit apps on 64bit kernel.

Right, this should cover all cases of the ioctl itself misbehaving.
In addition, we still need to disallow the mmap() interface on compat
kernels then. Strictly speaking, we could allow the snd_pcm_mmap_status
but not snd_pcm_mmap_control to be mapped, but I'm not sure if
that's better than disallowing both.

       Arnd

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [musl] Re: [alsa-devel] [PATCH v7 8/9] ALSA: add new 32-bit layout for snd_pcm_mmap_status/control
  2021-10-08 11:45                           ` Arnd Bergmann
@ 2021-10-08 11:53                             ` Takashi Iwai
  2021-10-08 12:13                               ` Arnd Bergmann
  0 siblings, 1 reply; 24+ messages in thread
From: Takashi Iwai @ 2021-10-08 11:53 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: musl, Rich Felker, Michael Forney, ALSA Development Mailing List,
	Takashi Iwai, Baolin Wang, y2038 Mailman List,
	Linux Kernel Mailing List, Mark Brown, Baolin Wang

On Fri, 08 Oct 2021 13:45:45 +0200,
Arnd Bergmann wrote:
> 
> On Fri, Oct 8, 2021 at 1:11 PM Takashi Iwai <tiwai@suse.de> wrote:
> > On Fri, 08 Oct 2021 11:24:39 +0200, Arnd Bergmann wrote:
> 
> > >
> > > I've tried to understand this part of musl's convert_ioctl_struct(), but I just
> > > can't figure out whether it does the conversion based the on the layout that
> > > is currently used in the kernel, or based on the layout we should have been
> > > using, and would use with the above fix. Rich, can you help me here?
> >
> > So, at this moment, I'm not sure whether we should correct the struct
> > at all.  This will lead to yet more breakage, and basically the struct
> > itself *works* -- the only bug is in 32bit compat handling in the
> > kernel (again).
> 
> I'm still unsure if the musl fallback code is correct or not.

We need to verify the current behavior in anyway...

> > The below is a revised kernel patch (again untested), just correcting
> > the behavior of 32bit compat mode.  32bit apps on 32bit kernel work
> > fine as is, as well as 64bit apps on 64bit kernel.
> 
> Right, this should cover all cases of the ioctl itself misbehaving.
> In addition, we still need to disallow the mmap() interface on compat
> kernels then. Strictly speaking, we could allow the snd_pcm_mmap_status
> but not snd_pcm_mmap_control to be mapped, but I'm not sure if
> that's better than disallowing both.

IIRC, the compat mmap is already disallowed even for the
SNDRV_PCM_MMAP_OFFSET_CONTROL_NEW (in pcm_control_mmap_allowed()), so
no need to change around that.


thanks,

Takashi

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [musl] Re: [alsa-devel] [PATCH v7 8/9] ALSA: add new 32-bit layout for snd_pcm_mmap_status/control
  2021-10-08  9:24                       ` Arnd Bergmann
  2021-10-08 11:11                         ` Takashi Iwai
@ 2021-10-08 12:06                         ` Rich Felker
  2021-10-08 12:37                           ` Arnd Bergmann
  1 sibling, 1 reply; 24+ messages in thread
From: Rich Felker @ 2021-10-08 12:06 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: musl, Michael Forney, ALSA Development Mailing List,
	Takashi Iwai, Baolin Wang, y2038 Mailman List,
	Linux Kernel Mailing List, Mark Brown, Baolin Wang

On Fri, Oct 08, 2021 at 11:24:39AM +0200, Arnd Bergmann wrote:
> On Fri, Oct 8, 2021 at 10:43 AM Takashi Iwai <tiwai@suse.de> wrote:
> > 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:
> >
> > @@ -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
> 
> I think these should remain unchanged, the complex expression was intentionally
> done so the structures are laid out the same way on 64-bit
> architectures, so that
> the kernel can use the __SND_STRUCT_TIME64 path internally on both 32-bit
> and 64-bit architectures.
> 
> > @@ -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;
> > +       }
> 
> The problem I see with this is that it might break musl's ability to
> emulate the new
> interface on top of the old (time32) one for linux-4.x and older
> kernels, as the conversion
> function is no longer stateless but has to know the negotiated
> interface version.
> 
> It's probably fine as long as we can be sure that the 2.0.16+ API
> version only gets
> negotiated if both the kernel and user sides support it, and musl only emulates
> the 2.0.15 API version from the current kernels.
> 
> I've tried to understand this part of musl's convert_ioctl_struct(), but I just
> can't figure out whether it does the conversion based the on the layout that
> is currently used in the kernel, or based on the layout we should have been
> using, and would use with the above fix. Rich, can you help me here?

If the attempted 64-bit ioctl is missing (ENOTTY), it does the
conversion to the legacy 32-bit one and retries with that, then
converts the results back to the 64-bit form.

Not only do I fail to see how the proposed fix is workable with this
framework; I also don't see how the proposed fix would let new
applications (compiled without the buggy type) run on old kernels. I'm
pretty sure there really should be a new ioctl number for this...

Rich

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [musl] Re: [alsa-devel] [PATCH v7 8/9] ALSA: add new 32-bit layout for snd_pcm_mmap_status/control
  2021-10-08 11:11                         ` Takashi Iwai
  2021-10-08 11:45                           ` Arnd Bergmann
@ 2021-10-08 12:07                           ` Rich Felker
  2021-10-10  7:53                             ` Takashi Iwai
  1 sibling, 1 reply; 24+ messages in thread
From: Rich Felker @ 2021-10-08 12:07 UTC (permalink / raw)
  To: Takashi Iwai
  Cc: Arnd Bergmann, musl, Michael Forney,
	ALSA Development Mailing List, Takashi Iwai, Baolin Wang,
	y2038 Mailman List, Linux Kernel Mailing List, Mark Brown,
	Baolin Wang

On Fri, Oct 08, 2021 at 01:11:34PM +0200, Takashi Iwai wrote:
> On Fri, 08 Oct 2021 11:24:39 +0200,
> Arnd Bergmann wrote:
> > 
> > On Fri, Oct 8, 2021 at 10:43 AM Takashi Iwai <tiwai@suse.de> wrote:
> > > 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:
> > >
> > > @@ -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
> > 
> > I think these should remain unchanged, the complex expression was intentionally
> > done so the structures are laid out the same way on 64-bit
> > architectures, so that
> > the kernel can use the __SND_STRUCT_TIME64 path internally on both 32-bit
> > and 64-bit architectures.
> 
> That was explicitly defined, but OK, this isn't necessarily defined
> here.
> 
> > > @@ -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;
> > > +       }
> > 
> > The problem I see with this is that it might break musl's ability to
> > emulate the new
> > interface on top of the old (time32) one for linux-4.x and older
> > kernels, as the conversion
> > function is no longer stateless but has to know the negotiated
> > interface version.
> > 
> > It's probably fine as long as we can be sure that the 2.0.16+ API
> > version only gets
> > negotiated if both the kernel and user sides support it, and musl only emulates
> > the 2.0.15 API version from the current kernels.
> > 
> > I've tried to understand this part of musl's convert_ioctl_struct(), but I just
> > can't figure out whether it does the conversion based the on the layout that
> > is currently used in the kernel, or based on the layout we should have been
> > using, and would use with the above fix. Rich, can you help me here?
> 
> So, at this moment, I'm not sure whether we should correct the struct
> at all.  This will lead to yet more breakage, and basically the struct
> itself *works* -- the only bug is in 32bit compat handling in the
> kernel (again).
> 
> The below is a revised kernel patch (again untested), just correcting
> the behavior of 32bit compat mode.  32bit apps on 32bit kernel work
> fine as is, as well as 64bit apps on 64bit kernel.

I'm perfectly okay with this if Arnd is! It's probably the least
invasive and has the least long-term maintenance cost and fallout on
other projects.

Rich

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [musl] Re: [alsa-devel] [PATCH v7 8/9] ALSA: add new 32-bit layout for snd_pcm_mmap_status/control
  2021-10-08 11:53                             ` Takashi Iwai
@ 2021-10-08 12:13                               ` Arnd Bergmann
  0 siblings, 0 replies; 24+ messages in thread
From: Arnd Bergmann @ 2021-10-08 12:13 UTC (permalink / raw)
  To: musl
  Cc: Arnd Bergmann, Rich Felker, Michael Forney,
	ALSA Development Mailing List, Takashi Iwai, Baolin Wang,
	y2038 Mailman List, Linux Kernel Mailing List, Mark Brown,
	Baolin Wang

On Fri, Oct 8, 2021 at 1:53 PM Takashi Iwai <tiwai@suse.de> wrote:
> On Fri, 08 Oct 2021 13:45:45 +0200, Arnd Bergmann wrote:
> > On Fri, Oct 8, 2021 at 1:11 PM Takashi Iwai <tiwai@suse.de> wrote:
> > > On Fri, 08 Oct 2021 11:24:39 +0200, Arnd Bergmann wrote:
> > > The below is a revised kernel patch (again untested), just correcting
> > > the behavior of 32bit compat mode.  32bit apps on 32bit kernel work
> > > fine as is, as well as 64bit apps on 64bit kernel.
> >
> > Right, this should cover all cases of the ioctl itself misbehaving.
> > In addition, we still need to disallow the mmap() interface on compat
> > kernels then. Strictly speaking, we could allow the snd_pcm_mmap_status
> > but not snd_pcm_mmap_control to be mapped, but I'm not sure if
> > that's better than disallowing both.
>
> IIRC, the compat mmap is already disallowed even for the
> SNDRV_PCM_MMAP_OFFSET_CONTROL_NEW (in pcm_control_mmap_allowed()), so
> no need to change around that.

Ah, right. I think it was meant to become allowed as part of commit
80fe7430c708 ("ALSA: add new 32-bit layout for snd_pcm_mmap_status/control"),
which did allow the snd_pcm_mmap_status to be mmap()ed, but it appears
to be the rare case where two mistakes cancel out and we don't have to
change the mmap code.

         Arnd

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [musl] Re: [alsa-devel] [PATCH v7 8/9] ALSA: add new 32-bit layout for snd_pcm_mmap_status/control
  2021-10-08 12:06                         ` Rich Felker
@ 2021-10-08 12:37                           ` Arnd Bergmann
  2021-10-08 17:20                             ` Rich Felker
  0 siblings, 1 reply; 24+ messages in thread
From: Arnd Bergmann @ 2021-10-08 12:37 UTC (permalink / raw)
  To: musl
  Cc: Arnd Bergmann, Michael Forney, ALSA Development Mailing List,
	Takashi Iwai, Baolin Wang, y2038 Mailman List,
	Linux Kernel Mailing List, Mark Brown, Baolin Wang

On Fri, Oct 8, 2021 at 2:06 PM Rich Felker <dalias@libc.org> wrote:
> On Fri, Oct 08, 2021 at 11:24:39AM +0200, Arnd Bergmann wrote:
> >
> > I've tried to understand this part of musl's convert_ioctl_struct(), but I just
> > can't figure out whether it does the conversion based the on the layout that
> > is currently used in the kernel, or based on the layout we should have been
> > using, and would use with the above fix. Rich, can you help me here?
>
> If the attempted 64-bit ioctl is missing (ENOTTY), it does the
> conversion to the legacy 32-bit one and retries with that, then
> converts the results back to the 64-bit form.

I understand that it tries to do that.

The part that I'm not sure about is which of the two possible
64-bit forms it's using -- the broken one we have defined in the
kernel headers, or the one we were trying to define but failed.

      Arnd

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [musl] Re: [alsa-devel] [PATCH v7 8/9] ALSA: add new 32-bit layout for snd_pcm_mmap_status/control
  2021-10-08 12:37                           ` Arnd Bergmann
@ 2021-10-08 17:20                             ` Rich Felker
  0 siblings, 0 replies; 24+ messages in thread
From: Rich Felker @ 2021-10-08 17:20 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: musl, Michael Forney, ALSA Development Mailing List,
	Takashi Iwai, Baolin Wang, y2038 Mailman List,
	Linux Kernel Mailing List, Mark Brown, Baolin Wang

On Fri, Oct 08, 2021 at 02:37:12PM +0200, Arnd Bergmann wrote:
> On Fri, Oct 8, 2021 at 2:06 PM Rich Felker <dalias@libc.org> wrote:
> > On Fri, Oct 08, 2021 at 11:24:39AM +0200, Arnd Bergmann wrote:
> > >
> > > I've tried to understand this part of musl's convert_ioctl_struct(), but I just
> > > can't figure out whether it does the conversion based the on the layout that
> > > is currently used in the kernel, or based on the layout we should have been
> > > using, and would use with the above fix. Rich, can you help me here?
> >
> > If the attempted 64-bit ioctl is missing (ENOTTY), it does the
> > conversion to the legacy 32-bit one and retries with that, then
> > converts the results back to the 64-bit form.
> 
> I understand that it tries to do that.
> 
> The part that I'm not sure about is which of the two possible
> 64-bit forms it's using -- the broken one we have defined in the
> kernel headers, or the one we were trying to define but failed.

It's attempting to convert the intended format, not the one that the
uapi headers defined. That is, it's taking padded-to-64-bit values at
offsets 0 and 8 in __snd_pcm_mmap_control64, putting them at offsets 0
and 4 in the 32-bit struct, and padding them back to 64-bit in the
result.

Since applications would have been compiled with the buggy
(unintended) version of the uapi headers, this will not match the
application's layout of the struct. I haven't worked through what all
the consequences of that are, but I think some fix is needed here in
musl regardless of what happens on the kernel side.

Rich

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [musl] Re: [alsa-devel] [PATCH v7 8/9] ALSA: add new 32-bit layout for snd_pcm_mmap_status/control
  2021-10-08 12:07                           ` Rich Felker
@ 2021-10-10  7:53                             ` Takashi Iwai
  2021-10-18 14:43                               ` Rich Felker
  0 siblings, 1 reply; 24+ messages in thread
From: Takashi Iwai @ 2021-10-10  7:53 UTC (permalink / raw)
  To: Rich Felker
  Cc: Arnd Bergmann, musl, Michael Forney,
	ALSA Development Mailing List, Takashi Iwai, Baolin Wang,
	y2038 Mailman List, Linux Kernel Mailing List, Mark Brown,
	Baolin Wang

On Fri, 08 Oct 2021 14:07:39 +0200,
Rich Felker wrote:
> 
> On Fri, Oct 08, 2021 at 01:11:34PM +0200, Takashi Iwai wrote:
> > On Fri, 08 Oct 2021 11:24:39 +0200,
> > Arnd Bergmann wrote:
> > > 
> > > On Fri, Oct 8, 2021 at 10:43 AM Takashi Iwai <tiwai@suse.de> wrote:
> > > > 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:
> > > >
> > > > @@ -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
> > > 
> > > I think these should remain unchanged, the complex expression was intentionally
> > > done so the structures are laid out the same way on 64-bit
> > > architectures, so that
> > > the kernel can use the __SND_STRUCT_TIME64 path internally on both 32-bit
> > > and 64-bit architectures.
> > 
> > That was explicitly defined, but OK, this isn't necessarily defined
> > here.
> > 
> > > > @@ -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;
> > > > +       }
> > > 
> > > The problem I see with this is that it might break musl's ability to
> > > emulate the new
> > > interface on top of the old (time32) one for linux-4.x and older
> > > kernels, as the conversion
> > > function is no longer stateless but has to know the negotiated
> > > interface version.
> > > 
> > > It's probably fine as long as we can be sure that the 2.0.16+ API
> > > version only gets
> > > negotiated if both the kernel and user sides support it, and musl only emulates
> > > the 2.0.15 API version from the current kernels.
> > > 
> > > I've tried to understand this part of musl's convert_ioctl_struct(), but I just
> > > can't figure out whether it does the conversion based the on the layout that
> > > is currently used in the kernel, or based on the layout we should have been
> > > using, and would use with the above fix. Rich, can you help me here?
> > 
> > So, at this moment, I'm not sure whether we should correct the struct
> > at all.  This will lead to yet more breakage, and basically the struct
> > itself *works* -- the only bug is in 32bit compat handling in the
> > kernel (again).
> > 
> > The below is a revised kernel patch (again untested), just correcting
> > the behavior of 32bit compat mode.  32bit apps on 32bit kernel work
> > fine as is, as well as 64bit apps on 64bit kernel.
> 
> I'm perfectly okay with this if Arnd is! It's probably the least
> invasive and has the least long-term maintenance cost and fallout on
> other projects.

OK, I'll submit a proper patch now, to be included in the next PR for
5.15-rc.  For further fixes, let's think carefully.


thanks,

Takashi

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [musl] Re: [alsa-devel] [PATCH v7 8/9] ALSA: add new 32-bit layout for snd_pcm_mmap_status/control
  2021-10-10  7:53                             ` Takashi Iwai
@ 2021-10-18 14:43                               ` Rich Felker
  2021-10-18 14:58                                 ` Takashi Iwai
  0 siblings, 1 reply; 24+ messages in thread
From: Rich Felker @ 2021-10-18 14:43 UTC (permalink / raw)
  To: Takashi Iwai
  Cc: Arnd Bergmann, musl, Michael Forney,
	ALSA Development Mailing List, Takashi Iwai, Baolin Wang,
	y2038 Mailman List, Linux Kernel Mailing List, Mark Brown,
	Baolin Wang

On Sun, Oct 10, 2021 at 09:53:38AM +0200, Takashi Iwai wrote:
> On Fri, 08 Oct 2021 14:07:39 +0200,
> Rich Felker wrote:
> > 
> > On Fri, Oct 08, 2021 at 01:11:34PM +0200, Takashi Iwai wrote:
> > > On Fri, 08 Oct 2021 11:24:39 +0200,
> > > Arnd Bergmann wrote:
> > > > 
> > > > On Fri, Oct 8, 2021 at 10:43 AM Takashi Iwai <tiwai@suse.de> wrote:
> > > > > 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:
> > > > >
> > > > > @@ -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
> > > > 
> > > > I think these should remain unchanged, the complex expression was intentionally
> > > > done so the structures are laid out the same way on 64-bit
> > > > architectures, so that
> > > > the kernel can use the __SND_STRUCT_TIME64 path internally on both 32-bit
> > > > and 64-bit architectures.
> > > 
> > > That was explicitly defined, but OK, this isn't necessarily defined
> > > here.
> > > 
> > > > > @@ -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;
> > > > > +       }
> > > > 
> > > > The problem I see with this is that it might break musl's ability to
> > > > emulate the new
> > > > interface on top of the old (time32) one for linux-4.x and older
> > > > kernels, as the conversion
> > > > function is no longer stateless but has to know the negotiated
> > > > interface version.
> > > > 
> > > > It's probably fine as long as we can be sure that the 2.0.16+ API
> > > > version only gets
> > > > negotiated if both the kernel and user sides support it, and musl only emulates
> > > > the 2.0.15 API version from the current kernels.
> > > > 
> > > > I've tried to understand this part of musl's convert_ioctl_struct(), but I just
> > > > can't figure out whether it does the conversion based the on the layout that
> > > > is currently used in the kernel, or based on the layout we should have been
> > > > using, and would use with the above fix. Rich, can you help me here?
> > > 
> > > So, at this moment, I'm not sure whether we should correct the struct
> > > at all.  This will lead to yet more breakage, and basically the struct
> > > itself *works* -- the only bug is in 32bit compat handling in the
> > > kernel (again).
> > > 
> > > The below is a revised kernel patch (again untested), just correcting
> > > the behavior of 32bit compat mode.  32bit apps on 32bit kernel work
> > > fine as is, as well as 64bit apps on 64bit kernel.
> > 
> > I'm perfectly okay with this if Arnd is! It's probably the least
> > invasive and has the least long-term maintenance cost and fallout on
> > other projects.
> 
> OK, I'll submit a proper patch now, to be included in the next PR for
> 5.15-rc.  For further fixes, let's think carefully.

Am I correct in my understanding that the fix of keeping the "broken"
definition (and having the 64-bit kernel honor it for 32-bit binaries)
has been accepted? Since musl's translation for pre-time64 kernels
seems to have been using the "non-broken" definition, I think
completing the fix requires a change in musl too.

Rich

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [musl] Re: [alsa-devel] [PATCH v7 8/9] ALSA: add new 32-bit layout for snd_pcm_mmap_status/control
  2021-10-18 14:43                               ` Rich Felker
@ 2021-10-18 14:58                                 ` Takashi Iwai
  2021-10-18 15:08                                   ` Rich Felker
  0 siblings, 1 reply; 24+ messages in thread
From: Takashi Iwai @ 2021-10-18 14:58 UTC (permalink / raw)
  To: Rich Felker
  Cc: Arnd Bergmann, musl, Michael Forney,
	ALSA Development Mailing List, Takashi Iwai, Baolin Wang,
	y2038 Mailman List, Linux Kernel Mailing List, Mark Brown,
	Baolin Wang

On Mon, 18 Oct 2021 16:43:00 +0200,
Rich Felker wrote:
> 
> On Sun, Oct 10, 2021 at 09:53:38AM +0200, Takashi Iwai wrote:
> > On Fri, 08 Oct 2021 14:07:39 +0200,
> > Rich Felker wrote:
> > > 
> > > On Fri, Oct 08, 2021 at 01:11:34PM +0200, Takashi Iwai wrote:
> > > > On Fri, 08 Oct 2021 11:24:39 +0200,
> > > > Arnd Bergmann wrote:
> > > > > 
> > > > > On Fri, Oct 8, 2021 at 10:43 AM Takashi Iwai <tiwai@suse.de> wrote:
> > > > > > 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:
> > > > > >
> > > > > > @@ -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
> > > > > 
> > > > > I think these should remain unchanged, the complex expression was intentionally
> > > > > done so the structures are laid out the same way on 64-bit
> > > > > architectures, so that
> > > > > the kernel can use the __SND_STRUCT_TIME64 path internally on both 32-bit
> > > > > and 64-bit architectures.
> > > > 
> > > > That was explicitly defined, but OK, this isn't necessarily defined
> > > > here.
> > > > 
> > > > > > @@ -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;
> > > > > > +       }
> > > > > 
> > > > > The problem I see with this is that it might break musl's ability to
> > > > > emulate the new
> > > > > interface on top of the old (time32) one for linux-4.x and older
> > > > > kernels, as the conversion
> > > > > function is no longer stateless but has to know the negotiated
> > > > > interface version.
> > > > > 
> > > > > It's probably fine as long as we can be sure that the 2.0.16+ API
> > > > > version only gets
> > > > > negotiated if both the kernel and user sides support it, and musl only emulates
> > > > > the 2.0.15 API version from the current kernels.
> > > > > 
> > > > > I've tried to understand this part of musl's convert_ioctl_struct(), but I just
> > > > > can't figure out whether it does the conversion based the on the layout that
> > > > > is currently used in the kernel, or based on the layout we should have been
> > > > > using, and would use with the above fix. Rich, can you help me here?
> > > > 
> > > > So, at this moment, I'm not sure whether we should correct the struct
> > > > at all.  This will lead to yet more breakage, and basically the struct
> > > > itself *works* -- the only bug is in 32bit compat handling in the
> > > > kernel (again).
> > > > 
> > > > The below is a revised kernel patch (again untested), just correcting
> > > > the behavior of 32bit compat mode.  32bit apps on 32bit kernel work
> > > > fine as is, as well as 64bit apps on 64bit kernel.
> > > 
> > > I'm perfectly okay with this if Arnd is! It's probably the least
> > > invasive and has the least long-term maintenance cost and fallout on
> > > other projects.
> > 
> > OK, I'll submit a proper patch now, to be included in the next PR for
> > 5.15-rc.  For further fixes, let's think carefully.
> 
> Am I correct in my understanding that the fix of keeping the "broken"
> definition (and having the 64-bit kernel honor it for 32-bit binaries)
> has been accepted?

Yes, as it was already set in stone, we accept the broken definition
as is.

> Since musl's translation for pre-time64 kernels
> seems to have been using the "non-broken" definition, I think
> completing the fix requires a change in musl too.

Hm, musl translator contains the own definition of ioctl?

If so, we may reconsider about renumbering ioctls altogether.
Suppose musl having a fallback to the old ioctl, the possible breakage
by old kernels (that don't support renewed ioctls) would be minimal,
right?


Takashi

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [musl] Re: [alsa-devel] [PATCH v7 8/9] ALSA: add new 32-bit layout for snd_pcm_mmap_status/control
  2021-10-18 14:58                                 ` Takashi Iwai
@ 2021-10-18 15:08                                   ` Rich Felker
  2021-10-18 15:26                                     ` Arnd Bergmann
  0 siblings, 1 reply; 24+ messages in thread
From: Rich Felker @ 2021-10-18 15:08 UTC (permalink / raw)
  To: Takashi Iwai
  Cc: Arnd Bergmann, musl, Michael Forney,
	ALSA Development Mailing List, Takashi Iwai, Baolin Wang,
	y2038 Mailman List, Linux Kernel Mailing List, Mark Brown,
	Baolin Wang

[-- Attachment #1: Type: text/plain, Size: 6154 bytes --]

On Mon, Oct 18, 2021 at 04:58:03PM +0200, Takashi Iwai wrote:
> On Mon, 18 Oct 2021 16:43:00 +0200,
> Rich Felker wrote:
> > 
> > On Sun, Oct 10, 2021 at 09:53:38AM +0200, Takashi Iwai wrote:
> > > On Fri, 08 Oct 2021 14:07:39 +0200,
> > > Rich Felker wrote:
> > > > 
> > > > On Fri, Oct 08, 2021 at 01:11:34PM +0200, Takashi Iwai wrote:
> > > > > On Fri, 08 Oct 2021 11:24:39 +0200,
> > > > > Arnd Bergmann wrote:
> > > > > > 
> > > > > > On Fri, Oct 8, 2021 at 10:43 AM Takashi Iwai <tiwai@suse.de> wrote:
> > > > > > > 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:
> > > > > > >
> > > > > > > @@ -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
> > > > > > 
> > > > > > I think these should remain unchanged, the complex expression was intentionally
> > > > > > done so the structures are laid out the same way on 64-bit
> > > > > > architectures, so that
> > > > > > the kernel can use the __SND_STRUCT_TIME64 path internally on both 32-bit
> > > > > > and 64-bit architectures.
> > > > > 
> > > > > That was explicitly defined, but OK, this isn't necessarily defined
> > > > > here.
> > > > > 
> > > > > > > @@ -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;
> > > > > > > +       }
> > > > > > 
> > > > > > The problem I see with this is that it might break musl's ability to
> > > > > > emulate the new
> > > > > > interface on top of the old (time32) one for linux-4.x and older
> > > > > > kernels, as the conversion
> > > > > > function is no longer stateless but has to know the negotiated
> > > > > > interface version.
> > > > > > 
> > > > > > It's probably fine as long as we can be sure that the 2.0.16+ API
> > > > > > version only gets
> > > > > > negotiated if both the kernel and user sides support it, and musl only emulates
> > > > > > the 2.0.15 API version from the current kernels.
> > > > > > 
> > > > > > I've tried to understand this part of musl's convert_ioctl_struct(), but I just
> > > > > > can't figure out whether it does the conversion based the on the layout that
> > > > > > is currently used in the kernel, or based on the layout we should have been
> > > > > > using, and would use with the above fix. Rich, can you help me here?
> > > > > 
> > > > > So, at this moment, I'm not sure whether we should correct the struct
> > > > > at all.  This will lead to yet more breakage, and basically the struct
> > > > > itself *works* -- the only bug is in 32bit compat handling in the
> > > > > kernel (again).
> > > > > 
> > > > > The below is a revised kernel patch (again untested), just correcting
> > > > > the behavior of 32bit compat mode.  32bit apps on 32bit kernel work
> > > > > fine as is, as well as 64bit apps on 64bit kernel.
> > > > 
> > > > I'm perfectly okay with this if Arnd is! It's probably the least
> > > > invasive and has the least long-term maintenance cost and fallout on
> > > > other projects.
> > > 
> > > OK, I'll submit a proper patch now, to be included in the next PR for
> > > 5.15-rc.  For further fixes, let's think carefully.
> > 
> > Am I correct in my understanding that the fix of keeping the "broken"
> > definition (and having the 64-bit kernel honor it for 32-bit binaries)
> > has been accepted?
> 
> Yes, as it was already set in stone, we accept the broken definition
> as is.
> 
> > Since musl's translation for pre-time64 kernels
> > seems to have been using the "non-broken" definition, I think
> > completing the fix requires a change in musl too.
> 
> Hm, musl translator contains the own definition of ioctl?
> 
> If so, we may reconsider about renumbering ioctls altogether.

No, I don't think so. The musl translator is to translate between the
time64 ioctl structures and the old time32 ones for the sake of
executing on an old kernel. Up til now, it has been broken comparably
to how 32-bit binaries running in compat mode on a 64-bit kernel were
broken: the code in musl translated the time64 structure to (and back
from) the time32 one assuming the intended padding. But the
application was using the actual kernel uapi struct where the padding
was (and still is) illogical. Thus, nothing was built with the wrong
ABI; it's only the musl-internal translation logic that was wrong (and
only pre-time64 kernels are affected).

The attached patch should fix it, I think.

Rich

[-- Attachment #2: musl_snd_pcm_mmap_control.diff --]
[-- Type: text/plain, Size: 898 bytes --]

diff --git a/src/misc/ioctl.c b/src/misc/ioctl.c
index 49282811..6d3e5095 100644
--- a/src/misc/ioctl.c
+++ b/src/misc/ioctl.c
@@ -6,6 +6,7 @@
 #include <stddef.h>
 #include <stdint.h>
 #include <string.h>
+#include <endian.h>
 #include "syscall.h"
 
 #define alignof(t) offsetof(struct { char c; t x; }, x)
@@ -90,7 +91,14 @@ static void convert_ioctl_struct(const struct ioctl_compat_map *map, char *old,
 		 * if another exception appears this needs changing. */
 		convert_ioctl_struct(map+1, old, new, dir);
 		convert_ioctl_struct(map+2, old+4, new+8, dir);
-		convert_ioctl_struct(map+3, old+68, new+72, dir);
+		int adj = BYTE_ORDER==BIG_ENDIAN ? 4 : 0;
+		if (dir==W) {
+			memcpy(old+68, new+72+adj, 4);
+			memcpy(old+72, new+72+4+2*adj, 4);
+		} else {
+			memcpy(new+72+adj, old+68, 4);
+			memcpy(new+72+4+2*adj, old+72, 4);
+		}
 		return;
 	}
 	for (int i=0; i < map->noffs; i++) {

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [musl] Re: [alsa-devel] [PATCH v7 8/9] ALSA: add new 32-bit layout for snd_pcm_mmap_status/control
  2021-10-18 15:08                                   ` Rich Felker
@ 2021-10-18 15:26                                     ` Arnd Bergmann
  2021-10-18 20:42                                       ` Rich Felker
  0 siblings, 1 reply; 24+ messages in thread
From: Arnd Bergmann @ 2021-10-18 15:26 UTC (permalink / raw)
  To: musl
  Cc: Takashi Iwai, Arnd Bergmann, Michael Forney,
	ALSA Development Mailing List, Takashi Iwai, Baolin Wang,
	y2038 Mailman List, Linux Kernel Mailing List, Mark Brown,
	Baolin Wang

On Mon, Oct 18, 2021 at 5:08 PM Rich Felker <dalias@libc.org> wrote:
> On Mon, Oct 18, 2021 at 04:58:03PM +0200, Takashi Iwai wrote:
> > On Mon, 18 Oct 2021 16:43:00 +0200, Rich Felker wrote:
>
> No, I don't think so. The musl translator is to translate between the
> time64 ioctl structures and the old time32 ones for the sake of
> executing on an old kernel. Up til now, it has been broken comparably
> to how 32-bit binaries running in compat mode on a 64-bit kernel were
> broken: the code in musl translated the time64 structure to (and back
> from) the time32 one assuming the intended padding. But the
> application was using the actual kernel uapi struct where the padding
> was (and still is) illogical. Thus, nothing was built with the wrong
> ABI; it's only the musl-internal translation logic that was wrong (and
> only pre-time64 kernels are affected).
>
> The attached patch should fix it, I think.
>
> + int adj = BYTE_ORDER==BIG_ENDIAN ? 4 : 0;
> + if (dir==W) {
> +     memcpy(old+68, new+72+adj, 4);
> +     memcpy(old+72, new+72+4+2*adj, 4);

I think that should be "new+72+4+3*adj": the "2*adj" would
be what the code does already for the originally intended
format.

        Arnd

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [musl] Re: [alsa-devel] [PATCH v7 8/9] ALSA: add new 32-bit layout for snd_pcm_mmap_status/control
  2021-10-18 15:26                                     ` Arnd Bergmann
@ 2021-10-18 20:42                                       ` Rich Felker
  2021-10-19 14:16                                         ` Rich Felker
  0 siblings, 1 reply; 24+ messages in thread
From: Rich Felker @ 2021-10-18 20:42 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: musl, Takashi Iwai, Michael Forney,
	ALSA Development Mailing List, Takashi Iwai, Baolin Wang,
	y2038 Mailman List, Linux Kernel Mailing List, Mark Brown,
	Baolin Wang

On Mon, Oct 18, 2021 at 05:26:35PM +0200, Arnd Bergmann wrote:
> On Mon, Oct 18, 2021 at 5:08 PM Rich Felker <dalias@libc.org> wrote:
> > On Mon, Oct 18, 2021 at 04:58:03PM +0200, Takashi Iwai wrote:
> > > On Mon, 18 Oct 2021 16:43:00 +0200, Rich Felker wrote:
> >
> > No, I don't think so. The musl translator is to translate between the
> > time64 ioctl structures and the old time32 ones for the sake of
> > executing on an old kernel. Up til now, it has been broken comparably
> > to how 32-bit binaries running in compat mode on a 64-bit kernel were
> > broken: the code in musl translated the time64 structure to (and back
> > from) the time32 one assuming the intended padding. But the
> > application was using the actual kernel uapi struct where the padding
> > was (and still is) illogical. Thus, nothing was built with the wrong
> > ABI; it's only the musl-internal translation logic that was wrong (and
> > only pre-time64 kernels are affected).
> >
> > The attached patch should fix it, I think.
> >
> > + int adj = BYTE_ORDER==BIG_ENDIAN ? 4 : 0;
> > + if (dir==W) {
> > +     memcpy(old+68, new+72+adj, 4);
> > +     memcpy(old+72, new+72+4+2*adj, 4);
> 
> I think that should be "new+72+4+3*adj": the "2*adj" would
> be what the code does already for the originally intended
> format.

Well for little endian either would work (because adj is 0 :) but yes
there are 3 such paddings before the second member on big endian, so
it should be 3.

Rich

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [musl] Re: [alsa-devel] [PATCH v7 8/9] ALSA: add new 32-bit layout for snd_pcm_mmap_status/control
  2021-10-18 20:42                                       ` Rich Felker
@ 2021-10-19 14:16                                         ` Rich Felker
  2021-10-19 14:23                                           ` Arnd Bergmann
  0 siblings, 1 reply; 24+ messages in thread
From: Rich Felker @ 2021-10-19 14:16 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: musl, Takashi Iwai, Michael Forney,
	ALSA Development Mailing List, Takashi Iwai, Baolin Wang,
	y2038 Mailman List, Linux Kernel Mailing List, Mark Brown,
	Baolin Wang

[-- Attachment #1: Type: text/plain, Size: 1704 bytes --]

On Mon, Oct 18, 2021 at 04:42:04PM -0400, Rich Felker wrote:
> On Mon, Oct 18, 2021 at 05:26:35PM +0200, Arnd Bergmann wrote:
> > On Mon, Oct 18, 2021 at 5:08 PM Rich Felker <dalias@libc.org> wrote:
> > > On Mon, Oct 18, 2021 at 04:58:03PM +0200, Takashi Iwai wrote:
> > > > On Mon, 18 Oct 2021 16:43:00 +0200, Rich Felker wrote:
> > >
> > > No, I don't think so. The musl translator is to translate between the
> > > time64 ioctl structures and the old time32 ones for the sake of
> > > executing on an old kernel. Up til now, it has been broken comparably
> > > to how 32-bit binaries running in compat mode on a 64-bit kernel were
> > > broken: the code in musl translated the time64 structure to (and back
> > > from) the time32 one assuming the intended padding. But the
> > > application was using the actual kernel uapi struct where the padding
> > > was (and still is) illogical. Thus, nothing was built with the wrong
> > > ABI; it's only the musl-internal translation logic that was wrong (and
> > > only pre-time64 kernels are affected).
> > >
> > > The attached patch should fix it, I think.
> > >
> > > + int adj = BYTE_ORDER==BIG_ENDIAN ? 4 : 0;
> > > + if (dir==W) {
> > > +     memcpy(old+68, new+72+adj, 4);
> > > +     memcpy(old+72, new+72+4+2*adj, 4);
> > 
> > I think that should be "new+72+4+3*adj": the "2*adj" would
> > be what the code does already for the originally intended
> > format.
> 
> Well for little endian either would work (because adj is 0 :) but yes
> there are 3 such paddings before the second member on big endian, so
> it should be 3.

How about this? It avoids open coding the logic and handles it as 2
4-byte substructures with endian-specific offsets.

Rich

[-- Attachment #2: snd_pcm_mmap_control_v2.diff --]
[-- Type: text/plain, Size: 1506 bytes --]

diff --git a/src/misc/ioctl.c b/src/misc/ioctl.c
index 49282811..35804f02 100644
--- a/src/misc/ioctl.c
+++ b/src/misc/ioctl.c
@@ -6,6 +6,7 @@
 #include <stddef.h>
 #include <stdint.h>
 #include <string.h>
+#include <endian.h>
 #include "syscall.h"
 
 #define alignof(t) offsetof(struct { char c; t x; }, x)
@@ -53,7 +54,7 @@ static const struct ioctl_compat_map compat_map[] = {
 	{ _IOWR('A', 0x23, char[136]), _IOWR('A', 0x23, char[132]), 0, WR, 1, 0 },
 	{ 0, 0, 4, WR, 1, 0 }, /* snd_pcm_sync_ptr (flags only) */
 	{ 0, 0, 32, WR, 1, OFFS(8,12,16,24,28) }, /* snd_pcm_mmap_status */
-	{ 0, 0, 8, WR, 1, OFFS(0,4) }, /* snd_pcm_mmap_control */
+	{ 0, 0, 4, WR, 1, 0 }, /* snd_pcm_mmap_control (each member) */
 
 	/* VIDIOC_QUERYBUF, VIDIOC_QBUF, VIDIOC_DQBUF, VIDIOC_PREPARE_BUF */
 	{ _IOWR('V',  9, new_misaligned(68)), _IOWR('V',  9, char[68]), 68, WR, 1, OFFS(20, 24) },
@@ -90,7 +91,11 @@ static void convert_ioctl_struct(const struct ioctl_compat_map *map, char *old,
 		 * if another exception appears this needs changing. */
 		convert_ioctl_struct(map+1, old, new, dir);
 		convert_ioctl_struct(map+2, old+4, new+8, dir);
-		convert_ioctl_struct(map+3, old+68, new+72, dir);
+		/* snd_pcm_mmap_control, special-cased due to kernel
+		 * type definition having been botched. */
+		int adj = BYTE_ORDER==BIG_ENDIAN ? 4 : 0;
+		convert_ioctl_struct(map+3, old+68, new+72+adj, dir);
+		convert_ioctl_struct(map+3, old+72, new+76+3*adj, dir);
 		return;
 	}
 	for (int i=0; i < map->noffs; i++) {

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [musl] Re: [alsa-devel] [PATCH v7 8/9] ALSA: add new 32-bit layout for snd_pcm_mmap_status/control
  2021-10-19 14:16                                         ` Rich Felker
@ 2021-10-19 14:23                                           ` Arnd Bergmann
  0 siblings, 0 replies; 24+ messages in thread
From: Arnd Bergmann @ 2021-10-19 14:23 UTC (permalink / raw)
  To: musl
  Cc: Arnd Bergmann, Takashi Iwai, Michael Forney,
	ALSA Development Mailing List, Takashi Iwai, Baolin Wang,
	y2038 Mailman List, Linux Kernel Mailing List, Mark Brown,
	Baolin Wang

On Tue, Oct 19, 2021 at 4:16 PM Rich Felker <dalias@libc.org> wrote:
> On Mon, Oct 18, 2021 at 04:42:04PM -0400, Rich Felker wrote:
> >
> > Well for little endian either would work (because adj is 0 :) but yes
> > there are 3 such paddings before the second member on big endian, so
> > it should be 3.
>
> How about this? It avoids open coding the logic and handles it as 2
> 4-byte substructures with endian-specific offsets.

Looks good to me.

      Arnd

^ permalink raw reply	[flat|nested] 24+ messages in thread

end of thread, other threads:[~2021-10-19 14:24 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20191211212025.1981822-1-arnd@arndb.de>
     [not found] ` <20191211212025.1981822-9-arnd@arndb.de>
     [not found]   ` <29QBMJU8DE71E.2YZSH8IHT5HMH@mforney.org>
     [not found]     ` <s5hpmsh9kdx.wl-tiwai@suse.de>
     [not found]       ` <CAK8P3a0K3XtjiszC3XWgG0L8+AgO+xUGr_KEAnb9a5GmyecoUQ@mail.gmail.com>
     [not found]         ` <s5hee8x9f92.wl-tiwai@suse.de>
2021-10-07 13:11           ` [musl] Re: [alsa-devel] [PATCH v7 8/9] ALSA: add new 32-bit layout for snd_pcm_mmap_status/control Arnd Bergmann
2021-10-07 15:33             ` Takashi Iwai
2021-10-07 16:06               ` Rich Felker
2021-10-07 16:18                 ` Takashi Iwai
2021-10-07 16:51                   ` Rich Felker
2021-10-08  8:43                     ` Takashi Iwai
2021-10-08  8:44                       ` Takashi Iwai
2021-10-08  9:24                       ` Arnd Bergmann
2021-10-08 11:11                         ` Takashi Iwai
2021-10-08 11:45                           ` Arnd Bergmann
2021-10-08 11:53                             ` Takashi Iwai
2021-10-08 12:13                               ` Arnd Bergmann
2021-10-08 12:07                           ` Rich Felker
2021-10-10  7:53                             ` Takashi Iwai
2021-10-18 14:43                               ` Rich Felker
2021-10-18 14:58                                 ` Takashi Iwai
2021-10-18 15:08                                   ` Rich Felker
2021-10-18 15:26                                     ` Arnd Bergmann
2021-10-18 20:42                                       ` Rich Felker
2021-10-19 14:16                                         ` Rich Felker
2021-10-19 14:23                                           ` Arnd Bergmann
2021-10-08 12:06                         ` Rich Felker
2021-10-08 12:37                           ` Arnd Bergmann
2021-10-08 17:20                             ` Rich Felker

Code repositories for project(s) associated with this public inbox

	https://git.vuxu.org/mirror/musl/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).