Re: [PATCH] x86/ptrace: Remove questionable TS_COMPAT usage in ptrace

[Andy Lutomirski]

On Mon, Jun 20, 2016 at 9:14 AM, Oleg Nesterov  wrote:
> On 06/20, Andy Lutomirski wrote:
>>
>> On Mon, Jun 20, 2016 at 8:24 AM, Oleg Nesterov  wrote:
>> >
>> > How about the simple change below for now? IIRC 32-bit task can't use
>> > "syscall" so if syscall_get_nr() >= 0 then even the wrong TS_COMPAT is
>> > not that bad, even if it "leaks" to user-mode.
>>
>> Hmm.  That should fix the minor security issue, but it will even
>> further break cross-arch tracing: now a 32-bit tracer tracing a 64-bit
>> task that does int $0x80 will malfunction even more than it would
>> have.
>
> This is broken in any case. I mean, a 32-bit debugger can't really
> debug a 64-bit task.
>
> I don't think this change makes the things really worse.
>
>> Also, it relies on bizarre arch details IMO.
>
> Heh,  it looks as if your patch do not ;)
>
>> I think I prefer my version, coming momentarily.
>
> I disagree... I don't really understand why do we need the additional
> complications for the minimal fix which doesn't look very nice anyway.
>
> But I won't argue, and your patch looks correct to me.

Part of the reason is that I actually want to fix this in two parts.
After my TS_REGS_POKED_I386 patch, I have a follow-up patch that I
want to polish and test a bit more that's intended to get the
64-bit-tracer/32-bit-tracee case right, but it probably needs extra
testing, is less likely to make 4.8, and is definitely not for
4.7/stab.e.  That patch deletes TS_REGS_POKED_I386 again.  So by
having the minimal fix be more mindless, I'm more comfortable with the
backport.  I'll send out the second patch soon.


--Andy


-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [PATCH] x86/ptrace: Remove questionable TS_COMPAT usage in ptrace

[Andy Lutomirski]

On Mon, Jun 20, 2016 at 9:14 AM, Oleg Nesterov  wrote:
> On 06/20, Andy Lutomirski wrote:
>>
>> On Mon, Jun 20, 2016 at 8:24 AM, Oleg Nesterov  wrote:
>> >
>> > How about the simple change below for now? IIRC 32-bit task can't use
>> > "syscall" so if syscall_get_nr() >= 0 then even the wrong TS_COMPAT is
>> > not that bad, even if it "leaks" to user-mode.
>>
>> Hmm.  That should fix the minor security issue, but it will even
>> further break cross-arch tracing: now a 32-bit tracer tracing a 64-bit
>> task that does int $0x80 will malfunction even more than it would
>> have.
>
> This is broken in any case. I mean, a 32-bit debugger can't really
> debug a 64-bit task.
>
> I don't think this change makes the things really worse.
>
>> Also, it relies on bizarre arch details IMO.
>
> Heh,  it looks as if your patch do not ;)
>
>> I think I prefer my version, coming momentarily.
>
> I disagree... I don't really understand why do we need the additional
> complications for the minimal fix which doesn't look very nice anyway.
>
> But I won't argue, and your patch looks correct to me.

Part of the reason is that I actually want to fix this in two parts.
After my TS_REGS_POKED_I386 patch, I have a follow-up patch that I
want to polish and test a bit more that's intended to get the
64-bit-tracer/32-bit-tracee case right, but it probably needs extra
testing, is less likely to make 4.8, and is definitely not for
4.7/stab.e.  That patch deletes TS_REGS_POKED_I386 again.  So by
having the minimal fix be more mindless, I'm more comfortable with the
backport.  I'll send out the second patch soon.


--Andy


-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [PATCH] x86/ptrace: Remove questionable TS_COMPAT usage in ptrace

[Oleg Nesterov]

On 06/20, Andy Lutomirski wrote:
>
> On Mon, Jun 20, 2016 at 8:24 AM, Oleg Nesterov  wrote:
> >
> > How about the simple change below for now? IIRC 32-bit task can't use
> > "syscall" so if syscall_get_nr() >= 0 then even the wrong TS_COMPAT is
> > not that bad, even if it "leaks" to user-mode.
>
> Hmm.  That should fix the minor security issue, but it will even
> further break cross-arch tracing: now a 32-bit tracer tracing a 64-bit
> task that does int $0x80 will malfunction even more than it would
> have.

This is broken in any case. I mean, a 32-bit debugger can't really
debug a 64-bit task.

I don't think this change makes the things really worse.

> Also, it relies on bizarre arch details IMO.

Heh,  it looks as if your patch do not ;)

> I think I prefer my version, coming momentarily.

I disagree... I don't really understand why do we need the additional
complications for the minimal fix which doesn't look very nice anyway.

But I won't argue, and your patch looks correct to me.

Oleg.

Re: [PATCH] x86/ptrace: Remove questionable TS_COMPAT usage in ptrace

[Oleg Nesterov]

On 06/20, Andy Lutomirski wrote:
>
> On Mon, Jun 20, 2016 at 8:24 AM, Oleg Nesterov  wrote:
> >
> > How about the simple change below for now? IIRC 32-bit task can't use
> > "syscall" so if syscall_get_nr() >= 0 then even the wrong TS_COMPAT is
> > not that bad, even if it "leaks" to user-mode.
>
> Hmm.  That should fix the minor security issue, but it will even
> further break cross-arch tracing: now a 32-bit tracer tracing a 64-bit
> task that does int $0x80 will malfunction even more than it would
> have.

This is broken in any case. I mean, a 32-bit debugger can't really
debug a 64-bit task.

I don't think this change makes the things really worse.

> Also, it relies on bizarre arch details IMO.

Heh,  it looks as if your patch do not ;)

> I think I prefer my version, coming momentarily.

I disagree... I don't really understand why do we need the additional
complications for the minimal fix which doesn't look very nice anyway.

But I won't argue, and your patch looks correct to me.

Oleg.

Re: [PATCH] x86/ptrace: Remove questionable TS_COMPAT usage in ptrace

[Andy Lutomirski]

On Mon, Jun 20, 2016 at 8:24 AM, Oleg Nesterov  wrote:
> On 06/19, Andy Lutomirski wrote:
>>
>> On Sat, Jun 18, 2016 at 10:02 AM, Andy Lutomirski  
>> wrote:
>> Step 1: for 4.7 and for -stable, introduce TS_I386_REGS_POKED.  Set it
>> in putreg32.  Use it in syscall_get_error, get_nr_restart_syscall,
>> etc.  Clear it in do_signal.
>
> do_signal() won't be necessarily called...

True.  But I should have said "clear it in prepare_exit_to_usermode",
and the patch I'm just about to send does that.
>
>> I wonder if we could actually get away with doing syscall restart
>> processing before ptrace invocation.
>
> How? this doesn't look possible or I misunderstood.
>
> How about the simple change below for now? IIRC 32-bit task can't use
> "syscall" so if syscall_get_nr() >= 0 then even the wrong TS_COMPAT is
> not that bad, even if it "leaks" to user-mode.

Hmm.  That should fix the minor security issue, but it will even
further break cross-arch tracing: now a 32-bit tracer tracing a 64-bit
task that does int $0x80 will malfunction even more than it would
have.  Also, it relies on bizarre arch details IMO.

I think I prefer my version, coming momentarily.

--Andy

Re: [PATCH] x86/ptrace: Remove questionable TS_COMPAT usage in ptrace

[Andy Lutomirski]

