linux-kernel - Re: syscall_get_error() && TS

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 30 Mar 2017 11:35:45 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Andy Lutomirski <luto@...capital.net>
Cc:     Oleg Nesterov <oleg@...hat.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Andy Lutomirski <luto@...nel.org>,
        Denys Vlasenko <dvlasenk@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>, Ingo Molnar <mingo@...hat.com>,
        Jan Kratochvil <jan.kratochvil@...hat.com>,
        Pedro Alves <palves@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        "the arch/x86 maintainers" <x86@...nel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: syscall_get_error() && TS_ checks

On Thu, Mar 30, 2017 at 11:23 AM, Andy Lutomirski <luto@...capital.net> wrote:
> On Thu, Mar 30, 2017 at 10:46 AM, Linus Torvalds
> <torvalds@...ux-foundation.org> wrote:
>>
>> We *really* shouldn't sign-extend that value if the debugger ends up
>> updating the pointer (or maybe the debugger just reloads previous
>> values, not really "updating" anything - I think that's what gdb does
>> when you do a call within the context of the debugged program from
>> within gdb, for example)
>
> Can you think of a case where this would actually matter?

I'd actually be willing to have people try, but then you'd have to
sign-extend *all* registers. No way in hell will I accept a patch that
randomly sign-extends just %eax.

And then actually run such a kernel on a 32-bit distro, and verifying
that things like gdb and strace really work. But it needs real
testing, not some kind of handwaving. It's a *big* change.

> --- issue 1 ---
>
> get_nr_restart_syscall() does:
>
>        if (current->thread.status & (TS_COMPAT|TS_I386_REGS_POKED))
>                return __NR_ia32_restart_syscall;
>
> This is very, very buggy

Quite frankly, a bug somewhere else is not an excuse for then making
other code buggier. I don't see what "issue 1" has to do with anything
what-so-ever,.

> --- issue 2 ---
>
> syscall_get_error().  This is available on all arches, but it appears
> to be used *only* on x86.

So I think that one is a red herring. We can trivially fix that by
just (a) removing everybody elses syscall_get_error(), and just
inlining the x86 case into the x86 signal handling. Boom, gone.

And in the signal handling path, the sign-extension and test of %eax
is reivially ok. Not because it's ok in general, but because we've
verified using %orig_eax that we're in a system call return path. We
could happily delete the whole TS_I386_REGS_POKED thing, I think.

But then it's very much about the fact that within the particular case
of the signal handling code and system call return detection, the
whole sign extension checking makes sense.

So don't even _try_ to equate this code with the general sign
extension code by ptrace. That is a totally different animal
altogether.

              Linus