Skip to contentSkip to conversation

x86/asm/64: do not clear high 32 bits of syscall number when CONFIG_X86_X32=y

PATCH In review Dmitry V. Levin 6 participants 13 messages Sep 12, 2017 – Sep 17, 2017

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 x86/asm/64: do not clear high 32 bits of syscall number when CONFIG_X86_X32=y

Before this change, CONFIG_X86_X32=y fastpath behaviour was different from slowpath:

$ gcc -xc -Wall -O2 - <<'EOF'
#include <unistd.h>
#include <asm/unistd.h>
int main(void) {
	unsigned long nr = ~0xffffffffUL | __NR_exit;
	return !!syscall(nr, 42, 1, 2, 3, 4, 5);
}
EOF
$ ./a.out; echo \$?=$?
$?=42
$ strace -enone ./a.out
syscall_18446744069414584380(0x2a, 0x1, 0x2, 0x3, 0x4, 0x5) = -1 (errno 38)
+++ exited with 1 +++

This change syncs CONFIG_X86_X32=y fastpath behaviour with the case when CONFIG_X86_X32 is not enabled.

Fixes fca460f95e92 ("x32: Handle the x32 system call flag")
---
 arch/x86/entry/entry_64.S | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)
arch/x86/entry/entry_64.S +3 -5
index 4916725..3bab6af 100644
@@ -185,12 +185,10 @@ entry_SYSCALL_64_fastpath:
 	 */
 	TRACE_IRQS_ON
 	ENABLE_INTERRUPTS(CLBR_NONE)
-#if __SYSCALL_MASK == ~0
-	cmpq	$__NR_syscall_max, %rax
-#else
-	andl	$__SYSCALL_MASK, %eax
-	cmpl	$__NR_syscall_max, %eax
+#if __SYSCALL_MASK != ~0
+	andq	$__SYSCALL_MASK, %rax
 #endif
+	cmpq	$__NR_syscall_max, %rax
 	ja	1f				/* return -ENOSYS (already in pt_regs->ax) */
 	movq	%r10, %rcx
 
Signature · 1 line
ldv
#1 lore raw
Quoted 3 lines · Dmitry V. Levin

Before this change, CONFIG_X86_X32=y fastpath behaviour was different from slowpath:

and even with this change they differ if CONFIG_X86_X32=n? do_syscall_64() does "nr & __SYSCALL_MASK" unconditionally, this clears the upper bits, no?

And why __SYSCALL_MASK is not "unsigned long" ? IOW, why do we want to silently ignore the upper bits in $rax ?

Or I am totally confused?

Oleg.

Quoted 46 lines
$ gcc -xc -Wall -O2 - <<'EOF'
#include <unistd.h>
#include <asm/unistd.h>
int main(void) {
	unsigned long nr = ~0xffffffffUL | __NR_exit;
	return !!syscall(nr, 42, 1, 2, 3, 4, 5);
}
EOF
$ ./a.out; echo \$?=$?
$?=42
$ strace -enone ./a.out
syscall_18446744069414584380(0x2a, 0x1, 0x2, 0x3, 0x4, 0x5) = -1 (errno 38)
+++ exited with 1 +++

This change syncs CONFIG_X86_X32=y fastpath behaviour with the case when CONFIG_X86_X32 is not enabled.

Fixes fca460f95e92 ("x32: Handle the x32 system call flag")
---
 arch/x86/entry/entry_64.S | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)
arch/x86/entry/entry_64.S +3 -5
index 4916725..3bab6af 100644
@@ -185,12 +185,10 @@ entry_SYSCALL_64_fastpath:
 	 */
 	TRACE_IRQS_ON
 	ENABLE_INTERRUPTS(CLBR_NONE)
-#if __SYSCALL_MASK == ~0
-	cmpq	$__NR_syscall_max, %rax
-#else
-	andl	$__SYSCALL_MASK, %eax
-	cmpl	$__NR_syscall_max, %eax
+#if __SYSCALL_MASK != ~0
+	andq	$__SYSCALL_MASK, %rax
 #endif