On Mon, Jun 20, 2016 at 8:24 AM, Oleg Nesterov  wrote:
> On 06/19, Andy Lutomirski wrote:
>>
>> On Sat, Jun 18, 2016 at 10:02 AM, Andy Lutomirski  
>> wrote:
>> Step 1: for 4.7 and for -stable, introduce TS_I386_REGS_POKED.  Set it
>> in putreg32.  Use it in syscall_get_error, get_nr_restart_syscall,
>> etc.  Clear it in do_signal.
>
> do_signal() won't be necessarily called...

True.  But I should have said "clear it in prepare_exit_to_usermode",
and the patch I'm just about to send does that.
>
>> I wonder if we could actually get away with doing syscall restart
>> processing before ptrace invocation.
>
> How? this doesn't look possible or I misunderstood.
>
> How about the simple change below for now? IIRC 32-bit task can't use
> "syscall" so if syscall_get_nr() >= 0 then even the wrong TS_COMPAT is
> not that bad, even if it "leaks" to user-mode.

Hmm.  That should fix the minor security issue, but it will even
further break cross-arch tracing: now a 32-bit tracer tracing a 64-bit
task that does int $0x80 will malfunction even more than it would
have.  Also, it relies on bizarre arch details IMO.

I think I prefer my version, coming momentarily.

--Andy

Re: [PATCH] x86/ptrace: Remove questionable TS_COMPAT usage in ptrace

[Oleg Nesterov]

On 06/19, Andy Lutomirski wrote:
>
> On Sat, Jun 18, 2016 at 10:02 AM, Andy Lutomirski  wrote:
> Step 1: for 4.7 and for -stable, introduce TS_I386_REGS_POKED.  Set it
> in putreg32.  Use it in syscall_get_error, get_nr_restart_syscall,
> etc.  Clear it in do_signal.

do_signal() won't be necessarily called...

> I wonder if we could actually get away with doing syscall restart
> processing before ptrace invocation.

How? this doesn't look possible or I misunderstood.

How about the simple change below for now? IIRC 32-bit task can't use
"syscall" so if syscall_get_nr() >= 0 then even the wrong TS_COMPAT is
not that bad, even if it "leaks" to user-mode.

nobody should use, say, in_ia32_syscall() unless we know that "in syscall"
is actually true. Hmm, arch/x86/kernel/uprobes.c does and this is wrong
regardless, I'll send the fix.

Oleg.

