Re: __X32_SYSCALL_BIT being defined as UL constant breaks userspace

Re: __X32_SYSCALL_BIT being defined as UL constant breaks userspace

[Andy Lutomirski]

On Wed, Apr 8, 2020 at 7:34 AM Thorsten Glaser <t.glaser@tarent.de> wrote:
>
> Hello,
>
> I’m writing to you because your name shows up on this:
>
> commit 45e29d119e9923ff14dfb840e3482bef1667bbfb
> Author: Andy Lutomirski <luto@kernel.org>
> 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 <luto@kernel.org>
>     Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
>     Link: https://lkml.kernel.org/r/99b0d83ad891c67105470a1a6b63243fd63a5061.1562185330.git.luto@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 <asm/unistd.h>.
>
> 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      0x40000000
>         +#define __X32_SYSCALL_BIT      0x40000000UL
>     … 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.
>
> **********

Re: __X32_SYSCALL_BIT being defined as UL constant breaks userspace

[Thomas Gleixner]

Andy Lutomirski <luto@kernel.org> writes:
> On Wed, Apr 8, 2020 at 7:34 AM Thorsten Glaser <t.glaser@tarent.de> 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.

Re: __X32_SYSCALL_BIT being defined as UL constant breaks userspace

[Thorsten Glaser]

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