x86: remove the x32 syscall bitmask from syscall_get_nr()

PATCH In review Paul Moore 4 participants 10 messages Feb 15, 2013 – Apr 3, 2013

Subsystems (2)

Stable branch x86

lore

Share & tools

lore

Paul Moore

author

2013-02-15 17:22 UTC #0 lore raw

PATCH 1/1 x86: remove the x32 syscall bitmask from syscall_get_nr()

Commit fca460f95e928bae373daa8295877b6905bc62b8 simplified the x32 implementation by creating a syscall bitmask, equal to 0x40000000, that could be applied to x32 syscalls such that the masked syscall number would be the same as a x86_64 syscall. While that patch was a nice way to simplify the code, it went a bit too far by adding the mask to syscall_get_nr(); returning the masked syscall numbers can cause confusion with callers that expect syscall numbers matching the x32 ABI, e.g. unmasked syscall numbers.

This patch fixes this by simply removing the mask from syscall_get_nr() while preserving the other changes from the original commit. While there are several syscall_get_nr() callers in the kernel, most simply check that the syscall number is greater than zero, in this case this patch will have no effect. Of those remaining callers, they appear to be few, seccomp and ftrace, and from my testing of seccomp without this patch the original commit definitely breaks things; the seccomp filter does not correctly filter the syscalls due to the difference in syscall numbers in the BPF filter and the value from syscall_get_nr(). Applying this patch restores the seccomp BPF filter functionality on x32.

I've tested this patch with the seccomp BPF filters as well as ftrace and everything looks reasonable to me; needless to say general usage seemed fine as well.

Signed-off-by Paul Moore <[email protected]>

Cc [email protected]

Cc Will Drewry <[email protected]>

Cc H. Peter Anvin <[email protected]>

---
 arch/x86/include/asm/syscall.h |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

arch/x86/include/asm/syscall.h +2 -2