--- x/arch/x86/kernel/ptrace.c
+++ x/arch/x86/kernel/ptrace.c
@@ -930,7 +930,7 @@ static int putreg32(struct task_struct *
 * exit from a 32-bit syscall with TS_COMPAT still set.
 */
regs->orig_ax = value;
-   if (syscall_get_nr(child, regs) >= 0)
+   if (syscall_get_nr(child, regs) >= 0 && !user_64bit_mode(regs))
task_thread_info(child)->status |= TS_COMPAT;
break;
 

Re: [PATCH] x86/ptrace: Remove questionable TS_COMPAT usage in ptrace

[Oleg Nesterov]

On 06/19, Andy Lutomirski wrote:
>
> On Sat, Jun 18, 2016 at 10:02 AM, Andy Lutomirski  wrote:
> Step 1: for 4.7 and for -stable, introduce TS_I386_REGS_POKED.  Set it
> in putreg32.  Use it in syscall_get_error, get_nr_restart_syscall,
> etc.  Clear it in do_signal.

do_signal() won't be necessarily called...

> I wonder if we could actually get away with doing syscall restart
> processing before ptrace invocation.

How? this doesn't look possible or I misunderstood.

How about the simple change below for now? IIRC 32-bit task can't use
"syscall" so if syscall_get_nr() >= 0 then even the wrong TS_COMPAT is
not that bad, even if it "leaks" to user-mode.

nobody should use, say, in_ia32_syscall() unless we know that "in syscall"
is actually true. Hmm, arch/x86/kernel/uprobes.c does and this is wrong
regardless, I'll send the fix.

Oleg.

--- x/arch/x86/kernel/ptrace.c
+++ x/arch/x86/kernel/ptrace.c
@@ -930,7 +930,7 @@ static int putreg32(struct task_struct *
 * exit from a 32-bit syscall with TS_COMPAT still set.
 */
regs->orig_ax = value;
-   if (syscall_get_nr(child, regs) >= 0)
+   if (syscall_get_nr(child, regs) >= 0 && !user_64bit_mode(regs))
task_thread_info(child)->status |= TS_COMPAT;
break;
 

Re: [PATCH] x86/ptrace: Remove questionable TS_COMPAT usage in ptrace

[Oleg Nesterov]

On 06/19, Andy Lutomirski wrote:
>
> On Sun, Jun 19, 2016 at 2:19 PM, Oleg Nesterov  wrote:
> >
> > And this leads to another question, why do we actually need to set/clear
> > TS_COMPAT in set_personality_ia32() ??
>
> Something's clearly buggy there, considering that
> set_personality_64bit() does *not* clear it.

Yes, yes, I too noticed this, and this doesn't match the "if (x32)"
branch in set_personality_ia32().

But I think we do not really need to clear this bit. And probably
set_personality_ia32() doesn't need to play with TS_COMPAT.

Oleg.

Re: [PATCH] x86/ptrace: Remove questionable TS_COMPAT usage in ptrace

[Oleg Nesterov]

On 06/19, Andy Lutomirski wrote:
>
> On Sun, Jun 19, 2016 at 2:19 PM, Oleg Nesterov  wrote:
> >
> > And this leads to another question, why do we actually need to set/clear
> > TS_COMPAT in set_personality_ia32() ??
>
> Something's clearly buggy there, considering that
> set_personality_64bit() does *not* clear it.

Yes, yes, I too noticed this, and this doesn't match the "if (x32)"
branch in set_personality_ia32().

But I think we do not really need to clear this bit. And probably
set_personality_ia32() doesn't need to play with TS_COMPAT.

Oleg.

Re: [PATCH] x86/ptrace: Remove questionable TS_COMPAT usage in ptrace

[Jan Kratochvil]

On Mon, 20 Jun 2016 12:07:56 +0200, Pedro Alves wrote:
> On 06/18/2016 06:02 PM, Andy Lutomirski wrote:
> > Yuck.  I should have dug in to the history.  Why not just
> > unconditionally sign-extend eax when set by a 32-bit tracer?
> 
> No idea.

Roland McGrath knows why he wrote it that way, Cced.

What if eax contains an address in 2GB..4GB range?  I guess currently the
orig_eax check tries to verify %eax cannot contain an address.


Jan

Re: [PATCH] x86/ptrace: Remove questionable TS_COMPAT usage in ptrace

[Jan Kratochvil]

On Mon, 20 Jun 2016 12:07:56 +0200, Pedro Alves wrote:
> On 06/18/2016 06:02 PM, Andy Lutomirski wrote:
> > Yuck.  I should have dug in to the history.  Why not just
> > unconditionally sign-extend eax when set by a 32-bit tracer?
> 
> No idea.

Roland McGrath knows why he wrote it that way, Cced.

What if eax contains an address in 2GB..4GB range?  I guess currently the
orig_eax check tries to verify %eax cannot contain an address.


Jan

Re: [PATCH] x86/ptrace: Remove questionable TS_COMPAT usage in ptrace

[Pedro Alves]

On 06/19/2016 11:09 PM, Andy Lutomirski wrote:
> 
> The latter bit is a mess and is probably broken on current kernels for
> 64-bit gdb attached to a 32-bit process.  (Is it?  All of this stuff
> is a bit of a pain to test.)

The testcase at:

 https://sourceware.org/ml/gdb/2014-05/msg4.html

still fails for me on Fedora 23 with git master gdb.

Nevermind the misleading URL, that's a kernel patch.

 $ gcc -g -m32 interrupt.c -o interrupt.32
 ...
 (gdb) r
 Starting program: /home/pedro/tmp/interrupt.32 
 talk to me baby
 ^C
 Program received signal SIGINT, Interrupt.
 0xf7fd9d49 in __kernel_vsyscall ()
 (gdb) p func1()
 $1 = 4
 (gdb) c
 Continuing.
 Unknown error 512
 [Inferior 1 (process 20252) exited with code 01]
 (gdb) 

That was a 64-bit gdb.

Note it doesn't fail with fedora 23's gdb, because of a 
fedora-local workaround.

Thanks,
Pedro Alves

Re: [PATCH] x86/ptrace: Remove questionable TS_COMPAT usage in ptrace

[Pedro Alves]

On 06/19/2016 11:09 PM, Andy Lutomirski wrote:
> 
> The latter bit is a mess and is probably broken on current kernels for
> 64-bit gdb attached to a 32-bit process.  (Is it?  All of this stuff
> is a bit of a pain to test.)

The testcase at:

 https://sourceware.org/ml/gdb/2014-05/msg4.html

still fails for me on Fedora 23 with git master gdb.

Nevermind the misleading URL, that's a kernel patch.

 $ gcc -g -m32 interrupt.c -o interrupt.32
 ...
 (gdb) r
 Starting program: /home/pedro/tmp/interrupt.32 
 talk to me baby
 ^C
 Program received signal SIGINT, Interrupt.
 0xf7fd9d49 in __kernel_vsyscall ()
 (gdb) p func1()
 $1 = 4
 (gdb) c
 Continuing.
 Unknown error 512
 [Inferior 1 (process 20252) exited with code 01]
 (gdb) 

That was a 64-bit gdb.

Note it doesn't fail with fedora 23's gdb, because of a 
fedora-local workaround.

Thanks,
Pedro Alves

Re: [PATCH] x86/ptrace: Remove questionable TS_COMPAT usage in ptrace

[Pedro Alves]

On 06/18/2016 06:02 PM, Andy Lutomirski wrote:

> Yuck.  I should have dug in to the history.  Why not just
> unconditionally sign-extend eax when set by a 32-bit tracer?

No idea.

> 
> Do you know how to acquire a copy of erestartsys-trap.c?  The old
> links appear to be broken.

That's part of the ptrace testsuite project, still in cvs, though the url 
changed:

 $ https://sourceware.org/systemtap/wiki/utrace/tests

 $ cvs -d :pserver:anoncvs:anon...@sourceware.org:/cvs/systemtap co ptrace-tests

Can't seem to find a cvsweb interface for that.

I think it'd be great to move these to the selftests infrastructure
directly in the kernel tree.  However, nobody's has ever managed to
find energy for that.

> 
> Also, while I have your attention: when gdb restores old state like
> this, does it do it with individual calls to PTRACE_POKEUSER or does
> it use SETREGSET or similar to do it all at once?  I'm asking because
> I have some other code (fsgsbase) that's on hold until I can figure
> out how to keep it from breaking gdb if and when gdb writes to fs and
> fs_base.
> 

It depends on which register you're accessing, and on kernel version.
But on a recent kernel, it should be using PTRACE_SETREGS / PTRACE_SETREGSET,
thus storing a whole register set in one go.  (And it's likely we could
get rid of PTRACE_POKE fallback paths by now.)
To write to the debug registers (dr0-dr7), PTRACE_POKEUSER is always used.

There's code that coordinates with glibc's libthread_db.so that ends
up _reading_ fs_base/gs_base, and gdb uses PTRACE_PEEKUSER for that,
though there's a pending series that changes it, exposing fs_base/gs_base
as just another register in gdb's register cache:

 https://www.sourceware.org/ml/gdb-patches/2015-11/msg00076.html
 https://www.sourceware.org/ml/gdb-patches/2015-11/msg00077.html

Guess that makes fs_base/gs_base user-writable, if the kernel allows it.

Thanks,
Pedro Alves

Re: [PATCH] x86/ptrace: Remove questionable TS_COMPAT usage in ptrace

[Pedro Alves]

On 06/18/2016 06:02 PM, Andy Lutomirski wrote:

> Yuck.  I should have dug in to the history.  Why not just
> unconditionally sign-extend eax when set by a 32-bit tracer?

No idea.

> 
> Do you know how to acquire a copy of erestartsys-trap.c?  The old
> links appear to be broken.

That's part of the ptrace testsuite project, still in cvs, though the url 
changed:

 $ https://sourceware.org/systemtap/wiki/utrace/tests

 $ cvs -d :pserver:anoncvs:anon...@sourceware.org:/cvs/systemtap co ptrace-tests

Can't seem to find a cvsweb interface for that.

I think it'd be great to move these to the selftests infrastructure
directly in the kernel tree.  However, nobody's has ever managed to
find energy for that.

> 
> Also, while I have your attention: when gdb restores old state like
> this, does it do it with individual calls to PTRACE_POKEUSER or does
> it use SETREGSET or similar to do it all at once?  I'm asking because
> I have some other code (fsgsbase) that's on hold until I can figure
> out how to keep it from breaking gdb if and when gdb writes to fs and
> fs_base.
> 

It depends on which register you're accessing, and on kernel version.
But on a recent kernel, it should be using PTRACE_SETREGS / PTRACE_SETREGSET,
thus storing a whole register set in one go.  (And it's likely we could
get rid of PTRACE_POKE fallback paths by now.)
To write to the debug registers (dr0-dr7), PTRACE_POKEUSER is always used.

There's code that coordinates with glibc's libthread_db.so that ends
up _reading_ fs_base/gs_base, and gdb uses PTRACE_PEEKUSER for that,
though there's a pending series that changes it, exposing fs_base/gs_base
as just another register in gdb's register cache:

 https://www.sourceware.org/ml/gdb-patches/2015-11/msg00076.html
 https://www.sourceware.org/ml/gdb-patches/2015-11/msg00077.html

Guess that makes fs_base/gs_base user-writable, if the kernel allows it.

Thanks,
Pedro Alves

Re: [PATCH] x86/ptrace: Remove questionable TS_COMPAT usage in ptrace

[Andy Lutomirski]

On Sun, Jun 19, 2016 at 2:19 PM, Oleg Nesterov  wrote:
> Let me first thank Pedro who has already replied!
>
> And I have to admit I will need to re-read his explanations after
> sleep to (try to) convince myself I fully understans the problems ;)
> Too late for me.
>
> Right now I have nothing to add, but
>
> On 06/18, Andy Lutomirski wrote:
>>
>> @@ -922,16 +922,7 @@ static int putreg32(struct task_struct *child, unsigned 
>> regno, u32 value)
>>   R32(esp, sp);
>>
>>   case offsetof(struct user32, regs.orig_eax):
>> - /*
>> -  * A 32-bit debugger setting orig_eax means to restore
>> -  * the state of the task restarting a 32-bit syscall.
>> -  * Make sure we interpret the -ERESTART* codes correctly
>> -  * in case the task is not actually still sitting at the
>> -  * exit from a 32-bit syscall with TS_COMPAT still set.
>> -  */
>>   regs->orig_ax = value;
>> - if (syscall_get_nr(child, regs) >= 0)
>> - task_thread_info(child)->status |= TS_COMPAT;
>
> I agree it would be nice to remove this code, but then it is not clear
> how/when we should sign-extend regs->ax..
>
> And this leads to another question, why do we actually need to set/clear
> TS_COMPAT in set_personality_ia32() ??

Something's clearly buggy there, considering that
set_personality_64bit() does *not* clear it.

--Andy

Re: [PATCH] x86/ptrace: Remove questionable TS_COMPAT usage in ptrace

[Andy Lutomirski]

On Sun, Jun 19, 2016 at 2:19 PM, Oleg Nesterov  wrote:
> Let me first thank Pedro who has already replied!
>
> And I have to admit I will need to re-read his explanations after
> sleep to (try to) convince myself I fully understans the problems ;)
> Too late for me.
>
> Right now I have nothing to add, but
>
> On 06/18, Andy Lutomirski wrote:
>>
>> @@ -922,16 +922,7 @@ static int putreg32(struct task_struct *child, unsigned 
>> regno, u32 value)
>>   R32(esp, sp);
>>
>>   case offsetof(struct user32, regs.orig_eax):
>> - /*
>> -  * A 32-bit debugger setting orig_eax means to restore
>> -  * the state of the task restarting a 32-bit syscall.
>> -  * Make sure we interpret the -ERESTART* codes correctly
>> -  * in case the task is not actually still sitting at the
>> -  * exit from a 32-bit syscall with TS_COMPAT still set.
>> -  */
>>   regs->orig_ax = value;
>> - if (syscall_get_nr(child, regs) >= 0)
>> - task_thread_info(child)->status |= TS_COMPAT;
>
> I agree it would be nice to remove this code, but then it is not clear
> how/when we should sign-extend regs->ax..
>
> And this leads to another question, why do we actually need to set/clear
> TS_COMPAT in set_personality_ia32() ??