+	cmpq	$__NR_syscall_max, %rax
 	ja	1f				/* return -ENOSYS (already in pt_regs->ax) */
 	movq	%r10, %rcx
 
Signature · 1 line
ldv
#2 lore raw
Quoted 1 line · Oleg Nesterov

IOW, why do we want to silently ignore the upper bits in $rax ?

By the way, they are ignored elsewhere, in audit[1] or seccomp[2], for example.

[1] include/linux/audit.h
[2] include/uapi/linux/seccomp.h, definition of struct seccomp_data

Signature · 3 lines
Eugene Syromyatnikov
mailto:[email protected]
xmpp:esyr@jabber.{ru|org}
#3 lore raw
Quoted 7 lines · Eugene Syromyatnikov
Quoted 1 line · Oleg Nesterov

IOW, why do we want to silently ignore the upper bits in $rax ?

By the way, they are ignored elsewhere, in audit[1] or seccomp[2], for example.

[1] include/linux/audit.h
[2] include/uapi/linux/seccomp.h, definition of struct seccomp_data

Yes, unfortunately, they are ignored later, but that is another story.

Signature · 1 line
ldv
#4 lore raw
In reply to Oleg Nesterov
Quoted 6 lines · Oleg Nesterov
Quoted 3 lines · Dmitry V. Levin

Before this change, CONFIG_X86_X32=y fastpath behaviour was different from slowpath:

and even with this change they differ if CONFIG_X86_X32=n?

No, I don't think so.

Quoted 1 line

do_syscall_64() does "nr & __SYSCALL_MASK" unconditionally,

yes

Quoted 1 line

this clears the upper bits, no?

Why? As "nr" is of type "unsigned long" and __SYSCALL_MASK is either (~(__X32_SYSCALL_BIT)) or (~0), that is, an integer with the sign bit set, in "nr & __SYSCALL_MASK" expression __SYSCALL_MASK is sign-extended to unsigned long. When __SYSCALL_MASK is defined to (~0),
"nr & __SYSCALL_MASK" is optimized to "nr" at compilation time:

$ echo 'unsigned long foo(unsigned long nr) { return nr & (~0); }' |
  gcc -Wall -O2 -xc -S -o - - |
  sed -n '/cfi_/,/cfi_/p'
	.cfi_startproc
	movq	%rdi, %rax
	ret
	.cfi_endproc
Quoted 2 lines

And why __SYSCALL_MASK is not "unsigned long" ? IOW, why do we want to silently ignore the upper bits in $rax ?

__SYSCALL_MASK is "int" but it is being sign-extended to unsigned long in all (two) places of arch/x86/entry/common.c where it is used.

Quoted 1 line

Or I am totally confused?

The thing looks like it was designed to confuse people.

Signature · 1 line
ldv
#5 lore raw
Quoted 11 lines · Dmitry V. Levin
Quoted 1 line · Oleg Nesterov

do_syscall_64() does "nr & __SYSCALL_MASK" unconditionally,

yes

Quoted 1 line

this clears the upper bits, no?

Why? As "nr" is of type "unsigned long" and __SYSCALL_MASK is either (~(__X32_SYSCALL_BIT)) or (~0), that is, an integer with the sign bit set,

Yes, thanks for correcting me, it is "int" but somehow I thought it is "unsigned int".

Oleg.

maintainer
#6 lore raw
In reply to Dmitry V. Levin
Quoted 17 lines · Dmitry V. Levin

Before this change, CONFIG_X86_X32=y fastpath behaviour was different from slowpath:

$ gcc -xc -Wall -O2 - <<'EOF'
#include <unistd.h>
#include <asm/unistd.h>
int main(void) {
        unsigned long nr = ~0xffffffffUL | __NR_exit;
        return !!syscall(nr, 42, 1, 2, 3, 4, 5);
}
EOF
$ ./a.out; echo \$?=$?
$?=42
$ strace -enone ./a.out
syscall_18446744069414584380(0x2a, 0x1, 0x2, 0x3, 0x4, 0x5) = -1 (errno 38)
+++ exited with 1 +++
Quoted 2 lines