index 1ace47b..2e188d6 100644
@@ -29,13 +29,13 @@ extern const unsigned long sys_call_table[];
  */
 static inline int syscall_get_nr(struct task_struct *task, struct pt_regs *regs)
 {
-	return regs->orig_ax & __SYSCALL_MASK;
+	return regs->orig_ax;
 }
 
 static inline void syscall_rollback(struct task_struct *task,
 				    struct pt_regs *regs)
 {
-	regs->ax = regs->orig_ax & __SYSCALL_MASK;
+	regs->ax = regs->orig_ax;
 }
 
 static inline long syscall_get_error(struct task_struct *task,

H. Peter Anvin

reviewer

2013-02-15 19:03 UTC #1 lore raw

Quoted 21 lines · Paul Moore

Commit fca460f95e928bae373daa8295877b6905bc62b8 simplified the x32 implementation by creating a syscall bitmask, equal to 0x40000000, that could be applied to x32 syscalls such that the masked syscall number would be the same as a x86_64 syscall. While that patch was a nice way to simplify the code, it went a bit too far by adding the mask to syscall_get_nr(); returning the masked syscall numbers can cause confusion with callers that expect syscall numbers matching the x32 ABI, e.g. unmasked syscall numbers.
This patch fixes this by simply removing the mask from syscall_get_nr() while preserving the other changes from the original commit. While there are several syscall_get_nr() callers in the kernel, most simply check that the syscall number is greater than zero, in this case this patch will have no effect. Of those remaining callers, they appear to be few, seccomp and ftrace, and from my testing of seccomp without this patch the original commit definitely breaks things; the seccomp filter does not correctly filter the syscalls due to the difference in syscall numbers in the BPF filter and the value from syscall_get_nr(). Applying this patch restores the seccomp BPF filter functionality on x32.

Quoted 4 lines

I've tested this patch with the seccomp BPF filters as well as ftrace and everything looks reasonable to me; needless to say general usage seemed fine as well.

Hi... it isn't 100% clear from the description if you have audited *all* the callers?

-hpa

Paul Moore

author

2013-02-15 20:52 UTC #2 lore raw

Quoted 22 lines · H. Peter Anvin

Quoted 21 lines · Paul Moore
Commit fca460f95e928bae373daa8295877b6905bc62b8 simplified the x32 implementation by creating a syscall bitmask, equal to 0x40000000, that could be applied to x32 syscalls such that the masked syscall number would be the same as a x86_64 syscall. While that patch was a nice way to simplify the code, it went a bit too far by adding the mask to syscall_get_nr(); returning the masked syscall numbers can cause confusion with callers that expect syscall numbers matching the x32 ABI, e.g. unmasked syscall numbers.
This patch fixes this by simply removing the mask from syscall_get_nr() while preserving the other changes from the original commit. While there are several syscall_get_nr() callers in the kernel, most simply check that the syscall number is greater than zero, in this case this patch will have no effect. Of those remaining callers, they appear to be few, seccomp and ftrace, and from my testing of seccomp without this patch the original commit definitely breaks things; the seccomp filter does not correctly filter the syscalls due to the difference in syscall numbers in the BPF filter and the value from syscall_get_nr(). Applying this patch restores the seccomp BPF filter functionality on x32.

Quoted 6 lines

Quoted 3 lines
I've tested this patch with the seccomp BPF filters as well as ftrace and everything looks reasonable to me; needless to say general usage seemed fine as well.
Hi... it isn't 100% clear from the description if you have audited *all* the callers?

I audited all of the syscall_get_nr() callers using the LXR at
http://lxr.free-electrons.com with the 3.7 sources. If you exclude all of the architecture dependent stuff that is non-x86 you arrive at the following list of callers:

* kernel/seccomp.c:seccomp_bpf_load()
This is where I noticed the problem, broken w/o the patch.

* lib/syscall.c:collect_syscall()/task_current_syscall()
The task_current_syscall() function is really only called by proc_pid_syscall() which displays the syscall number back to the user via a /proc entry, in which case this patch appears to correct a problem similar to what was seen with seccomp.

* kernel/trace/trace_syscalls.c:ftrace_syscall_enter()
* kernel/trace/trace_syscalls.c:ftrace_syscall_exit()
* kernel/trace/trace_syscalls.c:perf_syscall_enter()
* kernel/trace/trace_syscalls.c:perf_syscall_exit()
The ftrace/perf is the one user that I am least sure about, as noted above, I did some simple tests based on what I could find via Google but a quick review by someone who is more familiar with this code would be appreciated. I'm most concerned about the syscall_metadata bits ...

* include/trace/events/syscall.h
Another, what I assume, is a ftrace user; I'm assuming the patch is reasonable based on my testing, but once again further review would be appreciated.

* arch/x86/kernel/ptrace.c:putreg32()
* arch/x86/kernel/signal.c:handle_signal()
* arch/x86/kernel/signal.c:do_signal()
Simple grater than zero checks.

Signature · 2 lines

paul moore
security and virtualization @ redhat

Paul Moore

author

2013-02-26 20:58 UTC #3 lore raw

In reply to Paul Moore

Quoted 21 lines · Paul Moore

Commit fca460f95e928bae373daa8295877b6905bc62b8 simplified the x32 implementation by creating a syscall bitmask, equal to 0x40000000, that could be applied to x32 syscalls such that the masked syscall number would be the same as a x86_64 syscall. While that patch was a nice way to simplify the code, it went a bit too far by adding the mask to syscall_get_nr(); returning the masked syscall numbers can cause confusion with callers that expect syscall numbers matching the x32 ABI, e.g. unmasked syscall numbers.
This patch fixes this by simply removing the mask from syscall_get_nr() while preserving the other changes from the original commit. While there are several syscall_get_nr() callers in the kernel, most simply check that the syscall number is greater than zero, in this case this patch will have no effect. Of those remaining callers, they appear to be few, seccomp and ftrace, and from my testing of seccomp without this patch the original commit definitely breaks things; the seccomp filter does not correctly filter the syscalls due to the difference in syscall numbers in the BPF filter and the value from syscall_get_nr(). Applying this patch restores the seccomp BPF filter functionality on x32.

Quoted 3 lines

I've tested this patch with the seccomp BPF filters as well as ftrace and everything looks reasonable to me; needless to say general usage seemed fine as well.

I just wanted to check and see where things stood with this patch. I'm not overly concerned about how this problem is solved, just that it is solved. If someone else has a better approach that is fine with me; I'll even make the offer to do additional testing if needed.

-Paul

Quoted 28 lines

Signed-off-by Paul Moore <[email protected]>

Cc [email protected]

Cc Will Drewry <[email protected]>

Cc H. Peter Anvin <[email protected]>

---
 arch/x86/include/asm/syscall.h |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

arch/x86/include/asm/syscall.h +2 -2

index 1ace47b..2e188d6 100644
@@ -29,13 +29,13 @@ extern const unsigned long sys_call_table[];
  */
 static inline int syscall_get_nr(struct task_struct *task, struct pt_regs
 *regs) {
-	return regs->orig_ax & __SYSCALL_MASK;
+	return regs->orig_ax;
 }
 
 static inline void syscall_rollback(struct task_struct *task,
 				    struct pt_regs *regs)
 {
-	regs->ax = regs->orig_ax & __SYSCALL_MASK;
+	regs->ax = regs->orig_ax;
 }
 
 static inline long syscall_get_error(struct task_struct *task,

Signature · 2 lines

paul moore
security and virtualization @ redhat

Paul Moore

author

2013-03-15 21:16 UTC #4 lore raw

Quoted 22 lines · Paul Moore

Quoted 21 lines · Paul Moore
Commit fca460f95e928bae373daa8295877b6905bc62b8 simplified the x32 implementation by creating a syscall bitmask, equal to 0x40000000, that could be applied to x32 syscalls such that the masked syscall number would be the same as a x86_64 syscall. While that patch was a nice way to simplify the code, it went a bit too far by adding the mask to syscall_get_nr(); returning the masked syscall numbers can cause confusion with callers that expect syscall numbers matching the x32 ABI, e.g. unmasked syscall numbers.
This patch fixes this by simply removing the mask from syscall_get_nr() while preserving the other changes from the original commit. While there are several syscall_get_nr() callers in the kernel, most simply check that the syscall number is greater than zero, in this case this patch will have no effect. Of those remaining callers, they appear to be few, seccomp and ftrace, and from my testing of seccomp without this patch the original commit definitely breaks things; the seccomp filter does not correctly filter the syscalls due to the difference in syscall numbers in the BPF filter and the value from syscall_get_nr(). Applying this patch restores the seccomp BPF filter functionality on x32.

Quoted 8 lines

Quoted 3 lines
I've tested this patch with the seccomp BPF filters as well as ftrace and everything looks reasonable to me; needless to say general usage seemed fine as well.
I just wanted to check and see where things stood with this patch. I'm not overly concerned about how this problem is solved, just that it is solved. If someone else has a better approach that is fine with me; I'll even make the offer to do additional testing if needed.

Anyone? The seccomp filter bits are completely broken on x32 and I'd like to get this fixed, if not with this patch then something else - I'm more than happy to test/verify/etc whatever solution is deemed best ...

Quoted 37 lines

Signed-off-by Paul Moore <[email protected]>

Cc [email protected]

Cc Will Drewry <[email protected]>

Cc H. Peter Anvin <[email protected]>

---

 arch/x86/include/asm/syscall.h |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

(quoted diff) +0 -0

b/arch/x86/include/asm/syscall.h index 1ace47b..2e188d6 100644

arch/x86/include/asm/syscall.h +1 -1

@@ -29,13 +29,13 @@ extern const unsigned long sys_call_table[];
 
  */
 
 static inline int syscall_get_nr(struct task_struct *task, struct pt_regs
 
 *regs) {
-	return regs->orig_ax & __SYSCALL_MASK;
+	return regs->orig_ax;
 
 }
 
 static inline void syscall_rollback(struct task_struct *task,
 
 				    struct pt_regs *regs)

{

arch/x86/include/asm/syscall.h +1 -1

-	regs->ax = regs->orig_ax & __SYSCALL_MASK;
+	regs->ax = regs->orig_ax;
 
 }
 
 static inline long syscall_get_error(struct task_struct *task,

Signature · 2 lines

paul moore
security and virtualization @ redhat

H. Peter Anvin

reviewer

2013-03-15 22:00 UTC #5 lore raw

Quoted 27 lines · Paul Moore

Quoted 26 lines · Paul Moore
Quoted 24 lines · Paul Moore
Commit fca460f95e928bae373daa8295877b6905bc62b8 simplified the x32 implementation by creating a syscall bitmask, equal to 0x40000000, that could be applied to x32 syscalls such that the masked syscall number would be the same as a x86_64 syscall. While that patch was a nice way to simplify the code, it went a bit too far by adding the mask to syscall_get_nr(); returning the masked syscall numbers can cause confusion with callers that expect syscall numbers matching the x32 ABI, e.g. unmasked syscall numbers.
This patch fixes this by simply removing the mask from syscall_get_nr() while preserving the other changes from the original commit. While there are several syscall_get_nr() callers in the kernel, most simply check that the syscall number is greater than zero, in this case this patch will have no effect. Of those remaining callers, they appear to be few, seccomp and ftrace, and from my testing of seccomp without this patch the original commit definitely breaks things; the seccomp filter does not correctly filter the syscalls due to the difference in syscall numbers in the BPF filter and the value from syscall_get_nr(). Applying this patch restores the seccomp BPF filter functionality on x32.
I've tested this patch with the seccomp BPF filters as well as ftrace and everything looks reasonable to me; needless to say general usage seemed fine as well.

Quoted 9 lines

Quoted 4 lines
I just wanted to check and see where things stood with this patch. I'm not overly concerned about how this problem is solved, just that it is solved. If someone else has a better approach that is fine with me; I'll even make the offer to do additional testing if needed.
Anyone? The seccomp filter bits are completely broken on x32 and I'd like to get this fixed, if not with this patch then something else - I'm more than happy to test/verify/etc whatever solution is deemed best ...

Seems good to me -- H.J., do you seen any problem with this?

-hpa

Quoted 37 lines

Quoted 37 lines
Quoted 37 lines
Signed-off-by Paul Moore <[email protected]>
Cc [email protected]
Cc Will Drewry <[email protected]>
Cc H. Peter Anvin <[email protected]>
---
 arch/x86/include/asm/syscall.h |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
(quoted diff) +0 -0
b/arch/x86/include/asm/syscall.h index 1ace47b..2e188d6 100644
arch/x86/include/asm/syscall.h +1 -1
@@ -29,13 +29,13 @@ extern const unsigned long sys_call_table[];
 
  */
 
 static inline int syscall_get_nr(struct task_struct *task, struct pt_regs
 
 *regs) {
-	return regs->orig_ax & __SYSCALL_MASK;
+	return regs->orig_ax;
 
 }
 
 static inline void syscall_rollback(struct task_struct *task,
 
 				    struct pt_regs *regs)
{
arch/x86/include/asm/syscall.h +1 -1
-	regs->ax = regs->orig_ax & __SYSCALL_MASK;
+	regs->ax = regs->orig_ax;
 
 }
 
 static inline long syscall_get_error(struct task_struct *task,

H.J. Lu

2013-03-15 22:18 UTC #6 lore raw

Quoted 33 lines · H. Peter Anvin

Quoted 32 lines · Paul Moore
Quoted 30 lines · Paul Moore
Quoted 21 lines · Paul Moore
Commit fca460f95e928bae373daa8295877b6905bc62b8 simplified the x32 implementation by creating a syscall bitmask, equal to 0x40000000, that could be applied to x32 syscalls such that the masked syscall number would be the same as a x86_64 syscall. While that patch was a nice way to simplify the code, it went a bit too far by adding the mask to syscall_get_nr(); returning the masked syscall numbers can cause confusion with callers that expect syscall numbers matching the x32 ABI, e.g. unmasked syscall numbers.
This patch fixes this by simply removing the mask from syscall_get_nr() while preserving the other changes from the original commit. While there are several syscall_get_nr() callers in the kernel, most simply check that the syscall number is greater than zero, in this case this patch will have no effect. Of those remaining callers, they appear to be few, seccomp and ftrace, and from my testing of seccomp without this patch the original commit definitely breaks things; the seccomp filter does not correctly filter the syscalls due to the difference in syscall numbers in the BPF filter and the value from syscall_get_nr(). Applying this patch restores the seccomp BPF filter functionality on x32.
Quoted 3 lines
I've tested this patch with the seccomp BPF filters as well as ftrace and everything looks reasonable to me; needless to say general usage seemed fine as well.
I just wanted to check and see where things stood with this patch. I'm not overly concerned about how this problem is solved, just that it is solved. If someone else has a better approach that is fine with me; I'll even make the offer to do additional testing if needed.

Quoted 7 lines

Quoted 4 lines
Anyone? The seccomp filter bits are completely broken on x32 and I'd like to get this fixed, if not with this patch then something else - I'm more than happy to test/verify/etc whatever solution is deemed best ...
Seems good to me -- H.J., do you seen any problem with this?

It looks OK to me.

Signature · 1 line

H.J.

Paul Moore

author

2013-03-25 20:55 UTC #7 lore raw

Quoted 37 lines · H.J. Lu

Quoted 36 lines · H. Peter Anvin
Quoted 35 lines · Paul Moore
Quoted 33 lines · Paul Moore
Quoted 21 lines · Paul Moore
Commit fca460f95e928bae373daa8295877b6905bc62b8 simplified the x32 implementation by creating a syscall bitmask, equal to 0x40000000, that could be applied to x32 syscalls such that the masked syscall number would be the same as a x86_64 syscall. While that patch was a nice way to simplify the code, it went a bit too far by adding the mask to syscall_get_nr(); returning the masked syscall numbers can cause confusion with callers that expect syscall numbers matching the x32 ABI, e.g. unmasked syscall numbers.
This patch fixes this by simply removing the mask from syscall_get_nr() while preserving the other changes from the original commit. While there are several syscall_get_nr() callers in the kernel, most simply check that the syscall number is greater than zero, in this case this patch will have no effect. Of those remaining callers, they appear to be few, seccomp and ftrace, and from my testing of seccomp without this patch the original commit definitely breaks things; the seccomp filter does not correctly filter the syscalls due to the difference in syscall numbers in the BPF filter and the value from syscall_get_nr(). Applying this patch restores the seccomp BPF filter functionality on x32.
Quoted 3 lines
I've tested this patch with the seccomp BPF filters as well as ftrace and everything looks reasonable to me; needless to say general usage seemed fine as well.
I just wanted to check and see where things stood with this patch. I'm not
overly concerned about how this problem is solved, just that it is solved.
If someone else has a better approach that is fine with me; I'll even make
the offer to do additional testing if needed.

Quoted 7 lines

Quoted 5 lines
Quoted 3 lines
Anyone? The seccomp filter bits are completely broken on x32 and I'd like to get this fixed, if not with this patch then something else - I'm more than happy to test/verify/etc whatever solution is deemed best ...
Seems good to me -- H.J., do you seen any problem with this?
It looks OK to me.

Great, any chance of getting this fix merged for 3.9?

Signature · 2 lines

paul moore
security and virtualization @ redhat

Paul Moore

author

2013-04-02 21:34 UTC #8 lore raw

Quoted 48 lines · Paul Moore

Quoted 47 lines · H.J. Lu
Quoted 46 lines · H. Peter Anvin
Quoted 44 lines · Paul Moore
Quoted 30 lines · Paul Moore
Quoted 25 lines · Paul Moore
Commit fca460f95e928bae373daa8295877b6905bc62b8 simplified the x32 implementation by creating a syscall bitmask, equal to 0x40000000, that
could be applied to x32 syscalls such that the masked syscall number would be the same as a x86_64 syscall. While that patch was a nice way to simplify the code, it went a bit too far by adding the mask to syscall_get_nr(); returning the masked syscall numbers can cause confusion with callers that expect syscall numbers matching the x32 ABI, e.g. unmasked syscall numbers.
This patch fixes this by simply removing the mask from syscall_get_nr()
while preserving the other changes from the original commit. While there are several syscall_get_nr() callers in the kernel, most simply check that the syscall number is greater than zero, in this case this patch will have no effect. Of those remaining callers, they appear to be few, seccomp and ftrace, and from my testing of seccomp without this patch the original commit definitely breaks things; the seccomp filter does not correctly filter the syscalls due to the difference in
syscall numbers in the BPF filter and the value from syscall_get_nr().
Applying this patch restores the seccomp BPF filter functionality on x32.
Quoted 3 lines
I've tested this patch with the seccomp BPF filters as well as ftrace and everything looks reasonable to me; needless to say general usage seemed fine as well.
Quoted 8 lines
I just wanted to check and see where things stood with this patch. I'm
not
overly concerned about how this problem is solved, just that it is solved.
If someone else has a better approach that is fine with me; I'll even make
the offer to do additional testing if needed.
Anyone? The seccomp filter bits are completely broken on x32 and I'd like to get this fixed, if not with this patch then something else - I'm
more than happy to test/verify/etc whatever solution is deemed best ...

Quoted 5 lines

Quoted 3 lines
Quoted 1 line
Seems good to me -- H.J., do you seen any problem with this?
It looks OK to me.
Great, any chance of getting this fix merged for 3.9?

Just a ping to see where we stand on getting this patch merged. Just a reminder that SECCOMP_FILTER is completely broken on x32 and either needs this patch, or another one, to fix the regression.

Signature · 2 lines

paul moore
security and virtualization @ redhat

tip-bot for Paul Moore

merge bot

2013-04-03 00:17 UTC #9 lore raw

In reply to Paul Moore

x86: remove the x32 syscall bitmask from syscall_get_nr()

Commit-ID:  8b4b9f27e57584f3d90e0bb84cf800ad81cfe3a1
Gitweb:     http://git.kernel.org/tip/8b4b9f27e57584f3d90e0bb84cf800ad81cfe3a1
Author:     Paul Moore <[email protected]>
AuthorDate: Fri, 15 Feb 2013 12:21:43 -0500
Committer:  H. Peter Anvin <[email protected]>
CommitDate: Tue, 2 Apr 2013 14:38:09 -0700

x86: remove the x32 syscall bitmask from syscall_get_nr()

I've tested this patch with the seccomp BPF filters as well as ftrace and everything looks reasonable to me; needless to say general usage seemed fine as well.

Signed-off-by Paul Moore <[email protected]>

Link http://lkml.kernel.org/r/20130215172143.12549.10292.stgit@localhost open here

Cc <[email protected]>

Cc Will Drewry <[email protected]>

Cc H. Peter Anvin <[email protected]>

Signed-off-by H. Peter Anvin <[email protected]>

---
 arch/x86/include/asm/syscall.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

arch/x86/include/asm/syscall.h +2 -2

index 1ace47b..2e188d6 100644
@@ -29,13 +29,13 @@ extern const unsigned long sys_call_table[];
  */
 static inline int syscall_get_nr(struct task_struct *task, struct pt_regs *regs)
 {
-	return regs->orig_ax & __SYSCALL_MASK;
+	return regs->orig_ax;
 }
 
 static inline void syscall_rollback(struct task_struct *task,
 				    struct pt_regs *regs)
 {
-	regs->ax = regs->orig_ax & __SYSCALL_MASK;
+	regs->ax = regs->orig_ax;
 }
 
 static inline long syscall_get_error(struct task_struct *task,