Something's clearly buggy there, considering that
set_personality_64bit() does *not* clear it.

--Andy

Re: [PATCH] x86/ptrace: Remove questionable TS_COMPAT usage in ptrace

[Andy Lutomirski]

On Sun, Jun 19, 2016 at 2:19 PM, Oleg Nesterov  wrote:
> Let me first thank Pedro who has already replied!
>
> And I have to admit I will need to re-read his explanations after
> sleep to (try to) convince myself I fully understans the problems ;)
> Too late for me.
>
> Right now I have nothing to add, but
>
> On 06/18, Andy Lutomirski wrote:
>>
>> @@ -922,16 +922,7 @@ static int putreg32(struct task_struct *child, unsigned 
>> regno, u32 value)
>>   R32(esp, sp);
>>
>>   case offsetof(struct user32, regs.orig_eax):
>> - /*
>> -  * A 32-bit debugger setting orig_eax means to restore
>> -  * the state of the task restarting a 32-bit syscall.
>> -  * Make sure we interpret the -ERESTART* codes correctly
>> -  * in case the task is not actually still sitting at the
>> -  * exit from a 32-bit syscall with TS_COMPAT still set.
>> -  */
>>   regs->orig_ax = value;
>> - if (syscall_get_nr(child, regs) >= 0)
>> - task_thread_info(child)->status |= TS_COMPAT;
>
> I agree it would be nice to remove this code, but then it is not clear
> how/when we should sign-extend regs->ax..
>
> And this leads to another question, why do we actually need to set/clear
> TS_COMPAT in set_personality_ia32() ??

I have no idea.  Legacy junk?  Maybe so audit sees execution of a
64-bit task as a 64-bit sys_execve return even if the task was 32-bit
before execve?

--Andy

Re: [PATCH] x86/ptrace: Remove questionable TS_COMPAT usage in ptrace

[Andy Lutomirski]

On Sun, Jun 19, 2016 at 2:19 PM, Oleg Nesterov  wrote:
> Let me first thank Pedro who has already replied!
>
> And I have to admit I will need to re-read his explanations after
> sleep to (try to) convince myself I fully understans the problems ;)
> Too late for me.
>
> Right now I have nothing to add, but
>
> On 06/18, Andy Lutomirski wrote:
>>
>> @@ -922,16 +922,7 @@ static int putreg32(struct task_struct *child, unsigned 
>> regno, u32 value)
>>   R32(esp, sp);
>>
>>   case offsetof(struct user32, regs.orig_eax):
>> - /*
>> -  * A 32-bit debugger setting orig_eax means to restore
>> -  * the state of the task restarting a 32-bit syscall.
>> -  * Make sure we interpret the -ERESTART* codes correctly
>> -  * in case the task is not actually still sitting at the
>> -  * exit from a 32-bit syscall with TS_COMPAT still set.
>> -  */
>>   regs->orig_ax = value;
>> - if (syscall_get_nr(child, regs) >= 0)
>> - task_thread_info(child)->status |= TS_COMPAT;
>
> I agree it would be nice to remove this code, but then it is not clear
> how/when we should sign-extend regs->ax..
>
> And this leads to another question, why do we actually need to set/clear
> TS_COMPAT in set_personality_ia32() ??

I have no idea.  Legacy junk?  Maybe so audit sees execution of a
64-bit task as a 64-bit sys_execve return even if the task was 32-bit
before execve?

--Andy

Re: [PATCH] x86/ptrace: Remove questionable TS_COMPAT usage in ptrace

[Oleg Nesterov]

Let me first thank Pedro who has already replied!

And I have to admit I will need to re-read his explanations after
sleep to (try to) convince myself I fully understans the problems ;)
Too late for me.

Right now I have nothing to add, but

On 06/18, Andy Lutomirski wrote:
>
> @@ -922,16 +922,7 @@ static int putreg32(struct task_struct *child, unsigned 
> regno, u32 value)
>   R32(esp, sp);
>  
>   case offsetof(struct user32, regs.orig_eax):
> - /*
> -  * A 32-bit debugger setting orig_eax means to restore
> -  * the state of the task restarting a 32-bit syscall.
> -  * Make sure we interpret the -ERESTART* codes correctly
> -  * in case the task is not actually still sitting at the
> -  * exit from a 32-bit syscall with TS_COMPAT still set.
> -  */
>   regs->orig_ax = value;
> - if (syscall_get_nr(child, regs) >= 0)
> - task_thread_info(child)->status |= TS_COMPAT;

I agree it would be nice to remove this code, but then it is not clear
how/when we should sign-extend regs->ax..

And this leads to another question, why do we actually need to set/clear
TS_COMPAT in set_personality_ia32() ??

Oleg.

Re: [PATCH] x86/ptrace: Remove questionable TS_COMPAT usage in ptrace

[Oleg Nesterov]

Let me first thank Pedro who has already replied!

And I have to admit I will need to re-read his explanations after
sleep to (try to) convince myself I fully understans the problems ;)
Too late for me.

Right now I have nothing to add, but

On 06/18, Andy Lutomirski wrote:
>
> @@ -922,16 +922,7 @@ static int putreg32(struct task_struct *child, unsigned 
> regno, u32 value)
>   R32(esp, sp);
>  
>   case offsetof(struct user32, regs.orig_eax):
> - /*
> -  * A 32-bit debugger setting orig_eax means to restore
> -  * the state of the task restarting a 32-bit syscall.
> -  * Make sure we interpret the -ERESTART* codes correctly
> -  * in case the task is not actually still sitting at the
> -  * exit from a 32-bit syscall with TS_COMPAT still set.
> -  */
>   regs->orig_ax = value;
> - if (syscall_get_nr(child, regs) >= 0)
> - task_thread_info(child)->status |= TS_COMPAT;

I agree it would be nice to remove this code, but then it is not clear
how/when we should sign-extend regs->ax..

And this leads to another question, why do we actually need to set/clear
TS_COMPAT in set_personality_ia32() ??

Oleg.

Re: [PATCH] x86/ptrace: Remove questionable TS_COMPAT usage in ptrace

[Andy Lutomirski]

