All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] x86: remove the x32 syscall bitmask from syscall_get_nr()
@ 2013-02-15 17:21 Paul Moore
  2013-02-15 19:02 ` H. Peter Anvin
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Paul Moore @ 2013-02-15 17:21 UTC (permalink / raw)
  To: x86, linux-kernel; +Cc: coreyb, wad, hpa

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 <pmoore@redhat.com>
Cc: stable@vger.kernel.org
Cc: Will Drewry <wad@chromium.org>
Cc: H. Peter Anvin <hpa@zytor.com>
---
 arch/x86/include/asm/syscall.h |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/syscall.h b/arch/x86/include/asm/syscall.h
index 1ace47b..2e188d6 100644
--- a/arch/x86/include/asm/syscall.h
+++ b/arch/x86/include/asm/syscall.h
@@ -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,


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] x86: remove the x32 syscall bitmask from syscall_get_nr()
  2013-02-15 17:21 [PATCH] x86: remove the x32 syscall bitmask from syscall_get_nr() Paul Moore
@ 2013-02-15 19:02 ` H. Peter Anvin
  2013-02-15 20:52   ` Paul Moore
  2013-02-26 20:58 ` Paul Moore
  2013-04-03  0:17 ` [tip:x86/urgent] " tip-bot for Paul Moore
  2 siblings, 1 reply; 10+ messages in thread
From: H. Peter Anvin @ 2013-02-15 19:02 UTC (permalink / raw)
  To: Paul Moore; +Cc: x86, linux-kernel, coreyb, wad

On 02/15/2013 09:21 AM, Paul Moore wrote:
> 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.
> 

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

	-hpa



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] x86: remove the x32 syscall bitmask from syscall_get_nr()
  2013-02-15 19:02 ` H. Peter Anvin
@ 2013-02-15 20:52   ` Paul Moore
  0 siblings, 0 replies; 10+ messages in thread
From: Paul Moore @ 2013-02-15 20:52 UTC (permalink / raw)
  To: H. Peter Anvin; +Cc: x86, linux-kernel, coreyb, wad

On Friday, February 15, 2013 11:02:49 AM H. Peter Anvin wrote:
> On 02/15/2013 09:21 AM, Paul Moore wrote:
> > 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.
> 
> 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.

-- 
paul moore
security and virtualization @ redhat


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] x86: remove the x32 syscall bitmask from syscall_get_nr()
  2013-02-15 17:21 [PATCH] x86: remove the x32 syscall bitmask from syscall_get_nr() Paul Moore
  2013-02-15 19:02 ` H. Peter Anvin
@ 2013-02-26 20:58 ` Paul Moore
  2013-03-15 21:15   ` Paul Moore
  2013-04-03  0:17 ` [tip:x86/urgent] " tip-bot for Paul Moore
  2 siblings, 1 reply; 10+ messages in thread
From: Paul Moore @ 2013-02-26 20:58 UTC (permalink / raw)
  To: x86, hpa; +Cc: linux-kernel, coreyb, wad

On Friday, February 15, 2013 12:21:43 PM Paul Moore wrote:
> 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.

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

> Signed-off-by: Paul Moore <pmoore@redhat.com>
> Cc: stable@vger.kernel.org
> Cc: Will Drewry <wad@chromium.org>
> Cc: H. Peter Anvin <hpa@zytor.com>
> ---
>  arch/x86/include/asm/syscall.h |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/include/asm/syscall.h b/arch/x86/include/asm/syscall.h
> index 1ace47b..2e188d6 100644
> --- a/arch/x86/include/asm/syscall.h
> +++ b/arch/x86/include/asm/syscall.h
> @@ -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,

-- 
paul moore
security and virtualization @ redhat


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] x86: remove the x32 syscall bitmask from syscall_get_nr()
  2013-02-26 20:58 ` Paul Moore
@ 2013-03-15 21:15   ` Paul Moore
  2013-03-15 21:56     ` H. Peter Anvin
  0 siblings, 1 reply; 10+ messages in thread
