linux-kernel - Re: [PATCH V2] x86/ftrace: Make sure that ftrace trampolines are not RWX

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: <alpine.DEB.2.20.1705260859290.1902@nanos>
Date:   Fri, 26 May 2017 09:03:13 +0200 (CEST)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Kees Cook <keescook@...omium.org>
cc:     "Luis R. Rodriguez" <mcgrof@...nel.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        LKML <linux-kernel@...r.kernel.org>,
        "x86@...nel.org" <x86@...nel.org>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH V2] x86/ftrace: Make sure that ftrace trampolines are
 not RWX

On Thu, 25 May 2017, Kees Cook wrote:
> On Thu, May 25, 2017 at 10:46 AM, Luis R. Rodriguez <mcgrof@...nel.org> wrote:
> > On Thu, May 25, 2017 at 10:57:51AM +0200, Thomas Gleixner wrote:
> >> ftrace use module_alloc() to allocate trampoline pages. The mapping of
> >> module_alloc() is RWX, which makes sense as the memory is written to right
> >> after allocation. But nothing makes these pages RO after writing to them.
> >>
> >> Add proper set_memory_rw/ro() calls to protect the trampolines after
> >> modification.
> >>
> >> Signed-off-by: Thomas Gleixner <tglx@...utronix.de>
> >> ---
> >>  arch/x86/kernel/ftrace.c |   20 ++++++++++++++------
> >>  1 file changed, 14 insertions(+), 6 deletions(-)
> >>
> >> --- a/arch/x86/kernel/ftrace.c
> >> +++ b/arch/x86/kernel/ftrace.c
> >> @@ -689,8 +689,12 @@ static inline void *alloc_tramp(unsigned
> >>  {
> >>       return module_alloc(size);
> >>  }
> >> -static inline void tramp_free(void *tramp)
> >> +static inline void tramp_free(void *tramp, int size)
> >>  {
> >> +     int npages = PAGE_ALIGN(size) >> PAGE_SHIFT;
> >> +
> >> +     set_memory_nx((unsigned long)tramp, npages);
> >> +     set_memory_rw((unsigned long)tramp, npages);
> >>       module_memfree(tramp);
> >>  }
> >
> > Can/should module_memfree() just do this for users? With Masami's fix that'd
> > be 2 users already.
> 
> It seems like it really should. That would put it in a single place
> and avoid this mistake again in the future. Does module_memfree() have
> access to the allocation size, or does that need to get plumbed?

No, it doesn't. But the number of instances is pretty limited.

Btw, looking at BPF. It allocates memory via module_alloc() which means
it's RWX. There is nothing in that BPF code which changes the permissions
afterwards ....

Thanks,

	tglx