On Sat, Jun 18, 2016 at 10:02 AM, Andy Lutomirski  wrote:
> On Jun 18, 2016 6:56 AM, "Pedro Alves"  wrote:
>>
>> On 06/18/2016 11:21 AM, Andy Lutomirski wrote:
>> > A 32-bit tracer can set a tracee's TS_COMPAT flag by poking orig_ax.
>> >
>> > - If the tracee is stopped in a 32-bit syscall, this has no effect
>> >   as TS_COMPAT will already be set.
>> >
>> > - If the tracee is stopped on entry to a 64-bit syscall, this could
>> >   cause problems: in_compat_syscall() etc will be out of sync with
>> >   the actual syscall table in use.  I can imagine this bre aking
>> >   audit.  (It can't meaningfully break seccomp, as a malicious
>> >   tracer can already fully bypass seccomp.)  I could also imagine it
>> >   subtly confusing the implementations of a few syscalls.
>> >
>> >  - If the tracee is stopped in a non-syscall context, then TS_COMPAT
>> >will end up set in user mode, which isn't supposed to happen.  The 
>> > results
>> >are likely to be similar to the 64-bit syscall entry case.
>> >
>> > Fix it by deleting the code.
>> >
>> > Here's my understanding of the previous intent.  Suppose a
>> > 32-bit tracee makes a syscall that returns one of the -ERESTART
>> > codes.  A 32-bit tracer saves away its register state.  The tracee
>> > resumes, returns from the system call, and gets stopped again for a
>> > non-syscall reason (e.g. a signal).  Then the tracer tries to roll
>> > back the tracee's state by writing all of the saved registers back.
>> >
>> > The result of this sequence of events will be that the tracee's
>> > registers' low bits match what they were at the end of the syscall
>> > but TS_COMPAT will be clear.  This will cause syscall_get_error() to
>> > return a *positive* number, because we zero-extend registers poked
>> > by 32-bit tracers instead of sign-extending them.  As a result, with
>> > this change, syscall restart won't happen, whereas, historically, it
>> > would have happened.
>> >
>> > As far as I can tell, this corner case isn't very important, and I
>>
>> I believe it's actually very much very important for gdb, for restoring
>> the inferior state when the user calls a function in the inferior, with:
>>
>>  (gdb) print foo()
>>
>> Some background here:
>>
>>  
>> http://linux-kernel.vger.kernel.narkive.com/fqrh4xKK/patch-x86-ptrace-sign-extend-eax-with-orig-eax-0
>
> Yuck.  I should have dug in to the history.  Why not just
> unconditionally sign-extend eax when set by a 32-bit tracer?
>
> Do you know how to acquire a copy of erestartsys-trap.c?  The old
> links appear to be broken.
>
> Also, while I have your attention: when gdb restores old state like
> this, does it do it with individual calls to PTRACE_POKEUSER or does
> it use SETREGSET or similar to do it all at once?  I'm asking because
> I have some other code (fsgsbase) that's on hold until I can figure
> out how to keep it from breaking gdb if and when gdb writes to fs and
> fs_base.

OK, I studied this a bit.  What a mess!  Here's what's going on AFAICT:

1. For reasons that probably made sense, the kernel delivers signals
and triggers ptrace before handling syscall restart.  This means that
-ERESTART_RESTARTBLOCK, etc is visible to userspace.  We could
plausibly get away with changing that, but it seems quite risky.

2. As a result of (1), gdb (quite reasonably) expects that it can
snapshot user state on signal delivery, adjust regs to call a
function, and then restore user state.

3. Presumably as a result of (2), we do syscall restart if indicated
by the register state on ptrace resume even if we're *not* resuming a
syscall.

4. Also as a result of (1), gdb expects that writing -1 to orig_eax
via POKEUSER or similar will *disable* syscall restart, which is
necessary to get function calling on syscall exit to work.

The combination of (1) and (4) means that we need to skip syscall
restart if orig_eax == -1 (in a 32-bit signed sense).  The combination
of (1) and (2) means that we need to enable syscall restart if
orig_eax > 0 (in a 32-bit signed sense) and eax contains a
-ERESTARTxyz code (again in a signed sense).  And, for extra fun, we
need to make sure that, if we set __NR_restart_syscall, we set the
correct value based on the syscall that we're restarting.

The latter bit is a mess and is probably broken on current kernels for
64-bit gdb attached to a 32-bit process.  (Is it?  All of this stuff
is a bit of a pain to test.)

Here's my proposal:

Step 1: for 4.7 and for -stable, introduce TS_I386_REGS_POKED.  Set it
in putreg32.  Use it in syscall_get_error, get_nr_restart_syscall,
etc.  Clear it in do_signal.  This gets rid of the arch confusion
while (hopefully) preserving current behavior.

Step 2: Optionally, for 4.8, consider cleaning up the whole mess.
First, change putreg32 to sign-extend orig_eax and eax and just get
rid of the TS_COMPAT checks in syscall_get_nr and syscall_get_error.
Then we either change the 64-bit -ERESTART_BLOCK to a different number
or 

Re: [PATCH] x86/ptrace: Remove questionable TS_COMPAT usage in ptrace

[Andy Lutomirski]

On Sat, Jun 18, 2016 at 10:02 AM, Andy Lutomirski  wrote:
> On Jun 18, 2016 6:56 AM, "Pedro Alves"  wrote:
>>
>> On 06/18/2016 11:21 AM, Andy Lutomirski wrote:
>> > A 32-bit tracer can set a tracee's TS_COMPAT flag by poking orig_ax.
>> >
>> > - If the tracee is stopped in a 32-bit syscall, this has no effect
>> >   as TS_COMPAT will already be set.
>> >
>> > - If the tracee is stopped on entry to a 64-bit syscall, this could
>> >   cause problems: in_compat_syscall() etc will be out of sync with
>> >   the actual syscall table in use.  I can imagine this bre aking
>> >   audit.  (It can't meaningfully break seccomp, as a malicious
>> >   tracer can already fully bypass seccomp.)  I could also imagine it
>> >   subtly confusing the implementations of a few syscalls.
>> >
>> >  - If the tracee is stopped in a non-syscall context, then TS_COMPAT
>> >will end up set in user mode, which isn't supposed to happen.  The 
>> > results
>> >are likely to be similar to the 64-bit syscall entry case.
>> >
>> > Fix it by deleting the code.
>> >
>> > Here's my understanding of the previous intent.  Suppose a
>> > 32-bit tracee makes a syscall that returns one of the -ERESTART
>> > codes.  A 32-bit tracer saves away its register state.  The tracee
>> > resumes, returns from the system call, and gets stopped again for a
>> > non-syscall reason (e.g. a signal).  Then the tracer tries to roll
>> > back the tracee's state by writing all of the saved registers back.
>> >
>> > The result of this sequence of events will be that the tracee's
>> > registers' low bits match what they were at the end of the syscall
>> > but TS_COMPAT will be clear.  This will cause syscall_get_error() to
>> > return a *positive* number, because we zero-extend registers poked
>> > by 32-bit tracers instead of sign-extending them.  As a result, with
>> > this change, syscall restart won't happen, whereas, historically, it
>> > would have happened.
>> >
>> > As far as I can tell, this corner case isn't very important, and I
>>
>> I believe it's actually very much very important for gdb, for restoring
>> the inferior state when the user calls a function in the inferior, with:
>>
>>  (gdb) print foo()
>>
>> Some background here:
>>
>>  
>> http://linux-kernel.vger.kernel.narkive.com/fqrh4xKK/patch-x86-ptrace-sign-extend-eax-with-orig-eax-0
>
> Yuck.  I should have dug in to the history.  Why not just
> unconditionally sign-extend eax when set by a 32-bit tracer?
>
> Do you know how to acquire a copy of erestartsys-trap.c?  The old
> links appear to be broken.
>
> Also, while I have your attention: when gdb restores old state like
> this, does it do it with individual calls to PTRACE_POKEUSER or does
> it use SETREGSET or similar to do it all at once?  I'm asking because
> I have some other code (fsgsbase) that's on hold until I can figure
> out how to keep it from breaking gdb if and when gdb writes to fs and
> fs_base.