This change syncs CONFIG_X86_X32=y fastpath behaviour with the case when CONFIG_X86_X32 is not enabled.

Do you see real brokenness here, or is it just weird?

Quoted 15 lines
Fixes fca460f95e92 ("x32: Handle the x32 system call flag")
---
 arch/x86/entry/entry_64.S | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)
arch/x86/entry/entry_64.S +0 -0
index 4916725..3bab6af 100644
@@ -185,12 +185,10 @@ entry_SYSCALL_64_fastpath:
         */
        TRACE_IRQS_ON

arch/x86/entry/entry_64.S entry_SYSCALL_64_fastpath: · line 191

Quoted 10 lines
arch/x86/entry/entry_64.S +3 -5
        ENABLE_INTERRUPTS(CLBR_NONE)
-#if __SYSCALL_MASK == ~0
-       cmpq    $__NR_syscall_max, %rax
-#else
-       andl    $__SYSCALL_MASK, %eax
-       cmpl    $__NR_syscall_max, %eax
+#if __SYSCALL_MASK != ~0
+       andq    $__SYSCALL_MASK, %rax
 #endif
+       cmpq    $__NR_syscall_max, %rax

I don't know much about x32 userspace, but there's an argument that the high bits *should* be masked off if the x32 bit is set. Of course, that's slower, but it could be done without performance loss, I think.

--Andy

#7 lore raw
Quoted 18 lines · Andy Lutomirski
Quoted 17 lines · Dmitry V. Levin

Before this change, CONFIG_X86_X32=y fastpath behaviour was different from slowpath:

$ gcc -xc -Wall -O2 - <<'EOF'
#include <unistd.h>
#include <asm/unistd.h>
int main(void) {
        unsigned long nr = ~0xffffffffUL | __NR_exit;
        return !!syscall(nr, 42, 1, 2, 3, 4, 5);
}
EOF
$ ./a.out; echo \$?=$?
$?=42
$ strace -enone ./a.out
syscall_18446744069414584380(0x2a, 0x1, 0x2, 0x3, 0x4, 0x5) = -1 (errno 38)
+++ exited with 1 +++
Quoted 4 lines
Quoted 2 lines

This change syncs CONFIG_X86_X32=y fastpath behaviour with the case when CONFIG_X86_X32 is not enabled.

Do you see real brokenness here, or is it just weird?

It's definitely broken. A syscall should be either valid or invalid regardless of implementation peculiarities like fastpath vs slowpath.

Quoted 19 lines
Quoted 19 lines
Fixes fca460f95e92 ("x32: Handle the x32 system call flag")
---
 arch/x86/entry/entry_64.S | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)
arch/x86/entry/entry_64.S +0 -4
index 4916725..3bab6af 100644
@@ -185,12 +185,10 @@ entry_SYSCALL_64_fastpath:
         */
        TRACE_IRQS_ON
        ENABLE_INTERRUPTS(CLBR_NONE)
-#if __SYSCALL_MASK == ~0
-       cmpq    $__NR_syscall_max, %rax
-#else
-       andl    $__SYSCALL_MASK, %eax
Quoted 8 lines

arch/x86/entry/entry_64.S entry_SYSCALL_64_fastpath: · line 191

Quoted 5 lines
arch/x86/entry/entry_64.S +3 -1
-       cmpl    $__NR_syscall_max, %eax
+#if __SYSCALL_MASK != ~0
+       andq    $__SYSCALL_MASK, %rax
 #endif
+       cmpq    $__NR_syscall_max, %rax

I don't know much about x32 userspace, but there's an argument that the high bits *should* be masked off if the x32 bit is set.

Why?

Signature · 1 line
ldv
maintainer
#8 lore raw
Quoted 55 lines · Dmitry V. Levin
Quoted 18 lines · Andy Lutomirski
Quoted 17 lines · Dmitry V. Levin

Before this change, CONFIG_X86_X32=y fastpath behaviour was different from slowpath:

