linux-kernel - Re: [PATCH 0/8] use struct pt_regs based syscall calling for x86-64

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]

Message-ID: <20180406092046.2jqjdzvecmfm6sif@gmail.com>
Date:   Fri, 6 Apr 2018 11:20:46 +0200
From:   Ingo Molnar <mingo@...nel.org>
To:     Dominik Brodowski <linux@...inikbrodowski.net>
Cc:     linux-kernel@...r.kernel.org, Al Viro <viro@...iv.linux.org.uk>,
        Andi Kleen <ak@...ux.intel.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Andy Lutomirski <luto@...nel.org>,
        Brian Gerst <brgerst@...il.com>,
        Denys Vlasenko <dvlasenk@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>, Ingo Molnar <mingo@...hat.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Thomas Gleixner <tglx@...utronix.de>, x86@...nel.org
Subject: Re: [PATCH 0/8] use struct pt_regs based syscall calling for x86-64


* Dominik Brodowski <linux@...inikbrodowski.net> wrote:

> On Fri, Apr 06, 2018 at 10:23:22AM +0200, Ingo Molnar wrote:
> > 
> > * Dominik Brodowski <linux@...inikbrodowski.net> wrote:
> > 
> > > On Thu, Apr 05, 2018 at 05:19:33PM +0200, Ingo Molnar wrote:
> > > > Ok, this series looks mostly good to me, but AFAICS this breaks the UML build:
> > > > 
> > > >  make[2]: *** No rule to make target 'archheaders'.  Stop.
> > > >  arch/um/Makefile:119: recipe for target 'archheaders' failed
> > > >  make[1]: *** [archheaders] Error 2
> > > >  make[1]: *** Waiting for unfinished jobs....
> > > 
> > > Ah, that's caused by patch 8/8 which I did and do not like all that much
> > > anyway: UML re-uses syscall_64.tbl which now has x86-specific entries like
> > > __sys_x86_pread64, but expects the generic syscall stub sys_pread64
> > > referenced there. Fixup patch below; could be folded with patch 8/8. Or
> > > patch 8/8 could simply be dropped from the series altogether...
> > 
> > I still like the 'truth in advertising' aspect. For example if I see this in the 
> > syscall table:
> > 
> >  10      common  mprotect                __sys_x86_mprotect
> > 
> > I can immediately find the _real_ syscall entry point:
> > 
> > ffffffff81180a10 <__sys_x86_mprotect>:
> > ffffffff81180a10:       48 8b 57 60             mov    0x60(%rdi),%rdx
> > ffffffff81180a14:       48 8b 77 68             mov    0x68(%rdi),%rsi
> > ffffffff81180a18:       b9 ff ff ff ff          mov    $0xffffffff,%ecx
> > ffffffff81180a1d:       48 8b 7f 70             mov    0x70(%rdi),%rdi
> > ffffffff81180a21:       e8 fa fc ff ff          callq  ffffffff81180720 <do_mprotect_pkey>
> > ffffffff81180a26:       48 98                   cltq   
> > ffffffff81180a28:       c3                      retq   
> > ffffffff81180a29:       0f 1f 80 00 00 00 00    nopl   0x0(%rax)
> > 
> > If, on the other hand, I see this entry:
> > 
> >  10     common  mprotect                sys_mprotect
> > 
> > Then, as a first step, no symbol anywhere matches with this:
> > 
> >  triton:~/tip> grep sys_mprotect System.map 
> >  triton:~/tip> 
> > 
> > "sys_mprotect" does not exist in any easily discoverable sense. You have to *know* 
> > to replace the sys_ prefix with __sys_x86_ to find it.
> > 
> > Now arguably we could use a __sys_ prefix instead of the grep-barrier __sys_x86 
> > prefix - but that too would be somewhat confusing I think.
> 
> Well, if looking at the ARCH="um" kernel, you won't find the __sys_x86_mprotect 
> there in its System.map -- so we either have to disentangle um and plain x86, or 
> live with some cause for confusion.

I'm primarily concerned about everything making sense on x86 - UML is an entirely 
separate architecture with heavy tradeoffs and kludges.

> __sys_mprotect as prefix won't work by the way, as the double-underscore __sys_ 
> variant is already used in net/* for internal syscall helpers.

Ok - then triple underscore - but overall I think it's more confusing.

Btw., what was the problem with calling the x86 ptregs wrapper sys_mprotect?

The only reason I suggested the __sys_x86_ prefix was because you originally 
suggested that there's symbol name overlap, but I don't think that's the case 
within the same kernel build, as the regular non-ptregs prototype:

  asmlinkage long sys_mprotect(unsigned long start, size_t len, unsigned long prot);

... will only exist on !CONFIG_ARCH_HAS_SYSCALL_WRAPPER kernels.

So maybe that's the simplest and least confusing solution.

Thanks,

	Ingo