OK, I studied this a bit.  What a mess!  Here's what's going on AFAICT:

1. For reasons that probably made sense, the kernel delivers signals
and triggers ptrace before handling syscall restart.  This means that
-ERESTART_RESTARTBLOCK, etc is visible to userspace.  We could
plausibly get away with changing that, but it seems quite risky.

2. As a result of (1), gdb (quite reasonably) expects that it can
snapshot user state on signal delivery, adjust regs to call a
function, and then restore user state.

3. Presumably as a result of (2), we do syscall restart if indicated
by the register state on ptrace resume even if we're *not* resuming a
syscall.

4. Also as a result of (1), gdb expects that writing -1 to orig_eax
via POKEUSER or similar will *disable* syscall restart, which is
necessary to get function calling on syscall exit to work.

The combination of (1) and (4) means that we need to skip syscall
restart if orig_eax == -1 (in a 32-bit signed sense).  The combination
of (1) and (2) means that we need to enable syscall restart if
orig_eax > 0 (in a 32-bit signed sense) and eax contains a
-ERESTARTxyz code (again in a signed sense).  And, for extra fun, we
need to make sure that, if we set __NR_restart_syscall, we set the
correct value based on the syscall that we're restarting.

The latter bit is a mess and is probably broken on current kernels for
64-bit gdb attached to a 32-bit process.  (Is it?  All of this stuff
is a bit of a pain to test.)

Here's my proposal:

Step 1: for 4.7 and for -stable, introduce TS_I386_REGS_POKED.  Set it
in putreg32.  Use it in syscall_get_error, get_nr_restart_syscall,
etc.  Clear it in do_signal.  This gets rid of the arch confusion
while (hopefully) preserving current behavior.

Step 2: Optionally, for 4.8, consider cleaning up the whole mess.
First, change putreg32 to sign-extend orig_eax and eax and just get
rid of the TS_COMPAT checks in syscall_get_nr and syscall_get_error.
Then we either change the 64-bit -ERESTART_BLOCK to a different number
or encode the desired restart bitness in 

Re: [PATCH] x86/ptrace: Remove questionable TS_COMPAT usage in ptrace

[Kees Cook]

On Sat, Jun 18, 2016 at 3:21 AM, Andy Lutomirski  wrote:
> A 32-bit tracer can set a tracee's TS_COMPAT flag by poking orig_ax.
>
> - If the tracee is stopped in a 32-bit syscall, this has no effect
>   as TS_COMPAT will already be set.
>
> - If the tracee is stopped on entry to a 64-bit syscall, this could
>   cause problems: in_compat_syscall() etc will be out of sync with
>   the actual syscall table in use.  I can imagine this bre aking
>   audit.  (It can't meaningfully break seccomp, as a malicious
>   tracer can already fully bypass seccomp.)  I could also imagine it
>   subtly confusing the implementations of a few syscalls.
>
>  - If the tracee is stopped in a non-syscall context, then TS_COMPAT
>will end up set in user mode, which isn't supposed to happen.  The results
>are likely to be similar to the 64-bit syscall entry case.
>
> Fix it by deleting the code.
>
> Here's my understanding of the previous intent.  Suppose a
> 32-bit tracee makes a syscall that returns one of the -ERESTART
> codes.  A 32-bit tracer saves away its register state.  The tracee
> resumes, returns from the system call, and gets stopped again for a
> non-syscall reason (e.g. a signal).  Then the tracer tries to roll
> back the tracee's state by writing all of the saved registers back.
>
> The result of this sequence of events will be that the tracee's
> registers' low bits match what they were at the end of the syscall
> but TS_COMPAT will be clear.  This will cause syscall_get_error() to
> return a *positive* number, because we zero-extend registers poked
> by 32-bit tracers instead of sign-extending them.  As a result, with
> this change, syscall restart won't happen, whereas, historically, it
> would have happened.
>
> As far as I can tell, this corner case isn't very important, and I
> also don't see how one would reasonably have triggered it in the
> first place.  In particular, syscall restart happens before ptrace
> hooks in the syscall exit path, so a tracer should never see the
> problematic pre-restart syscall state in the first place.
>
> Signed-off-by: Andy Lutomirski 
> ---
>
> Oleg, you often have good insight into ptrace oddities.  Do you think I'm
> correct that this can safely be deleted?
>
> Kees, I think that either this or a similar fix is important to make the
> seccomp reordering series be fully effective.

For good measure, just to repeat what I said in the other thread:
yeah, this TS_COMPAT injection needs to be fixed, though I don't view
it as critical for seccomp since the major bypass situation will be
fixed already. The TS_COMPAT interaction isn't bad (since the arch
state is saved before the seccomp calls), but it can confuse a filter
intentionally trying to hit this bug via RET_SECCOMP_TRACE, but then
it would only be a problem for a filter that was either ignoring arch
tests or doing biarch filtering, which is extremely uncommon.

-Kees

>
> Ingo, this is intended for x86/urgent.  I deliberately didn't cc stable,
> as the bug this fixes seems minor enough that I think we can wait a while
> to backport it.
>
> arch/x86/kernel/ptrace.c | 9 -
>  1 file changed, 9 deletions(-)
>
> diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
> index 600edd225e81..bde57caf9aa9 100644
> --- a/arch/x86/kernel/ptrace.c
> +++ b/arch/x86/kernel/ptrace.c
> @@ -922,16 +922,7 @@ static int putreg32(struct task_struct *child, unsigned 
> regno, u32 value)
> R32(esp, sp);
>
> case offsetof(struct user32, regs.orig_eax):
> -   /*
> -* A 32-bit debugger setting orig_eax means to restore
> -* the state of the task restarting a 32-bit syscall.
> -* Make sure we interpret the -ERESTART* codes correctly
> -* in case the task is not actually still sitting at the
> -* exit from a 32-bit syscall with TS_COMPAT still set.
> -*/
> regs->orig_ax = value;
> -   if (syscall_get_nr(child, regs) >= 0)
> -   task_thread_info(child)->status |= TS_COMPAT;
> break;
>
> case offsetof(struct user32, regs.eflags):
> --
> 2.7.4
>



-- 
Kees Cook
Chrome OS & Brillo Security

Re: [PATCH] x86/ptrace: Remove questionable TS_COMPAT usage in ptrace

[Kees Cook]

