Bug#954294: systemd: FTBFS on x32 due to format string errors, need explicit casts

classic Classic list List threaded Threaded
17 messages Options
Reply | Threaded
Open this post in threaded view
|

Bug#954294: systemd: FTBFS on x32 due to format string errors, need explicit casts

Thorsten Glaser
Source: systemd
Version: 245.2-1
Severity: important
Tags: upstream ftbfs
Justification: fails to build from source (but built successfully in the past), on non-release architecture

https://buildd.debian.org/status/fetch.php?pkg=systemd&arch=x32&ver=245.2-1&stamp=1584573299&raw=0

The failures seem to be in the testsuite (assuming the main code has
been built before it gets build) only. They boil down to multiple
instances of:

../src/test/test-seccomp.c:562:27: error: format ‘%d’ expects argument of type ‘int’, but argument 8 has type ‘long unsigned int’ [-Werror=format=]
  562 |                 log_debug("arch %s: SCMP_SYS(mmap) = %d", seccomp_arch_to_string(arch), SCMP_SYS(mmap));

Here, SCMP_SYS(mmap) is an unsigned long, probably because that’s
what the native size type is.

If this is an int on other architectures, casting it to int might
be safe, as sizeof(long) == sizeof(int) on x32, but the safer path
would be changing it to long:

        log_debug("arch %s: SCMP_SYS(mmap) = %ld", seccomp_arch_to_string(arch), (long)SCMP_SYS(mmap));

Similar for all other occurrences of SCMP_SYS, I guess.
Reply | Threaded
Open this post in threaded view
|

Bug#954294: systemd: FTBFS on x32 due to format string errors, need explicit casts

Thorsten Glaser-6
reassign 954294 libseccomp-dev
found 954294 2.4.3-1
affects 954294 src:systemd
thanks

> Here, SCMP_SYS(mmap) is an unsigned long, probably because that’s
> what the native size type is.

Nevermind, I found the real culprit. SCMP_SYS() is defined to return int.

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

**********

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.

**********

Reply | Threaded
Open this post in threaded view
|

Bug#954294: libseccomp-dev: API break: SCMP_SYS() is unsigned long (was Re: Bug#954294: systemd: FTBFS on x32 due to format string errors, need explicit casts)

Thorsten Glaser-6
retitle 954294 libseccomp-dev: API break: SCMP_SYS() is unsigned long
tags 954294 + patch
thanks

> Nevermind, I found the real culprit. SCMP_SYS() is defined to return int.

Patch attached. Please apply and upload, and forward this upstream.

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

**********

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.

**********

fix-SCMP_SYS.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Bug#954294: libseccomp-dev: API break: SCMP_SYS() is unsigned long (was Re: Bug#954294: systemd: FTBFS on x32 due to format string errors, need explicit casts)

Thorsten Glaser-6
On Thu, 19 Mar 2020, Thorsten Glaser wrote:

> Nevermind, I found the real culprit. SCMP_SYS() is defined to return int.

I’ll be uploading this to debian-ports’ unreleased repo to get the
builds going again. Full debdiff and build log attached.

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

**********

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.

**********