From: Paul Moore @ 2013-03-15 21:15 UTC (permalink / raw)
  To: x86, hpa, keescook; +Cc: linux-kernel, coreyb, wad

On Tuesday, February 26, 2013 03:58:23 PM Paul Moore wrote:
> On Friday, February 15, 2013 12:21:43 PM Paul Moore wrote:
> > 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.
> 
> 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 ...

> > Signed-off-by: Paul Moore <pmoore@redhat.com>
> > Cc: stable@vger.kernel.org
> > Cc: Will Drewry <wad@chromium.org>
> > Cc: H. Peter Anvin <hpa@zytor.com>
> > ---
> > 
> >  arch/x86/include/asm/syscall.h |    4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/x86/include/asm/syscall.h
> > b/arch/x86/include/asm/syscall.h index 1ace47b..2e188d6 100644
> > --- a/arch/x86/include/asm/syscall.h
> > +++ b/arch/x86/include/asm/syscall.h
> > @@ -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,
-- 
paul moore
security and virtualization @ redhat


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] x86: remove the x32 syscall bitmask from syscall_get_nr()
  2013-03-15 21:15   ` Paul Moore
@ 2013-03-15 21:56     ` H. Peter Anvin
  2013-03-15 22:18       ` H.J. Lu
  0 siblings, 1 reply; 10+ messages in thread
From: H. Peter Anvin @ 2013-03-15 21:56 UTC (permalink / raw)
  To: Paul Moore; +Cc: x86, keescook, linux-kernel, coreyb, wad, H.J. Lu

On 03/15/2013 02:15 PM, Paul Moore wrote:
> On Tuesday, February 26, 2013 03:58:23 PM Paul Moore wrote:
>> On Friday, February 15, 2013 12:21:43 PM Paul Moore wrote:
>>> 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.
>>
>> 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

>>> Signed-off-by: Paul Moore <pmoore@redhat.com>
>>> Cc: stable@vger.kernel.org
>>> Cc: Will Drewry <wad@chromium.org>
>>> Cc: H. Peter Anvin <hpa@zytor.com>
>>> ---
>>>
>>>  arch/x86/include/asm/syscall.h |    4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/x86/include/asm/syscall.h
>>> b/arch/x86/include/asm/syscall.h index 1ace47b..2e188d6 100644
>>> --- a/arch/x86/include/asm/syscall.h
>>> +++ b/arch/x86/include/asm/syscall.h
>>> @@ -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,


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] x86: remove the x32 syscall bitmask from syscall_get_nr()
  2013-03-15 21:56     ` H. Peter Anvin
@ 2013-03-15 22:18       ` H.J. Lu
  2013-03-25 20:55         ` Paul Moore
  0 siblings, 1 reply; 10+ messages in thread
From: H.J. Lu @ 2013-03-15 22:18 UTC (permalink / raw)
  To: H. Peter Anvin; +Cc: Paul Moore, x86, keescook, linux-kernel, coreyb, wad

On Fri, Mar 15, 2013 at 2:56 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> On 03/15/2013 02:15 PM, Paul Moore wrote:
>> On Tuesday, February 26, 2013 03:58:23 PM Paul Moore wrote:
>>> On Friday, February 15, 2013 12:21:43 PM Paul Moore wrote:
>>>> 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.
>>>
>>> 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?
>

It looks OK to me.

-- 
H.J.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] x86: remove the x32 syscall bitmask from syscall_get_nr()
  2013-03-15 22:18       ` H.J. Lu
@ 2013-03-25 20:55         ` Paul Moore
  2013-04-02 21:31           ` Paul Moore
  0 siblings, 1 reply; 10+ messages in thread
From: Paul Moore @ 2013-03-25 20:55 UTC (permalink / raw)
  To: H.J. Lu, H. Peter Anvin, x86; +Cc: keescook, linux-kernel, coreyb, wad

On Friday, March 15, 2013 03:18:12 PM H.J. Lu wrote:
> On Fri, Mar 15, 2013 at 2:56 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> > On 03/15/2013 02:15 PM, Paul Moore wrote:
> >> On Tuesday, February 26, 2013 03:58:23 PM Paul Moore wrote:
> >>> On Friday, February 15, 2013 12:21:43 PM Paul Moore wrote:
> >>>> 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.
> >>> 
> >>> 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?
> 
> It looks OK to me.

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

-- 
paul moore
security and virtualization @ redhat


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] x86: remove the x32 syscall bitmask from syscall_get_nr()
  2013-03-25 20:55         ` Paul Moore