On Sat, Jun 18, 2016 at 3:21 AM, Andy Lutomirski  wrote:
> A 32-bit tracer can set a tracee's TS_COMPAT flag by poking orig_ax.
>
> - If the tracee is stopped in a 32-bit syscall, this has no effect
>   as TS_COMPAT will already be set.
>
> - If the tracee is stopped on entry to a 64-bit syscall, this could
>   cause problems: in_compat_syscall() etc will be out of sync with
>   the actual syscall table in use.  I can imagine this bre aking
>   audit.  (It can't meaningfully break seccomp, as a malicious
>   tracer can already fully bypass seccomp.)  I could also imagine it
>   subtly confusing the implementations of a few syscalls.
>
>  - If the tracee is stopped in a non-syscall context, then TS_COMPAT
>will end up set in user mode, which isn't supposed to happen.  The results
>are likely to be similar to the 64-bit syscall entry case.
>
> Fix it by deleting the code.
>
> Here's my understanding of the previous intent.  Suppose a
> 32-bit tracee makes a syscall that returns one of the -ERESTART
> codes.  A 32-bit tracer saves away its register state.  The tracee
> resumes, returns from the system call, and gets stopped again for a
> non-syscall reason (e.g. a signal).  Then the tracer tries to roll
> back the tracee's state by writing all of the saved registers back.
>
> The result of this sequence of events will be that the tracee's
> registers' low bits match what they were at the end of the syscall
> but TS_COMPAT will be clear.  This will cause syscall_get_error() to
> return a *positive* number, because we zero-extend registers poked
> by 32-bit tracers instead of sign-extending them.  As a result, with
> this change, syscall restart won't happen, whereas, historically, it
> would have happened.
>
> As far as I can tell, this corner case isn't very important, and I
> also don't see how one would reasonably have triggered it in the
> first place.  In particular, syscall restart happens before ptrace
> hooks in the syscall exit path, so a tracer should never see the
> problematic pre-restart syscall state in the first place.
>
> Signed-off-by: Andy Lutomirski 
> ---
>
> Oleg, you often have good insight into ptrace oddities.  Do you think I'm
> correct that this can safely be deleted?
>
> Kees, I think that either this or a similar fix is important to make the
> seccomp reordering series be fully effective.

For good measure, just to repeat what I said in the other thread:
yeah, this TS_COMPAT injection needs to be fixed, though I don't view
it as critical for seccomp since the major bypass situation will be
fixed already. The TS_COMPAT interaction isn't bad (since the arch
state is saved before the seccomp calls), but it can confuse a filter
intentionally trying to hit this bug via RET_SECCOMP_TRACE, but then
it would only be a problem for a filter that was either ignoring arch
tests or doing biarch filtering, which is extremely uncommon.

-Kees

>
> Ingo, this is intended for x86/urgent.  I deliberately didn't cc stable,
> as the bug this fixes seems minor enough that I think we can wait a while
> to backport it.
>
> arch/x86/kernel/ptrace.c | 9 -
>  1 file changed, 9 deletions(-)
>
> diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
> index 600edd225e81..bde57caf9aa9 100644
> --- a/arch/x86/kernel/ptrace.c
> +++ b/arch/x86/kernel/ptrace.c
> @@ -922,16 +922,7 @@ static int putreg32(struct task_struct *child, unsigned 
> regno, u32 value)
> R32(esp, sp);
>
> case offsetof(struct user32, regs.orig_eax):
> -   /*
> -* A 32-bit debugger setting orig_eax means to restore
> -* the state of the task restarting a 32-bit syscall.
> -* Make sure we interpret the -ERESTART* codes correctly
> -* in case the task is not actually still sitting at the
> -* exit from a 32-bit syscall with TS_COMPAT still set.
> -*/
> regs->orig_ax = value;
> -   if (syscall_get_nr(child, regs) >= 0)
> -   task_thread_info(child)->status |= TS_COMPAT;
> break;
>
> case offsetof(struct user32, regs.eflags):
> --
> 2.7.4
>



-- 
Kees Cook
Chrome OS & Brillo Security

Re: [PATCH] x86/ptrace: Remove questionable TS_COMPAT usage in ptrace

[Andy Lutomirski]

On Jun 18, 2016 6:56 AM, "Pedro Alves"  wrote:
>
> On 06/18/2016 11:21 AM, Andy Lutomirski wrote:
> > A 32-bit tracer can set a tracee's TS_COMPAT flag by poking orig_ax.
> >
> > - If the tracee is stopped in a 32-bit syscall, this has no effect
> >   as TS_COMPAT will already be set.
> >
> > - If the tracee is stopped on entry to a 64-bit syscall, this could
> >   cause problems: in_compat_syscall() etc will be out of sync with
> >   the actual syscall table in use.  I can imagine this bre aking
> >   audit.  (It can't meaningfully break seccomp, as a malicious
> >   tracer can already fully bypass seccomp.)  I could also imagine it
> >   subtly confusing the implementations of a few syscalls.
> >
> >  - If the tracee is stopped in a non-syscall context, then TS_COMPAT
> >will end up set in user mode, which isn't supposed to happen.  The 
> > results
> >are likely to be similar to the 64-bit syscall entry case.
> >
> > Fix it by deleting the code.
> >
> > Here's my understanding of the previous intent.  Suppose a
> > 32-bit tracee makes a syscall that returns one of the -ERESTART
> > codes.  A 32-bit tracer saves away its register state.  The tracee
> > resumes, returns from the system call, and gets stopped again for a
> > non-syscall reason (e.g. a signal).  Then the tracer tries to roll
> > back the tracee's state by writing all of the saved registers back.
> >
> > The result of this sequence of events will be that the tracee's
> > registers' low bits match what they were at the end of the syscall
> > but TS_COMPAT will be clear.  This will cause syscall_get_error() to
> > return a *positive* number, because we zero-extend registers poked
> > by 32-bit tracers instead of sign-extending them.  As a result, with
> > this change, syscall restart won't happen, whereas, historically, it
> > would have happened.
> >
> > As far as I can tell, this corner case isn't very important, and I
>
> I believe it's actually very much very important for gdb, for restoring
> the inferior state when the user calls a function in the inferior, with:
>
>  (gdb) print foo()
>
> Some background here:
>
>  
> http://linux-kernel.vger.kernel.narkive.com/fqrh4xKK/patch-x86-ptrace-sign-extend-eax-with-orig-eax-0

Yuck.  I should have dug in to the history.  Why not just
unconditionally sign-extend eax when set by a 32-bit tracer?

Do you know how to acquire a copy of erestartsys-trap.c?  The old
links appear to be broken.

Also, while I have your attention: when gdb restores old state like
this, does it do it with individual calls to PTRACE_POKEUSER or does
it use SETREGSET or similar to do it all at once?  I'm asking because
I have some other code (fsgsbase) that's on hold until I can figure
out how to keep it from breaking gdb if and when gdb writes to fs and
fs_base.

Re: [PATCH] x86/ptrace: Remove questionable TS_COMPAT usage in ptrace

[Andy Lutomirski]

On Jun 18, 2016 6:56 AM, "Pedro Alves"  wrote:
>
> On 06/18/2016 11:21 AM, Andy Lutomirski wrote:
> > A 32-bit tracer can set a tracee's TS_COMPAT flag by poking orig_ax.
> >
> > - If the tracee is stopped in a 32-bit syscall, this has no effect
> >   as TS_COMPAT will already be set.
> >
> > - If the tracee is stopped on entry to a 64-bit syscall, this could
> >   cause problems: in_compat_syscall() etc will be out of sync with
> >   the actual syscall table in use.  I can imagine this bre aking
> >   audit.  (It can't meaningfully break seccomp, as a malicious
> >   tracer can already fully bypass seccomp.)  I could also imagine it
> >   subtly confusing the implementations of a few syscalls.
> >
> >  - If the tracee is stopped in a non-syscall context, then TS_COMPAT
> >will end up set in user mode, which isn't supposed to happen.  The 
> > results
> >are likely to be similar to the 64-bit syscall entry case.
> >
> > Fix it by deleting the code.
> >
> > Here's my understanding of the previous intent.  Suppose a
> > 32-bit tracee makes a syscall that returns one of the -ERESTART
> > codes.  A 32-bit tracer saves away its register state.  The tracee
> > resumes, returns from the system call, and gets stopped again for a
> > non-syscall reason (e.g. a signal).  Then the tracer tries to roll
> > back the tracee's state by writing all of the saved registers back.
> >
> > The result of this sequence of events will be that the tracee's
> > registers' low bits match what they were at the end of the syscall
> > but TS_COMPAT will be clear.  This will cause syscall_get_error() to
> > return a *positive* number, because we zero-extend registers poked
> > by 32-bit tracers instead of sign-extending them.  As a result, with
> > this change, syscall restart won't happen, whereas, historically, it
> > would have happened.
> >
> > As far as I can tell, this corner case isn't very important, and I
>
> I believe it's actually very much very important for gdb, for restoring
> the inferior state when the user calls a function in the inferior, with:
>
>  (gdb) print foo()
>
> Some background here:
>
>  
> http://linux-kernel.vger.kernel.narkive.com/fqrh4xKK/patch-x86-ptrace-sign-extend-eax-with-orig-eax-0

