Date:	Wed, 11 May 2016 13:49:01 -0700
From:	Kees Cook <keescook@...omium.org>
To:	Hector Marco-Gisbert <hecmargi@....es>,
	Andy Lutomirski <luto@...nel.org>,
	Andi Kleen <ak@...ux.intel.com>
Cc:	LKML <linux-kernel@...r.kernel.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>,
	"H. Peter Anvin" <hpa@...or.com>,
	"x86@...nel.org" <x86@...nel.org>, Brian Gerst <brgerst@...il.com>,
	Borislav Petkov <bp@...e.de>,
	Huaitong Han <huaitong.han@...el.com>,
	Ismael Ripoll Ripoll <iripoll@....es>
Subject: Re: [PATCH] x86_64: Disabling read-implies-exec when the stack is executable

On Wed, May 11, 2016 at 3:45 AM, Hector Marco-Gisbert <hecmargi@....es> wrote:
> The READ_IMPLIES_EXEC personality was removed in 2005 for 64-bit processes,
> (commit a3cc2546a54361b86b73557df5b85c4fc3fc27c3 form history.git).
>
> But it's still possible to have all readable areas with EXEC permissions by
> setting the stack as executable in 64-bit ELF executables (also in 32-bit).

My memory is fuzzy here, but IIRC, RIE is needed for loading binaries
that have no concept of no-exec permissions. In those cases, there's
no way to tell if the process expected to need execute permissions in
arbitrary memory regions.

> This is because the macro elf_read_implies_exec() does not distinguish
> between 32 and 64-bit executables: when the stack is executable then the
> read-implies-exec personality is set (enabled) to the process.

However, I would tend to agree: RIE should only be needed on 32-bit
since 64-bit started its life knowing about no-exec permissions.

set_personality_64bit()'s (which is confusingly just an initializer
and not called during the personality() syscall) comment about this
makes no sense to me:

        /* TBD: overwrites user setup. Should have two bits.
           But 64bit processes have always behaved this way,
           so it's not too bad. The main problem is just that
           32bit childs are affected again. */
        current->personality &= ~READ_IMPLIES_EXEC;

> We think that this is not a desirable behaviour, maybe read-implies-exec
> could be used via personality but not by setting the stack as executable in
> the ELF.

I'd like to point out there are two cases here:

- if there is no PT_GNU_STACK marking, RIE is set for both 32-bit and 64-bit.
- if PT_GNU_STACK == EXSTACK_ENABLE_X, RIE is set for both 32-bit and 64-bit.

For 32-bit, these are both correct since a missing stack mark means we
must assume it might need executable memory in unexpected places. For
32-bit if it's marked with an executable stack, it's pretty certain
it's going to need exec memory in unexpected places.

For 64-bit, the first is wrong: the default for 64-bit is a non-exec
stack, so RIE shouldn't get triggered. The question remains: is RIE
needed for explicitly marked PT_GNU_STACKs?

My understanding matches yours: 64-bit never wants RIE, but I'd like
to better understand why. :)

> For x86_64 processes, is there any reason to disable read-implies-exec
> personality and at the same time enable it when the stack is executable ?

I've expanded the CC to folks that might know better.

> With this patch it's no longer possible to enable the read-implies-exec on
> the process by setting the stack as executable in the PT_GNU_STACK on
> x86_64 executables.
>
> Regarding 32-bits processes, is there any reason to enable
> read-implies-exec by setting the stack as executable instead of using the
> personality on X86_32 or when emulating IA32 on X86_64 ?

Yeah, this is for backward compatibility with ancient binaries that
lacked PT_GNU_STACK. This needs to stay, IMO.

> If not, I could re-send the patch which removes this possibility.
>
>
> Signed-off-by: Hector Marco-Gisbert <hecmargi@....es>
> Acked-by: Ismael Ripoll Ripoll <iripoll@....es>
> ---
>  arch/x86/include/asm/elf.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/include/asm/elf.h b/arch/x86/include/asm/elf.h
> index 15340e3..87fd15e 100644
> --- a/arch/x86/include/asm/elf.h
> +++ b/arch/x86/include/asm/elf.h
> @@ -271,8 +271,8 @@ extern int force_personality32;
>   * An executable for which elf_read_implies_exec() returns TRUE will
>   * have the READ_IMPLIES_EXEC personality flag set automatically.
>   */
> -#define elf_read_implies_exec(ex, executable_stack)    \
> -       (executable_stack != EXSTACK_DISABLE_X)
> +#define elf_read_implies_exec(ex, executable_stack) \
> +       (mmap_is_ia32() ? (executable_stack != EXSTACK_DISABLE_X) : 0)
>
>  struct task_struct;
>
> --
> 1.9.1
>

Regardless, I think the comment above this needs to be expanded to
explain why what's happening is happening, since currently the comment
just says the same thing as the code. :)

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

Powered by blists - more mailing lists