Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1611057 > unrolled thread

[PATCH 1/1] get_nr_restart_syscall() should return __NR_ia32_restart_syscall if __USER32_CS

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 1/1] get_nr_restart_syscall() should return  __NR_ia32_restart_syscall if __USER32_CS Oleg Nesterov <oleg@redhat.com> - 2017-03-28 17:10 +0200
    Re: [PATCH 1/1] get_nr_restart_syscall() should return  __NR_ia32_restart_syscall if __USER32_CS Andy Lutomirski <luto@kernel.org> - 2017-03-28 17:10 +0200
      Re: [PATCH 1/1] get_nr_restart_syscall() should return  __NR_ia32_restart_syscall if __USER32_CS Oleg Nesterov <oleg@redhat.com> - 2017-03-28 18:30 +0200
        Re: [PATCH 1/1] get_nr_restart_syscall() should return  __NR_ia32_restart_syscall if __USER32_CS Andy Lutomirski <luto@amacapital.net> - 2017-03-28 19:20 +0200
        Re: [PATCH 1/1] get_nr_restart_syscall() should return  __NR_ia32_restart_syscall if __USER32_CS Oleg Nesterov <oleg@redhat.com> - 2017-03-29 17:10 +0200
          Re: [PATCH 1/1] get_nr_restart_syscall() should return  __NR_ia32_restart_syscall if __USER32_CS Andy Lutomirski <luto@amacapital.net> - 2017-03-29 19:10 +0200
            Re: [PATCH 1/1] get_nr_restart_syscall() should return  __NR_ia32_restart_syscall if __USER32_CS Oleg Nesterov <oleg@redhat.com> - 2017-03-30 17:30 +0200
              Re: [PATCH 1/1] get_nr_restart_syscall() should return  __NR_ia32_restart_syscall if __USER32_CS Andy Lutomirski <luto@amacapital.net> - 2017-03-30 20:40 +0200

#1611057 — [PATCH 1/1] get_nr_restart_syscall() should return __NR_ia32_restart_syscall if __USER32_CS

get_nr_restart_syscall() checks TS_I386_REGS_POKED but this bit is only
set if debugger is 32-bit. If a 64-bit debugger restores the registers
of a 32-bit debugee outside of syscall exit path get_nr_restart_syscall()
wrongly returns __NR_restart_syscall.

Test-case:

  $ cvs -d :pserver:anoncvs:anoncvs@sourceware.org:/cvs/systemtap co ptrace-tests
  $ gcc -o erestartsys-trap-debuggee ptrace-tests/tests/erestartsys-trap-debuggee.c --m32
  $ gcc -o erestartsys-trap-debugger ptrace-tests/tests/erestartsys-trap-debugger.c -lutil
  $ ./erestartsys-trap-debugger
  Unexpected: retval 1, errno 22
  erestartsys-trap-debugger: ptrace-tests/tests/erestartsys-trap-debugger.c:421

As Jan explains this is what "(gdb) call func()" actually does:

	* Tracee calls sleep(2).
	* Debugger interrupts the tracee by CTRL-C after 1 sec.
	* Save regs by PTRACE_GETREGS.
	* Use PTRACE_SETREGS changing %rip to some 'func' and setting %orig_rax=-1
	* PTRACE_CONT
	* func() uses int3.
	* Debugger catches SIGTRAP.
	* Restore original regs by PTRACE_SETREGS.
	* PTRACE_CONT

Change get_nr_restart_syscall() to take __USER32_CS into account, to me
this looks a bit better than TIF_IA32 check but either way this logic
can't be always right as the comment explains.

Alternatively we could change putreg() to set TS_I386_REGS_POKED just like
putreg32() does if "child" is 32-bit, but this won't fix all the problems
too and I think it would be beter to kill TS_I386_REGS_POKED after this
change.