libseccomp_2.4.3-1+x32.1.debdiff (3K) Download Attachment
libseccomp_2.4.3-1+x32.1_x32.build.xz (24K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Bug#954294: libseccomp-dev: API break: SCMP_SYS() is unsigned long (was Re: Bug#954294: systemd: FTBFS on x32 due to format string errors, need explicit casts)

Thorsten Glaser-6
On Mon, 6 Apr 2020, Thorsten Glaser wrote:

> On Thu, 19 Mar 2020, Thorsten Glaser wrote:
>
> > Nevermind, I found the real culprit. SCMP_SYS() is defined to return int.
>
> I’ll be uploading this to debian-ports’ unreleased repo to get the
> builds going again. Full debdiff and build log attached.


>-#define SCMP_SYS(x)           (__SNR_##x)
>+#define SCMP_SYS(x)           ((int)__SNR_##x)

Ouch, this hasn’t worked:

../src/shared/seccomp-util.c: In function ‘seccomp_restrict_sxid’:                                              
../src/shared/seccomp-util.c:1977:5: error: missing binary operator before token "("                            
 1977 | #if SCMP_SYS(open) > 0                                                                                  
      |     ^~~~~~~~                                                                                            

Is what systemd does here even defined/supported?
How to even do that then :/

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

**********

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.

**********

Reply | Threaded
Open this post in threaded view
|

Bug#954294: libseccomp-dev: API break: SCMP_SYS() is unsigned long (was Re: Bug#954294: systemd: FTBFS on x32 due to format string errors, need explicit casts)

Thorsten Glaser-6
retitle 954294 linux: __X32_SYSCALL_BIT being defined as UL constant breaks userspace
reassign 954294 linux-libc-dev
found 954294 5.5.13-2
thanks

Dixi quod…

> >-#define SCMP_SYS(x)           (__SNR_##x)
> >+#define SCMP_SYS(x)           ((int)__SNR_##x)
>
> Ouch, this hasn’t worked:
>
> ../src/shared/seccomp-util.c: In function ‘seccomp_restrict_sxid’:                                              
> ../src/shared/seccomp-util.c:1977:5: error: missing binary operator before token "("                            
>  1977 | #if SCMP_SYS(open) > 0                                                                                  
>       |     ^~~~~~~~                                                                                            

Turns out that __X32_SYSCALL_BIT is OR’d into the __NR_* things
and defined, by default, as unsigned long constant.

/usr/include/x86_64-linux-gnux32/asm/unistd.h:#define __X32_SYSCALL_BIT 0x40000000UL

I’ve uploaded a workaround (attached); reassigning.

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

**********

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.

**********

libseccomp_2.4.3-1+x32.2.debdiff (2K) Download Attachment
libseccomp_2.4.3-1+x32.2_x32.build.xz (23K) Download Attachment
systemd_245.4-2_x32.build.xz (124K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Bug#954294: linux: __X32_SYSCALL_BIT being defined as UL constant breaks userspace (was Re: libseccomp-dev: API break: SCMP_SYS() is unsigned long (was Re: Bug#954294: systemd: FTBFS on x32 due to format string errors, need explicit casts))

Thorsten Glaser-6
Dear kernel team,

libseccomp uses the __NR_* constants from <asm/unistd.h> in its
macro SCMP_SYS() which is designed to return int.

However, on x32 some codes return unsigned long instead, breaking this.
The cause is that this…

> /usr/include/x86_64-linux-gnux32/asm/unistd.h:#define __X32_SYSCALL_BIT 0x40000000UL

… is OR’d into the numbers.

I’d like to propose the change…
-#define __X32_SYSCALL_BIT 0x40000000UL
+#define __X32_SYSCALL_BIT 0x40000000
… which should be safe to do as the number fits into signed int,
but must be checked against other users (especially in the kernel
itself I’d guess).

This should also technically be correct, since on all(?) other
architectures syscall numbers are int constants.

Please forward this to upstream.

Thanks,
//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

**********

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.

**********

Reply | Threaded
Open this post in threaded view
|

Bug#954294: libseccomp-dev: API break: SCMP_SYS() is unsigned long (was Re: Bug#954294: systemd: FTBFS on x32 due to format string errors, need explicit casts)

Michael Biebl-3
In reply to this post by Thorsten Glaser-6
Am 07.04.20 um 14:26 schrieb Thorsten Glaser:

> retitle 954294 linux: __X32_SYSCALL_BIT being defined as UL constant breaks userspace
> reassign 954294
> found 954294 5.5.13-2
> thanks
>
> Dixi quod…
>
>>> -#define SCMP_SYS(x)           (__SNR_##x)
>>> +#define SCMP_SYS(x)           ((int)__SNR_##x)
>>
>> Ouch, this hasn’t worked:
>>
>> ../src/shared/seccomp-util.c: In function ‘seccomp_restrict_sxid’:                                              
>> ../src/shared/seccomp-util.c:1977:5: error: missing binary operator before token "("                            
>>  1977 | #if SCMP_SYS(open) > 0                                                                                  
>>       |     ^~~~~~~~                                                                                            
>
> Turns out that __X32_SYSCALL_BIT is OR’d into the __NR_* things
> and defined, by default, as unsigned long constant.
>
> /usr/include/x86_64-linux-gnux32/asm/unistd.h:#define __X32_SYSCALL_BIT 0x40000000UL
>
> I’ve uploaded a workaround (attached); reassigning.
Is this workaround permanent or will systemd FTBFS again in the future?

If seccomp support on x32 is causing so much trouble, we can just as
well disable it in systemd for the time being by dropping libseccomp-dev
from Build-Depends.
Let me know what you prefer.

Michael


signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Bug#954294: libseccomp-dev: API break: SCMP_SYS() is unsigned long (was Re: Bug#954294: systemd: FTBFS on x32 due to format string errors, need explicit casts)

Michael Biebl-3

Am 08.04.20 um 09:56 schrieb Michael Biebl:
> If seccomp support on x32 is causing so much trouble, we can just as
> well disable it in systemd for the time being by dropping libseccomp-dev
> from Build-Depends

... on x32 only, of course.



signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Bug#954294: linux: __X32_SYSCALL_BIT being defined as UL constant breaks userspace (was Re: libseccomp-dev: API break: SCMP_SYS() is unsigned long (was Re: Bug#954294: systemd: FTBFS on x32 due to format string errors, need explicit casts))

Ben Hutchings-3
In reply to this post by Thorsten Glaser-6
On Tue, 2020-04-07 at 14:40 +0200, Thorsten Glaser wrote:

> Dear kernel team,
>
> libseccomp uses the __NR_* constants from <asm/unistd.h> in its
> macro SCMP_SYS() which is designed to return int.
>
> However, on x32 some codes return unsigned long instead, breaking this.
> The cause is that this…
>
> > /usr/include/x86_64-linux-gnux32/asm/unistd.h:#define __X32_SYSCALL_BIT 0x40000000UL
>
> … is OR’d into the numbers.
>
> I’d like to propose the change…
> -#define __X32_SYSCALL_BIT 0x40000000UL
> +#define __X32_SYSCALL_BIT 0x40000000
> … which should be safe to do as the number fits into signed int,
> but must be checked against other users (especially in the kernel
> itself I’d guess).
>
> This should also technically be correct, since on all(?) other
> architectures syscall numbers are int constants.
>
> Please forward this to upstream.
You should not expect me to spend time talking to upstream about non-
release architectures.  That is way down the priority list.

Ben.

--
Ben Hutchings
It's easier to fight for one's principles than to live up to them.



signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Bug#954294: libseccomp-dev: API break: SCMP_SYS() is unsigned long (was Re: Bug#954294: systemd: FTBFS on x32 due to format string errors, need explicit casts)

Thorsten Glaser-6
In reply to this post by Michael Biebl-3
On Wed, 8 Apr 2020, Michael Biebl wrote:

>> Is this workaround permanent or will systemd FTBFS again in the future?

It is not inherently permanent. If a new libseccomp version gets
uploaded it will pop back up. In these cases, I’ll most likely
notice it due to Multi-Arch skew (my x32 system has libudev1:i386
and libudev1:x32 installed, so when the former shows up in apt’s
output of “not updated” packages I’ll know), and it’s a matter of
maybe half an hour to bring the hack back, and I’ve got permissions
to give-back the systemd package so the buildds will build it.

>> If seccomp support on x32 is causing so much trouble, we can just as
>> well disable it in systemd for the time being by dropping libseccomp-dev
>> from Build-Depends.

My hope is to get this fixed in the kernel headers instead; it
seems like a straight-forward fix, aligns x32 more with other
architectures and seems to be the technically more correct solution,
plus other packages might be similarily affected (but don’t show
it as they don’t build with -Werror=format).

>... on x32 only, of course.

Yes, of course.

>> Let me know what you prefer.

For now, don’t take any action in systemd, and we’ll hope some
kernel developer picks it up, but thanks for the offer.

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

Reply | Threaded
Open this post in threaded view
|

Bug#954294: linux: __X32_SYSCALL_BIT being defined as UL constant breaks userspace (was Re: libseccomp-dev: API break: SCMP_SYS() is unsigned long (was Re: Bug#954294: systemd: FTBFS on x32 due to format string errors, need explicit casts))

Thorsten Glaser-6
In reply to this post by Ben Hutchings-3
On Wed, 8 Apr 2020, Ben Hutchings wrote:

> You should not expect me to spend time talking to upstream about non-
> release architectures.  That is way down the priority list.

DevRef §5.8.3.(6) is a “must”, but you’re lucky: it turns out that
this is a recent isolated change, so I can write to the persons in
that commit. (Will do so in a follow-up mail, so they’re not confu‐
sed by Debian specifics.)

The rationale for DevRef §5.8.3.(6) is that I, as user, would not
know whom to contact on the upstream side: especially with the
Linux kernel, there’s tons of maintainers, subsystems, mailing
lists, etc. and I’d not have an idea where to contact. (Luckily,
I *was* able to isolate it… this time. But I expect you to do at
least the talking, generally.)

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

**********

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.

**********

Reply | Threaded
Open this post in threaded view
|

Bug#954294: __X32_SYSCALL_BIT being defined as UL constant breaks userspace

Thorsten Glaser-6
In reply to this post by Thorsten Glaser
Hello,

I’m writing to you because your name shows up on this:

commit 45e29d119e9923ff14dfb840e3482bef1667bbfb
Author: Andy Lutomirski <[hidden email]>
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 <[hidden email]>
    Signed-off-by: Thomas Gleixner <[hidden email]>
    Link: https://lkml.kernel.org/r/99b0d83ad891c67105470a1a6b63243fd63a5061.1562185330.git.luto@...

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.

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

**********

Reply | Threaded
Open this post in threaded view
|

Bug#954294: __X32_SYSCALL_BIT being defined as UL constant breaks userspace

Andy Lutomirski-2
On Wed, Apr 8, 2020 at 7:34 AM Thorsten Glaser <[hidden email]> wrote:

>
> Hello,
>
> I’m writing to you because your name shows up on this:
>
> commit 45e29d119e9923ff14dfb840e3482bef1667bbfb
> Author: Andy Lutomirski <[hidden email]>
> 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 <[hidden email]>
>     Signed-off-by: Thomas Gleixner <[hidden email]>
>     Link: https://lkml.kernel.org/r/99b0d83ad891c67105470a1a6b63243fd63a5061.1562185330.git.luto@...
>
> 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.
>
> **********

Reply | Threaded
Open this post in threaded view
|

Bug#954294: __X32_SYSCALL_BIT being defined as UL constant breaks userspace

Thomas Gleixner
Andy Lutomirski <[hidden email]> writes:

> On Wed, Apr 8, 2020 at 7:34 AM Thorsten Glaser <[hidden email]> 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.

Reply | Threaded
Open this post in threaded view
|

Bug#954294: __X32_SYSCALL_BIT being defined as UL constant breaks userspace

Thorsten Glaser-6
In reply to this post by Andy Lutomirski-2
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

Reply | Threaded
Open this post in threaded view
|

Bug#954294: [tip: x86/urgent] x86/syscalls: Revert "x86/syscalls: Make __X32_SYSCALL_BIT be unsigned long" (fwd)

Thorsten Glaser-6
In reply to this post by Thorsten Glaser
tags 954294 + fixed-upstream
thanks

---------- Forwarded message ----------
From: tip-bot2 for Andy Lutomirski <[hidden email]>
Message-ID: <159050477082.17951.1414743958663563425.tip-bot2@tip-bot2>
To: [hidden email]
Cc: Thorsten Glaser <[hidden email]>, Andy Lutomirski <[hidden email]>,
    Borislav Petkov <[hidden email]>, [hidden email], x86 <[hidden email]>,
    LKML <[hidden email]>
Date: Tue, 26 May 2020 14:52:50 -0000
Subject: [tip: x86/urgent] x86/syscalls: Revert
    "x86/syscalls: Make __X32_SYSCALL_BIT be unsigned long"
Reply-To: [hidden email]

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     700d3a5a664df267f01ec8887fd2d8ff98f67e7f
Gitweb:        https://git.kernel.org/tip/700d3a5a664df267f01ec8887fd2d8ff98f67e7f
Author:        Andy Lutomirski <[hidden email]>
AuthorDate:    Fri, 08 May 2020 17:25:32 -07:00
Committer:     Borislav Petkov <[hidden email]>
CommitterDate: Tue, 26 May 2020 16:42:43 +02:00

x86/syscalls: Revert "x86/syscalls: Make __X32_SYSCALL_BIT be unsigned long"

Revert

  45e29d119e99 ("x86/syscalls: Make __X32_SYSCALL_BIT be unsigned long")

and add a comment to discourage someone else from making the same
mistake again.

It turns out that some user code fails to compile if __X32_SYSCALL_BIT
is unsigned long. See, for example [1] below.

 [ bp: Massage and do the same thing in the respective tools/ header. ]

Fixes: 45e29d119e99 ("x86/syscalls: Make __X32_SYSCALL_BIT be unsigned long")
Reported-by: Thorsten Glaser <[hidden email]>
Signed-off-by: Andy Lutomirski <[hidden email]>
Signed-off-by: Borislav Petkov <[hidden email]>
Cc: [hidden email]
Link: [1] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=954294
Link: https://lkml.kernel.org/r/92e55442b744a5951fdc9cfee10badd0a5f7f828.1588983892.git.luto@...
---
 arch/x86/include/uapi/asm/unistd.h       | 11 +++++++++--
 tools/arch/x86/include/uapi/asm/unistd.h |  2 +-
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/arch/x86/include/uapi/asm/unistd.h b/arch/x86/include/uapi/asm/unistd.h
index 196fdd0..be5e2e7 100644
--- a/arch/x86/include/uapi/asm/unistd.h
+++ b/arch/x86/include/uapi/asm/unistd.h
@@ -2,8 +2,15 @@
 #ifndef _UAPI_ASM_X86_UNISTD_H
 #define _UAPI_ASM_X86_UNISTD_H
 
-/* x32 syscall flag bit */
-#define __X32_SYSCALL_BIT 0x40000000UL
+/*
+ * x32 syscall flag bit.  Some user programs expect syscall NR macros
+ * and __X32_SYSCALL_BIT to have type int, even though syscall numbers
+ * are, for practical purposes, unsigned long.
+ *
+ * Fortunately, expressions like (nr & ~__X32_SYSCALL_BIT) do the right
+ * thing regardless.
+ */
+#define __X32_SYSCALL_BIT 0x40000000
 
 #ifndef __KERNEL__
 # ifdef __i386__
diff --git a/tools/arch/x86/include/uapi/asm/unistd.h b/tools/arch/x86/include/uapi/asm/unistd.h
index 196fdd0..30d7d04 100644
--- a/tools/arch/x86/include/uapi/asm/unistd.h
+++ b/tools/arch/x86/include/uapi/asm/unistd.h
@@ -3,7 +3,7 @@
 #define _UAPI_ASM_X86_UNISTD_H
 
 /* x32 syscall flag bit */
-#define __X32_SYSCALL_BIT 0x40000000UL
+#define __X32_SYSCALL_BIT 0x40000000
 
 #ifndef __KERNEL__
 # ifdef __i386__