Yuck.  I should have dug in to the history.  Why not just
unconditionally sign-extend eax when set by a 32-bit tracer?

Do you know how to acquire a copy of erestartsys-trap.c?  The old
links appear to be broken.

Also, while I have your attention: when gdb restores old state like
this, does it do it with individual calls to PTRACE_POKEUSER or does
it use SETREGSET or similar to do it all at once?  I'm asking because
I have some other code (fsgsbase) that's on hold until I can figure
out how to keep it from breaking gdb if and when gdb writes to fs and
fs_base.

Re: [PATCH] x86/ptrace: Remove questionable TS_COMPAT usage in ptrace

[Pedro Alves]

On 06/18/2016 02:55 PM, Pedro Alves wrote:

> This hunk being mentioned in this thread a couple years ago too:
> 
>  https://www.sourceware.org/ml/gdb/2014-04/msg00095.html
> 
> Please don't break this use case ( and fix the one reported in
> that thread :-) ).

BTW, there was a follow up v2 patch to that last url, here:

 [PATCH v2] Fix get ERESTARTSYS with m32 in x86_64 when debug by GDB
 https://sourceware.org/ml/gdb/2014-05/msg4.html

(for some reason, I can only find the ping on the lkml, not the
 original post, though it was apparently CCed.)

Thanks,
Pedro Alves

Re: [PATCH] x86/ptrace: Remove questionable TS_COMPAT usage in ptrace

[Pedro Alves]

On 06/18/2016 02:55 PM, Pedro Alves wrote:

> This hunk being mentioned in this thread a couple years ago too:
> 
>  https://www.sourceware.org/ml/gdb/2014-04/msg00095.html
> 
> Please don't break this use case ( and fix the one reported in
> that thread :-) ).

BTW, there was a follow up v2 patch to that last url, here:

 [PATCH v2] Fix get ERESTARTSYS with m32 in x86_64 when debug by GDB
 https://sourceware.org/ml/gdb/2014-05/msg4.html

(for some reason, I can only find the ping on the lkml, not the
 original post, though it was apparently CCed.)

Thanks,
Pedro Alves

Re: [PATCH] x86/ptrace: Remove questionable TS_COMPAT usage in ptrace

[Pedro Alves]

On 06/18/2016 11:21 AM, Andy Lutomirski wrote:
> A 32-bit tracer can set a tracee's TS_COMPAT flag by poking orig_ax.
> 
> - If the tracee is stopped in a 32-bit syscall, this has no effect
>   as TS_COMPAT will already be set.
> 
> - If the tracee is stopped on entry to a 64-bit syscall, this could
>   cause problems: in_compat_syscall() etc will be out of sync with
>   the actual syscall table in use.  I can imagine this bre aking
>   audit.  (It can't meaningfully break seccomp, as a malicious
>   tracer can already fully bypass seccomp.)  I could also imagine it
>   subtly confusing the implementations of a few syscalls.
> 
>  - If the tracee is stopped in a non-syscall context, then TS_COMPAT
>will end up set in user mode, which isn't supposed to happen.  The results
>are likely to be similar to the 64-bit syscall entry case.
> 
> Fix it by deleting the code.
> 
> Here's my understanding of the previous intent.  Suppose a
> 32-bit tracee makes a syscall that returns one of the -ERESTART
> codes.  A 32-bit tracer saves away its register state.  The tracee
> resumes, returns from the system call, and gets stopped again for a
> non-syscall reason (e.g. a signal).  Then the tracer tries to roll
> back the tracee's state by writing all of the saved registers back.
> 
> The result of this sequence of events will be that the tracee's
> registers' low bits match what they were at the end of the syscall
> but TS_COMPAT will be clear.  This will cause syscall_get_error() to
> return a *positive* number, because we zero-extend registers poked
> by 32-bit tracers instead of sign-extending them.  As a result, with
> this change, syscall restart won't happen, whereas, historically, it
> would have happened.
> 
> As far as I can tell, this corner case isn't very important, and I

I believe it's actually very much very important for gdb, for restoring
the inferior state when the user calls a function in the inferior, with:

 (gdb) print foo()

Some background here:

 
http://linux-kernel.vger.kernel.narkive.com/fqrh4xKK/patch-x86-ptrace-sign-extend-eax-with-orig-eax-0

This hunk being mentioned in this thread a couple years ago too:

 https://www.sourceware.org/ml/gdb/2014-04/msg00095.html

Please don't break this use case ( and fix the one reported in
that thread :-) ).

Thanks,
Pedro Alves

Re: [PATCH] x86/ptrace: Remove questionable TS_COMPAT usage in ptrace

[Pedro Alves]

On 06/18/2016 11:21 AM, Andy Lutomirski wrote:
> A 32-bit tracer can set a tracee's TS_COMPAT flag by poking orig_ax.
> 
> - If the tracee is stopped in a 32-bit syscall, this has no effect
>   as TS_COMPAT will already be set.
> 
> - If the tracee is stopped on entry to a 64-bit syscall, this could
>   cause problems: in_compat_syscall() etc will be out of sync with
>   the actual syscall table in use.  I can imagine this bre aking
>   audit.  (It can't meaningfully break seccomp, as a malicious
>   tracer can already fully bypass seccomp.)  I could also imagine it
>   subtly confusing the implementations of a few syscalls.
> 
>  - If the tracee is stopped in a non-syscall context, then TS_COMPAT
>will end up set in user mode, which isn't supposed to happen.  The results
>are likely to be similar to the 64-bit syscall entry case.
> 
> Fix it by deleting the code.
> 
> Here's my understanding of the previous intent.  Suppose a
> 32-bit tracee makes a syscall that returns one of the -ERESTART
> codes.  A 32-bit tracer saves away its register state.  The tracee
> resumes, returns from the system call, and gets stopped again for a
> non-syscall reason (e.g. a signal).  Then the tracer tries to roll
> back the tracee's state by writing all of the saved registers back.
> 
> The result of this sequence of events will be that the tracee's
> registers' low bits match what they were at the end of the syscall
> but TS_COMPAT will be clear.  This will cause syscall_get_error() to
> return a *positive* number, because we zero-extend registers poked
> by 32-bit tracers instead of sign-extending them.  As a result, with
> this change, syscall restart won't happen, whereas, historically, it
> would have happened.
> 
> As far as I can tell, this corner case isn't very important, and I

I believe it's actually very much very important for gdb, for restoring
the inferior state when the user calls a function in the inferior, with:

 (gdb) print foo()

Some background here:

 
http://linux-kernel.vger.kernel.narkive.com/fqrh4xKK/patch-x86-ptrace-sign-extend-eax-with-orig-eax-0

This hunk being mentioned in this thread a couple years ago too:

 https://www.sourceware.org/ml/gdb/2014-04/msg00095.html

Please don't break this use case ( and fix the one reported in
that thread :-) ).

Thanks,
Pedro Alves