$ gcc -xc -Wall -O2 - <<'EOF'
#include <unistd.h>
#include <asm/unistd.h>
int main(void) {
        unsigned long nr = ~0xffffffffUL | __NR_exit;
        return !!syscall(nr, 42, 1, 2, 3, 4, 5);
}
EOF
$ ./a.out; echo \$?=$?
$?=42
$ strace -enone ./a.out
syscall_18446744069414584380(0x2a, 0x1, 0x2, 0x3, 0x4, 0x5) = -1 (errno 38)
+++ exited with 1 +++
Quoted 4 lines
Quoted 2 lines

This change syncs CONFIG_X86_X32=y fastpath behaviour with the case when CONFIG_X86_X32 is not enabled.

Do you see real brokenness here, or is it just weird?

It's definitely broken. A syscall should be either valid or invalid regardless of implementation peculiarities like fastpath vs slowpath.

Quoted 19 lines
Quoted 19 lines
Fixes fca460f95e92 ("x32: Handle the x32 system call flag")
---
 arch/x86/entry/entry_64.S | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)
arch/x86/entry/entry_64.S +0 -4
index 4916725..3bab6af 100644
@@ -185,12 +185,10 @@ entry_SYSCALL_64_fastpath:
         */
        TRACE_IRQS_ON
        ENABLE_INTERRUPTS(CLBR_NONE)
-#if __SYSCALL_MASK == ~0
-       cmpq    $__NR_syscall_max, %rax
-#else
-       andl    $__SYSCALL_MASK, %eax
Quoted 8 lines

arch/x86/entry/entry_64.S entry_SYSCALL_64_fastpath: · line 191

Quoted 5 lines
arch/x86/entry/entry_64.S +3 -1
-       cmpl    $__NR_syscall_max, %eax
+#if __SYSCALL_MASK != ~0
+       andq    $__SYSCALL_MASK, %rax
 #endif
+       cmpq    $__NR_syscall_max, %rax

I don't know much about x32 userspace, but there's an argument that the high bits *should* be masked off if the x32 bit is set.

Quoted 1 line

Why?

Because it always worked that way.

That being said, I'd be okay with applying your patch and seeing whether anything breaks. Ingo?

Quoted 4 lines

--
ldv

#9 lore raw
Quoted 21 lines · Andy Lutomirski
Quoted 21 lines
Quoted 20 lines
Quoted 7 lines
arch/x86/entry/entry_64.S +0 -0
index 4916725..3bab6af 100644
@@ -185,12 +185,10 @@ entry_SYSCALL_64_fastpath:
         */
        TRACE_IRQS_ON

arch/x86/entry/entry_64.S entry_SYSCALL_64_fastpath: · line 191

Quoted 10 lines
arch/x86/entry/entry_64.S +3 -5
        ENABLE_INTERRUPTS(CLBR_NONE)
-#if __SYSCALL_MASK == ~0
-       cmpq    $__NR_syscall_max, %rax
-#else
-       andl    $__SYSCALL_MASK, %eax
-       cmpl    $__NR_syscall_max, %eax
+#if __SYSCALL_MASK != ~0
+       andq    $__SYSCALL_MASK, %rax
 #endif
+       cmpq    $__NR_syscall_max, %rax

I don't know much about x32 userspace, but there's an argument that the high bits *should* be masked off if the x32 bit is set.

Quoted 6 lines
Quoted 1 line

Why?

Because it always worked that way.

That being said, I'd be okay with applying your patch and seeing whether anything breaks. Ingo?

So I believe this was introduced with x32 as a 'fresh, modern syscall ABI' behavioral aspect, because we wanted to protect the overly complex syscall entry code from 'weird' input values. IIRC there was an old bug where we'd overflow the syscall table in certain circumstances ...

But our new, redesigned entry code is a lot less complex, a lot more readable and a lot more maintainable (not to mention a lot more robust), so if invalid RAX values with high bits set get reliably turned into -ENOSYS or such then I'd not mind the patch per se either, as a general consistency improvement.

Of course if something in x32 user-land breaks then this turns into an ABI and we have to reintroduce this aspect, as a quirk :-/

It should also improve x32 syscall performance a tiny bit, right? So might be worth a try on various grounds.

( Another future advantage would be that _maybe_ we could use the high RAX
component as an extra (64-bit only) special argument of sorts. Not that I can think of any such use right now. )

