On 03/30, Oleg Nesterov wrote:
>
> On 03/29, Linus Torvalds wrote:
> >
> > On Wed, Mar 29, 2017 at 11:50 AM, Oleg Nesterov <o...@redhat.com> wrote:
> > >
> > > Again, afaics we only need these compat checks because regs->ax could be
> > > changed by 32-bit debugger without sign-extension.
> >
> > You don't explain how you were planning on *fixing* that code. You
> > know why it exists, but then you just say "let's remove it", without
> > any explanation of what you'd replace it with.
>
> Hmm. I tried to explain... Let me quote my initial email,
>
> So why we can't simply change putreg32() to always sign-extend regs->ax
> regs->orig_ax and just do
>
> static inline long syscall_get_error(struct task_struct *task,
> struct pt_regs *regs)
> {
> return regs-ax;
> }
>
> ? Or, better, simply kill it and use syscall_get_return_value() in
> arch/x86/kernel/signal.c.
>
> Of course, if the tracee is 64-bit and debugger is 32-bit then the
> unconditional sign-extend can be wrong, but do we really care about
> this case? This can't really work anyway. And the current code is not
> right too. Say, debugger nacks the signal which interrupted syscall
> and sets regs->ax = -ERESTARTSYS to force the restart, this won't work
> because TS_COMPAT|TS_I386_REGS_POKED are not set.
>
> In short. can the patch below work?
>
> Oleg.
>
> diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
> index 9cc7d5a..96f21fc 100644
> --- a/arch/x86/kernel/ptrace.c
> +++ b/arch/x86/kernel/ptrace.c
> @@ -917,11 +917,14 @@ static int putreg32(struct task_struct *child, unsigned
> regno, u32 value)
> R32(edi, di);
> R32(esi, si);
> R32(ebp, bp);
> - R32(eax, ax);
> R32(eip, ip);
> R32(esp, sp);
>
> case offsetof(struct user32, regs.orig_eax):
^^^^^^^^^^^^^^
damn, just in case, of course this is typo, should be
case offsetof(struct user32, regs.eax);
> + regs->ax = (long) (int) value;
> + break;
> +
> + case offsetof(struct user32, regs.orig_eax):
> /*
> * Warning: bizarre corner case fixup here. A 32-bit
> * debugger setting orig_eax to -1 wants to disable
> diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
> index b3b98ff..41023f8 100644
> --- a/arch/x86/kernel/signal.c
> +++ b/arch/x86/kernel/signal.c
> @@ -710,7 +710,7 @@ handle_signal(struct ksignal *ksig, struct pt_regs *regs)
> /* Are we from a system call? */
> if (syscall_get_nr(current, regs) >= 0) {
> /* If so, check system call restarting.. */
> - switch (syscall_get_error(current, regs)) {
> + switch (syscall_get_return_value(current, regs)) {
> case -ERESTART_RESTARTBLOCK:
> case -ERESTARTNOHAND:
> regs->ax = -EINTR;
> @@ -813,7 +813,7 @@ void do_signal(struct pt_regs *regs)
> /* Did we come from a system call? */
> if (syscall_get_nr(current, regs) >= 0) {
> /* Restart the system call - no handlers present */
> - switch (syscall_get_error(current, regs)) {
> + switch (syscall_get_return_value(current, regs)) {
> case -ERESTARTNOHAND:
> case -ERESTARTSYS:
> case -ERESTARTNOINTR:
