Bug#954294: __X32_SYSCALL_BIT being defined as UL constant breaks userspace
Andy Lutomirski writes: > On Wed, Apr 8, 2020 at 7:34 AM Thorsten Glaser wrote: >> asm/unistd_x32.h:#define __NR_mmap (__X32_SYSCALL_BIT + 9) >> >> This construct is, thankfully, still usable in something like >> #if (__NR_mmap > __NR_somethingelse) >> but as __X32_SYSCALL_BIT is no longer int its type also isn’t. >> >> Therefore I ask you to revert this change, bringing x32 closer >> to all other architectures. >> > > One might reasonably ask whether it makes sense for syscall nrs to be > signed at all. > > But regardless, this breaks userspace and we should fix it. I can > whip up a patch to split it into X32_SYSCALL_BIT (unsigned long) and > __X32_SYSCALL_BIT (uapi, int). Thomas, etc, does this seem > reasonable? (For those not following all the machinations, this > change caused some userspace build failures in libseccomp and/or > systemd for reasons that are vaguely silly.) Yes.
Bug#954294: __X32_SYSCALL_BIT being defined as UL constant breaks userspace
On Wed, 8 Apr 2020, Andy Lutomirski wrote:
> One might reasonably ask whether it makes sense for syscall nrs to be
> signed at all.
It doesn’t, but it’s probably this way for hysteric raisins.
> But regardless, this breaks userspace and we should fix it. I can
> whip up a patch to split it into X32_SYSCALL_BIT (unsigned long) and
> __X32_SYSCALL_BIT (uapi, int). Thomas, etc, does this seem
This would help with the issue, thanks.
> reasonable? (For those not following all the machinations, this
> change caused some userspace build failures in libseccomp and/or
> systemd for reasons that are vaguely silly.)
Not very silly:
printf("%d\n", __NR_mmap);
That with -Wformat and -Werror (or -Werror=format) rightfully warns,
as the compiler cannot, on x32 (where int=long), detect that, on
architectures where int≠long the constant is int.
Even worse, switching userspace to unsigned long globally would
completely hose this on LP64 architectures.
So this is, in my opinion, completely justified. (Disclaimer: I’m
not affiliated with systemd and not even running it except udev.)
bye,
//mirabilos
--
tarent solutions GmbH
Rochusstraße 2-4, D-53123 Bonn • http://www.tarent.de/
Tel: +49 228 54881-393 • Fax: +49 228 54881-235
HRB 5168 (AG Bonn) • USt-ID (VAT): DE122264941
Geschäftsführer: Dr. Stefan Barth, Kai Ebenrett, Boris Esser, Alexander Steeg
Bug#954294: __X32_SYSCALL_BIT being defined as UL constant breaks userspace
On Wed, Apr 8, 2020 at 7:34 AM Thorsten Glaser wrote: > > Hello, > > I’m writing to you because your name shows up on this: > > commit 45e29d119e9923ff14dfb840e3482bef1667bbfb > Author: Andy Lutomirski > Date: Wed Jul 3 13:34:05 2019 -0700 > > x86/syscalls: Make __X32_SYSCALL_BIT be unsigned long > > Currently, it's an int. This is bizarre. Fortunately, the code using it > still works: ~__X32_SYSCALL_BIT is also int, so, if nr is unsigned long, > then C kindly sign-extends the ~__X32_SYSCALL_BIT part, and it actually > results in the desired value. > > This is far more subtle than it deserves to be. Syscall numbers are, for > all practical purposes, unsigned long, so make __X32_SYSCALL_BIT be > unsigned long. > > Signed-off-by: Andy Lutomirski > Signed-off-by: Thomas Gleixner > Link: > https://lkml.kernel.org/r/99b0d83ad891c67105470a1a6b63243fd63a5061.1562185330.git.l...@kernel.org > > This commit changed an uapi header, breaking userspace. Long debugging > story (https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=954294 if you > are interested) short, it goes like this: > > libseccomp exposes an interface SCMP_SYS() which is designed to expand > to an int and be usable in cpp context. It redirects to the __NR_* > constants from . > > Example: SCMP_SYS(mmap) becomes __NR_mmap or __NR_mmap2 (depending on > the architecture). > > Now, most architectures define __NR_mmap as a mere integer number: > > asm/unistd_32.h:#define __NR_mmap 90 > asm/unistd_64.h:#define __NR_mmap 9 > > x32 differs: > > asm/unistd_x32.h:#define __NR_mmap (__X32_SYSCALL_BIT + 9) > > This construct is, thankfully, still usable in something like > #if (__NR_mmap > __NR_somethingelse) > but as __X32_SYSCALL_BIT is no longer int its type also isn’t. > > Therefore I ask you to revert this change, bringing x32 closer > to all other architectures. > One might reasonably ask whether it makes sense for syscall nrs to be signed at all. But regardless, this breaks userspace and we should fix it. I can whip up a patch to split it into X32_SYSCALL_BIT (unsigned long) and __X32_SYSCALL_BIT (uapi, int). Thomas, etc, does this seem reasonable? (For those not following all the machinations, this change caused some userspace build failures in libseccomp and/or systemd for reasons that are vaguely silly.) > > Syscall numbers are, for > > all practical purposes, unsigned long > > Yes, except for the one purpose of the C data type of the > syscall constants exposed to userspace, they are. > > Feel free to handle __X32_SYSCALL_BIT differently in the > kernel (although even there it *will* introduce subtle > differences from other architectures), but please keep it > as int as visible from userspace. > > Thanks in advance, > //mirabilos > > PS: Please keep both me *and* the Debian bug in Cc, but > feel free to forward to relevant lists and persons; > I’m unsure where exactly to write to about this. > > @bwh: commit 45e29d119e9923ff14dfb840e3482bef1667bbfb is > literally just this… > -#define __X32_SYSCALL_BIT 0x4000 > +#define __X32_SYSCALL_BIT 0x4000UL > … so can you please revert it in Debian in the meantime, > even if you said you won’t spend time on this? > -- > tarent solutions GmbH > Rochusstraße 2-4, D-53123 Bonn • http://www.tarent.de/ > Tel: +49 228 54881-393 • Fax: +49 228 54881-235 > HRB 5168 (AG Bonn) • USt-ID (VAT): DE122264941 > Geschäftsführer: Dr. Stefan Barth, Kai Ebenrett, Boris Esser, Alexander Steeg > > ** > > Mit der tarent Academy bieten wir auch Trainings und Schulungen in den > Bereichen Softwareentwicklung, Agiles Arbeiten und Zukunftstechnologien an. > > Besuchen Sie uns auf www.tarent.de/academy. Wir freuen uns auf Ihren Kontakt. > > **
Bug#954294: __X32_SYSCALL_BIT being defined as UL constant breaks userspace
Hello, I’m writing to you because your name shows up on this: commit 45e29d119e9923ff14dfb840e3482bef1667bbfb Author: Andy Lutomirski Date: Wed Jul 3 13:34:05 2019 -0700 x86/syscalls: Make __X32_SYSCALL_BIT be unsigned long Currently, it's an int. This is bizarre. Fortunately, the code using it still works: ~__X32_SYSCALL_BIT is also int, so, if nr is unsigned long, then C kindly sign-extends the ~__X32_SYSCALL_BIT part, and it actually results in the desired value. This is far more subtle than it deserves to be. Syscall numbers are, for all practical purposes, unsigned long, so make __X32_SYSCALL_BIT be unsigned long. Signed-off-by: Andy Lutomirski Signed-off-by: Thomas Gleixner Link: https://lkml.kernel.org/r/99b0d83ad891c67105470a1a6b63243fd63a5061.1562185330.git.l...@kernel.org This commit changed an uapi header, breaking userspace. Long debugging story (https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=954294 if you are interested) short, it goes like this: libseccomp exposes an interface SCMP_SYS() which is designed to expand to an int and be usable in cpp context. It redirects to the __NR_* constants from . Example: SCMP_SYS(mmap) becomes __NR_mmap or __NR_mmap2 (depending on the architecture). Now, most architectures define __NR_mmap as a mere integer number: asm/unistd_32.h:#define __NR_mmap 90 asm/unistd_64.h:#define __NR_mmap 9 x32 differs: asm/unistd_x32.h:#define __NR_mmap (__X32_SYSCALL_BIT + 9) This construct is, thankfully, still usable in something like #if (__NR_mmap > __NR_somethingelse) but as __X32_SYSCALL_BIT is no longer int its type also isn’t. Therefore I ask you to revert this change, bringing x32 closer to all other architectures. > Syscall numbers are, for > all practical purposes, unsigned long Yes, except for the one purpose of the C data type of the syscall constants exposed to userspace, they are. Feel free to handle __X32_SYSCALL_BIT differently in the kernel (although even there it *will* introduce subtle differences from other architectures), but please keep it as int as visible from userspace. Thanks in advance, //mirabilos PS: Please keep both me *and* the Debian bug in Cc, but feel free to forward to relevant lists and persons; I’m unsure where exactly to write to about this. @bwh: commit 45e29d119e9923ff14dfb840e3482bef1667bbfb is literally just this… -#define __X32_SYSCALL_BIT 0x4000 +#define __X32_SYSCALL_BIT 0x4000UL … so can you please revert it in Debian in the meantime, even if you said you won’t spend time on this? -- tarent solutions GmbH Rochusstraße 2-4, D-53123 Bonn • http://www.tarent.de/ Tel: +49 228 54881-393 • Fax: +49 228 54881-235 HRB 5168 (AG Bonn) • USt-ID (VAT): DE122264941 Geschäftsführer: Dr. Stefan Barth, Kai Ebenrett, Boris Esser, Alexander Steeg ** Mit der tarent Academy bieten wir auch Trainings und Schulungen in den Bereichen Softwareentwicklung, Agiles Arbeiten und Zukunftstechnologien an. Besuchen Sie uns auf www.tarent.de/academy. Wir freuen uns auf Ihren Kontakt. **