Thanks,

Ingo

#10 lore raw
Quoted 63 lines · Ingo Molnar
Quoted 1 line · Andy Lutomirski
Quoted 1 line
Quoted 1 line

Review

Quoted 1 line
(quoted diff) +0 -0

b/arch/x86/entry/entry_64.S

Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines

index 4916725..3bab6af 100644

arch/x86/entry/entry_64.S +0 -3
@@ -185,12 +185,10 @@ entry_SYSCALL_64_fastpath:
         */
        TRACE_IRQS_ON
        ENABLE_INTERRUPTS(CLBR_NONE)
-#if __SYSCALL_MASK == ~0
-       cmpq    $__NR_syscall_max, %rax
-#else
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines

arch/x86/entry/entry_64.S entry_SYSCALL_64_fastpath: · line 191

Quoted 6 lines
arch/x86/entry/entry_64.S +3 -2
-       andl    $__SYSCALL_MASK, %eax
-       cmpl    $__NR_syscall_max, %eax
+#if __SYSCALL_MASK != ~0
+       andq    $__SYSCALL_MASK, %rax
 #endif
+       cmpq    $__NR_syscall_max, %rax

I don't know much about x32 userspace, but there's an argument

that

Quoted 8 lines
Quoted 3 lines
Quoted 1 line

the high bits *should* be masked off if the x32 bit is set.

Why?

Because it always worked that way.

That being said, I'd be okay with applying your patch and seeing whether anything breaks. Ingo?

So I believe this was introduced with x32 as a 'fresh, modern syscall ABI'
behavioral aspect, because we wanted to protect the overly complex syscall entry
code from 'weird' input values. IIRC there was an old bug where we'd overflow the
syscall table in certain circumstances ...

But our new, redesigned entry code is a lot less complex, a lot more readable and
a lot more maintainable (not to mention a lot more robust), so if invalid RAX
values with high bits set get reliably turned into -ENOSYS or such then I'd not
mind the patch per se either, as a general consistency improvement.

Of course if something in x32 user-land breaks then this turns into an ABI and we
have to reintroduce this aspect, as a quirk :-/

It should also improve x32 syscall performance a tiny bit, right? So might be
worth a try on various grounds.

( Another future advantage would be that _maybe_ we could use the high RAX
component as an extra (64-bit only) special argument of sorts. Not that I can
think of any such use right now. )

Quoted 3 lines

Thanks,

Ingo

x32 should be sharing the native 64-but entry code, by design. We have had the latter mask the upper 32 bits, so we should do that for x32 as well.

There seems to be little if no motivation to ever change both these ABIs.

Signature · 1 line
Sent from my Android device with K-9 Mail. Please excuse my brevity.
#11 lore raw
Quoted 64 lines · On Thu, Sep 14, 2017 at 10:46:37PM -0700, [email protected] wrote:
Quoted 63 lines · Ingo Molnar
Quoted 1 line · Andy Lutomirski
Quoted 1 line
Quoted 1 line

Review

Quoted 1 line
(quoted diff) +0 -0

b/arch/x86/entry/entry_64.S

Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines

index 4916725..3bab6af 100644

arch/x86/entry/entry_64.S +0 -3
@@ -185,12 +185,10 @@ entry_SYSCALL_64_fastpath:
         */
        TRACE_IRQS_ON
        ENABLE_INTERRUPTS(CLBR_NONE)
-#if __SYSCALL_MASK == ~0
-       cmpq    $__NR_syscall_max, %rax
-#else
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines

arch/x86/entry/entry_64.S entry_SYSCALL_64_fastpath: · line 191

Quoted 6 lines
arch/x86/entry/entry_64.S +3 -2
-       andl    $__SYSCALL_MASK, %eax
-       cmpl    $__NR_syscall_max, %eax
+#if __SYSCALL_MASK != ~0
+       andq    $__SYSCALL_MASK, %rax
 #endif
+       cmpq    $__NR_syscall_max, %rax

I don't know much about x32 userspace, but there's an argument

that