@ 2013-04-02 21:31           ` Paul Moore
  0 siblings, 0 replies; 10+ messages in thread
From: Paul Moore @ 2013-04-02 21:31 UTC (permalink / raw)
  To: H.J. Lu, H. Peter Anvin, x86, linux-kernel; +Cc: keescook, coreyb, wad

On Monday, March 25, 2013 04:55:17 PM Paul Moore wrote:
> On Friday, March 15, 2013 03:18:12 PM H.J. Lu wrote:
> > On Fri, Mar 15, 2013 at 2:56 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> > > On 03/15/2013 02:15 PM, Paul Moore wrote:
> > >> On Tuesday, February 26, 2013 03:58:23 PM Paul Moore wrote:
> > >>> On Friday, February 15, 2013 12:21:43 PM Paul Moore wrote:
> > >>>> 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.
> > >>> 
> > >>> 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?
> > 
> > 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.

-- 
paul moore
security and virtualization @ redhat


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [tip:x86/urgent] x86: remove the x32 syscall bitmask from syscall_get_nr()
  2013-02-15 17:21 [PATCH] x86: remove the x32 syscall bitmask from syscall_get_nr() Paul Moore
  2013-02-15 19:02 ` H. Peter Anvin
  2013-02-26 20:58 ` Paul Moore
@ 2013-04-03  0:17 ` tip-bot for Paul Moore
  2 siblings, 0 replies; 10+ messages in thread
From: tip-bot for Paul Moore @ 2013-04-03  0:17 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: linux-kernel, hpa, mingo, wad, pmoore, stable, tglx, hpa

Commit-ID:  8b4b9f27e57584f3d90e0bb84cf800ad81cfe3a1
Gitweb:     http://git.kernel.org/tip/8b4b9f27e57584f3d90e0bb84cf800ad81cfe3a1
Author:     Paul Moore <pmoore@redhat.com>
AuthorDate: Fri, 15 Feb 2013 12:21:43 -0500
Committer:  H. Peter Anvin <hpa@linux.intel.com>
CommitDate: Tue, 2 Apr 2013 14:38:09 -0700

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 <pmoore@redhat.com>
Link: http://lkml.kernel.org/r/20130215172143.12549.10292.stgit@localhost
Cc: <stable@vger.kernel.org>
Cc: Will Drewry <wad@chromium.org>
Cc: H. Peter Anvin <hpa@zytor.com>
Signed-off-by: H. Peter Anvin <hpa@linux.intel.com>
---
 arch/x86/include/asm/syscall.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/syscall.h b/arch/x86/include/asm/syscall.h
index 1ace47b..2e188d6 100644
--- a/arch/x86/include/asm/syscall.h
+++ b/arch/x86/include/asm/syscall.h
@@ -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,

^ permalink raw reply related	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2013-04-03  0:17 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-02-15 17:21 [PATCH] x86: remove the x32 syscall bitmask from syscall_get_nr() Paul Moore
2013-02-15 19:02 ` H. Peter Anvin
2013-02-15 20:52   ` Paul Moore
2013-02-26 20:58 ` Paul Moore
2013-03-15 21:15   ` Paul Moore
2013-03-15 21:56     ` H. Peter Anvin
2013-03-15 22:18       ` H.J. Lu
2013-03-25 20:55         ` Paul Moore
2013-04-02 21:31           ` Paul Moore
2013-04-03  0:17 ` [tip:x86/urgent] " tip-bot for Paul Moore

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.