Reported-by: Jan Kratochvil <jan.kratochvil@redhat.com>
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Cc: stable@vger.kernel.org
---
 arch/x86/kernel/signal.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index 763af1d..1b05448 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -785,7 +785,8 @@ static inline unsigned long get_nr_restart_syscall(const struct pt_regs *regs)
 	 * than the tracee.
 	 */
 #ifdef CONFIG_IA32_EMULATION
-	if (current->thread.status & (TS_COMPAT|TS_I386_REGS_POKED))
+	if ((current->thread.status & (TS_COMPAT|TS_I386_REGS_POKED)) ||
+	    regs->cs == __USER32_CS)
 		return __NR_ia32_restart_syscall;
 #endif
 #ifdef CONFIG_X86_X32_ABI
-- 
2.5.0

[toc] | [next] | [standalone]

#1611072

On Tue, Mar 28, 2017 at 7:54 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> get_nr_restart_syscall() checks TS_I386_REGS_POKED but this bit is only
> set if debugger is 32-bit. If a 64-bit debugger restores the registers
> of a 32-bit debugee outside of syscall exit path get_nr_restart_syscall()
> wrongly returns __NR_restart_syscall.

I had sent a patch that introduced a new syscall nr, but it's not
quite safe because it could break seccomp-using programs.  But your
patch here is also screwy.

How about we store the syscall arch to be restored in task_struct
along with restart_block?  It's not perfect, but it should be 99% of
the way there without heuristics as nasty as yours.

--Andy

P.S. __USER32_CS is the wrong check even if we used your approach.
user_64bit_regs() is much better.

[toc] | [prev] | [next] | [standalone]

#1611187

On 03/28, Andy Lutomirski wrote:
>
> On Tue, Mar 28, 2017 at 7:54 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> > get_nr_restart_syscall() checks TS_I386_REGS_POKED but this bit is only
> > set if debugger is 32-bit. If a 64-bit debugger restores the registers
> > of a 32-bit debugee outside of syscall exit path get_nr_restart_syscall()
> > wrongly returns __NR_restart_syscall.
>
> I had sent a patch that introduced a new syscall nr, but it's not
> quite safe because it could break seccomp-using programs.

Ah, indeed...

> But your
> patch here is also screwy.

Yes, yes, it doesn't try to solve all possible problems, I even mentioned
this in the changelog.

> How about we store the syscall arch to be restored in task_struct
> along with restart_block?

Yes, perhaps we will have to finally do this. Not really nice too.

> the way there without heuristics as nasty as yours.

I agree it will be better, but I refuse to treat them as mine checks ;)

> P.S. __USER32_CS is the wrong check even if we used your approach.
> user_64bit_regs() is much better.

Yes, thanks.  If only I understood what cs == pv_info.extra_user_64bit_cs
actually means...


OK, please ignore this patch, I'll try to make another fix.

Oleg.

[toc] | [prev] | [next] | [standalone]

#1611233

On Tue, Mar 28, 2017 at 9:27 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 03/28, Andy Lutomirski wrote:
>>
>> On Tue, Mar 28, 2017 at 7:54 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>> > get_nr_restart_syscall() checks TS_I386_REGS_POKED but this bit is only
>> > set if debugger is 32-bit. If a 64-bit debugger restores the registers
>> > of a 32-bit debugee outside of syscall exit path get_nr_restart_syscall()
>> > wrongly returns __NR_restart_syscall.
>>
>> I had sent a patch that introduced a new syscall nr, but it's not
>> quite safe because it could break seccomp-using programs.
>
> Ah, indeed...

This is, in theory, solvable.  It would be ugly and would pollute seccomp a bit.

>
>> But your
>> patch here is also screwy.
>
> Yes, yes, it doesn't try to solve all possible problems, I even mentioned
> this in the changelog.
>
>> How about we store the syscall arch to be restored in task_struct
>> along with restart_block?
>
> Yes, perhaps we will have to finally do this. Not really nice too.
>
>> the way there without heuristics as nasty as yours.
>
> I agree it will be better, but I refuse to treat them as mine checks ;)