Quoted 8 lines
Quoted 3 lines
Quoted 1 line

the high bits *should* be masked off if the x32 bit is set.

Why?

Because it always worked that way.

That being said, I'd be okay with applying your patch and seeing whether anything breaks. Ingo?

So I believe this was introduced with x32 as a 'fresh, modern syscall ABI'
behavioral aspect, because we wanted to protect the overly complex syscall entry
code from 'weird' input values. IIRC there was an old bug where we'd overflow the
syscall table in certain circumstances ...

But our new, redesigned entry code is a lot less complex, a lot more readable and
a lot more maintainable (not to mention a lot more robust), so if invalid RAX
values with high bits set get reliably turned into -ENOSYS or such then I'd not
mind the patch per se either, as a general consistency improvement.

Of course if something in x32 user-land breaks then this turns into an ABI and we
have to reintroduce this aspect, as a quirk :-/

It should also improve x32 syscall performance a tiny bit, right? So might be
worth a try on various grounds.

( Another future advantage would be that _maybe_ we could use the high RAX
component as an extra (64-bit only) special argument of sorts. Not that I can
think of any such use right now. )

Quoted 6 lines
Quoted 3 lines

Thanks,

Ingo

x32 should be sharing the native 64-but entry code, by design. We have had the latter mask the upper 32 bits,

There must be some misunderstanding on your side: the clearing of the upper 32 bits of 64-bit syscall numbers currently happens if and only if fastpath and CONFIG_X86_X32_ABI is enabled.

Signature · 1 line
ldv
#12 lore raw
In reply to Ingo Molnar
Quoted 63 lines · Ingo Molnar
Quoted 1 line · Andy Lutomirski
Quoted 1 line
Quoted 1 line

Review

Quoted 1 line
(quoted diff) +0 -0

b/arch/x86/entry/entry_64.S

Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines

index 4916725..3bab6af 100644

arch/x86/entry/entry_64.S +0 -3
@@ -185,12 +185,10 @@ entry_SYSCALL_64_fastpath:
         */
        TRACE_IRQS_ON
        ENABLE_INTERRUPTS(CLBR_NONE)
-#if __SYSCALL_MASK == ~0
-       cmpq    $__NR_syscall_max, %rax
-#else
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines

arch/x86/entry/entry_64.S entry_SYSCALL_64_fastpath: · line 191

Quoted 6 lines
arch/x86/entry/entry_64.S +3 -2
-       andl    $__SYSCALL_MASK, %eax
-       cmpl    $__NR_syscall_max, %eax
+#if __SYSCALL_MASK != ~0
+       andq    $__SYSCALL_MASK, %rax
 #endif
+       cmpq    $__NR_syscall_max, %rax

I don't know much about x32 userspace, but there's an argument

that

Quoted 8 lines
Quoted 3 lines
Quoted 1 line

the high bits *should* be masked off if the x32 bit is set.

Why?

Because it always worked that way.

That being said, I'd be okay with applying your patch and seeing whether anything breaks. Ingo?

So I believe this was introduced with x32 as a 'fresh, modern syscall ABI'
behavioral aspect, because we wanted to protect the overly complex syscall entry
code from 'weird' input values. IIRC there was an old bug where we'd overflow the
syscall table in certain circumstances ...

But our new, redesigned entry code is a lot less complex, a lot more readable and
a lot more maintainable (not to mention a lot more robust), so if invalid RAX
values with high bits set get reliably turned into -ENOSYS or such then I'd not
mind the patch per se either, as a general consistency improvement.

Of course if something in x32 user-land breaks then this turns into an ABI and we
have to reintroduce this aspect, as a quirk :-/

It should also improve x32 syscall performance a tiny bit, right? So might be
worth a try on various grounds.

( Another future advantage would be that _maybe_ we could use the high RAX
component as an extra (64-bit only) special argument of sorts. Not that I can
think of any such use right now. )

Quoted 3 lines

Thanks,

Ingo

If the consensus is that we should change the x86-64 ABI then we should change the x32 ABI to match, though.

Signature · 1 line
Sent from my Android device with K-9 Mail. Please excuse my brevity.