:)

>
>> P.S. __USER32_CS is the wrong check even if we used your approach.
>> user_64bit_regs() is much better.
>
> Yes, thanks.  If only I understood what cs == pv_info.extra_user_64bit_cs
> actually means...
>

It means that, if Linux is a Xen PV guest, the GDT contains a bunch of
entries supplied by Xen and outside of Linux's control, and one of
those entries is a 64-bit DPL=3 code segment.  On the one hand, it's
annoying.  On the other hand, it serves a real purpose
performance-wise.

--Andy

[toc] | [prev] | [next] | [standalone]

#1612018

On 03/28, Oleg Nesterov wrote:
>
> On 03/28, Andy Lutomirski wrote:
> >
> > How about we store the syscall arch to be restored in task_struct
> > along with restart_block?
>
> Yes, perhaps we will have to finally do this. Not really nice too.

OK, how about the hack below?

I do not want to a new member into task_struct/restart_block, so the
patch below adds a sticky TS_COMPAT bit which logically is a member
of "struct restart_block".

TS_I386_REGS_POKED must die, I think. But this needs another discussion.

Oleg.


diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
index b83c61c..a94bb5e 100644
--- a/arch/x86/entry/common.c
+++ b/arch/x86/entry/common.c
@@ -249,6 +249,17 @@ __visible inline void syscall_return_slowpath(struct pt_regs *regs)
 		local_irq_enable();
 
 	/*
+	 * Do this before debugger can change the regs.
+	 */
+	if (IS_ENABLED(CONFIG_IA32_EMULATION) &&
+	    unlikely(regs->ax == -ERESTART_RESTARTBLOCK)) {
+		if (current->thread.status & TS_COMPAT)
+			current->thread.status |= TS_COMPAT_XXX;
+		else
+			current->thread.status &= ~TS_COMPAT_XXX;
+	}
+
+	/*
 	 * First do one-time work.  If these work items are enabled, we
 	 * want to run them exactly once per syscall exit with IRQs on.
 	 */
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 1be64da..87179ab 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -477,6 +477,7 @@ struct thread_struct {
  * have to worry about atomic accesses.
  */
 #define TS_COMPAT		0x0002	/* 32bit syscall active (64BIT)*/
+#define TS_COMPAT_XXX		0x0008
 
 /*
  * Set IOPL bits in EFLAGS from given mask
diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index 763af1d..b3b98ff 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -785,7 +785,7 @@ static inline unsigned long get_nr_restart_syscall(const struct pt_regs *regs)
 	 * than the tracee.
 	 */
 #ifdef CONFIG_IA32_EMULATION
-	if (current->thread.status & (TS_COMPAT|TS_I386_REGS_POKED))
+	if (current->thread.status & TS_COMPAT_XXX)
 		return __NR_ia32_restart_syscall;
 #endif
 #ifdef CONFIG_X86_X32_ABI

[toc] | [prev] | [next] | [standalone]

#1612157

On Wed, Mar 29, 2017 at 8:05 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 03/28, Oleg Nesterov wrote:
>>
>> On 03/28, Andy Lutomirski wrote:
>> >
>> > How about we store the syscall arch to be restored in task_struct
>> > along with restart_block?
>>
>> Yes, perhaps we will have to finally do this. Not really nice too.
>
> OK, how about the hack below?
>
> I do not want to a new member into task_struct/restart_block, so the
> patch below adds a sticky TS_COMPAT bit which logically is a member
> of "struct restart_block".

Okay, but I'd much rather we just added a helper that's called in the
few places that actually write to restart_block.

Or we just add the new syscall nr and see what breaks.  The answer
could well be nothing at all.

--Andy

[toc] | [prev] | [next] | [standalone]

#1613164

On 03/29, Andy Lutomirski wrote:
>
> On Wed, Mar 29, 2017 at 8:05 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> > On 03/28, Oleg Nesterov wrote:
> >>
> >> On 03/28, Andy Lutomirski wrote:
> >> >
> >> > How about we store the syscall arch to be restored in task_struct
> >> > along with restart_block?
> >>
> >> Yes, perhaps we will have to finally do this. Not really nice too.
> >
> > OK, how about the hack below?
> >
> > I do not want to a new member into task_struct/restart_block, so the
> > patch below adds a sticky TS_COMPAT bit which logically is a member
> > of "struct restart_block".
>
> Okay, but I'd much rather we just added a helper that's called in the
> few places that actually write to restart_block.

Oh, yes, I thought about this too. This obviously needs more changes, and
every arch needs a dummy definition... I was thinking about

	static inline long setup_restart_block(void)
	{
		if (TS_COMPAT)
			set TS_COMPAT_XXX;
		else
			clear TS_COMPAT_XXX;

		return -ERESTART_RESTARTBLOCK;
	}

so that we can do

	-	ret = -ERESTART_RESTARTBLOCK;
	+	ret = setup_restart_block();

but I don't really like this... Do you strongly prefer it over the
-ERESTART_RESTARTBLOCK check in syscall_return_slowpath? I agree it doesn't
look nice too but it connects to other TS_ magic we do in arch/x86/entry/,
perhaps it is not that bad...

> Or we just add the new syscall nr and see what breaks.  The answer
> could well be nothing at all.

Well, strace knows about __NR_restart_syscall. It won't be really broken,
but I guess it will report something like "unknown syscall" rather than
restart_syscall(...).

However, this still looks like a best solution to me, just I have no idea
how much we can confuse user-space.

Oleg.

[toc] | [prev] | [next] | [standalone]

#1613358

On Thu, Mar 30, 2017 at 8:28 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 03/29, Andy Lutomirski wrote:
>>
>> On Wed, Mar 29, 2017 at 8:05 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>> > On 03/28, Oleg Nesterov wrote:
>> >>
>> >> On 03/28, Andy Lutomirski wrote:
>> >> >
>> >> > How about we store the syscall arch to be restored in task_struct
>> >> > along with restart_block?
>> >>
>> >> Yes, perhaps we will have to finally do this. Not really nice too.
>> >
>> > OK, how about the hack below?
>> >
>> > I do not want to a new member into task_struct/restart_block, so the
>> > patch below adds a sticky TS_COMPAT bit which logically is a member
>> > of "struct restart_block".
>>
>> Okay, but I'd much rather we just added a helper that's called in the
>> few places that actually write to restart_block.
>
> Oh, yes, I thought about this too. This obviously needs more changes, and
> every arch needs a dummy definition... I was thinking about
>
>         static inline long setup_restart_block(void)
>         {
>                 if (TS_COMPAT)
>                         set TS_COMPAT_XXX;
>                 else
>                         clear TS_COMPAT_XXX;
>
>                 return -ERESTART_RESTARTBLOCK;
>         }
>
> so that we can do
>
>         -       ret = -ERESTART_RESTARTBLOCK;
>         +       ret = setup_restart_block();
>
> but I don't really like this... Do you strongly prefer it over the
> -ERESTART_RESTARTBLOCK check in syscall_return_slowpath? I agree it doesn't
> look nice too but it connects to other TS_ magic we do in arch/x86/entry/,
> perhaps it is not that bad...

How about:

struct restart_block *restart = set_syscall_restart_fn(do_whatever);
restart->other_stuff = blah.

I'd rather avoid adding stuff to the slow path that runs *that* rarely.

>
>> Or we just add the new syscall nr and see what breaks.  The answer
>> could well be nothing at all.
>
> Well, strace knows about __NR_restart_syscall. It won't be really broken,
> but I guess it will report something like "unknown syscall" rather than
> restart_syscall(...).
>
> However, this still looks like a best solution to me, just I have no idea
> how much we can confuse user-space.

Me neither.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web