svn commit: r226343 - head/sys/vm

[Garrett Cooper]

On Fri, Oct 14, 2011 at 11:24 AM, Kostik Belousov <kostikbel at gmail.com> wrote:
> On Thu, Oct 13, 2011 at 06:50:30PM -0400, David Schultz wrote:
>> On Thu, Oct 13, 2011, Marcel Moolenaar wrote:
>> >
>> > On Oct 13, 2011, at 2:07 PM, John Baldwin wrote:
>> > >>
>> > >> That's really besides the point. ABI changes are made deliberately
>> > >> and ABIs must be well-documented for anyone to adhere to it. You
>> > >> can't post hoc wave your hand and say that at some unspecified time
>> > >> in the past the ABI changed: at what precise time does "supported
>> > >> by hardware mean" and how does that tie to a major FreeBSD version?
>> > >>
>> > >> Point in case: the JDK 1.4.x still works on FreeBSD 9.x (i386), so
>> > >> the ABI really hasn't changed at all in that respect.
>> > >
>> > > I think if you booted a FreeBSD 9.x i386 PAE kernel you'd find that the
>> > > jdk did not work.  That will be true for any i386 PAE kernel back to
>> > > when PG_NX support was introduced.
>> >
>> > That's bad.
> After more thought about the issue, I do not agree with you.
> Elf specification says about the PF_R flag that only read permission
> for the memory image of the segment is required, but read and execute
> is allowed.
>
> In other words, it is a bug in the old jre, which is further confirmed
> by the fact that later jres run with non-executable head.
>
>>
>> Recent binutils support a PT_GNU_HEAP flag in the ELF header that
>> controls whether heap allocations are executable by default.  In
>> Linux, the flag can be set using an ld option or the execstack(8)
>> command.  That seems like a better way to go than breaking old
>> JVMs or disabling the security feature.
>
> No, recent binutils do not support PT_GNU_HEAD. git grep -e PT_GNU_HEAD
> on the up to date checkout of binutils head gives zero matches. There are
> indeed PT_GNU_HEAP patches floating around from some hardening projects.
>
> There is indeed PT_GNU_STACK, which we do support.
>
> I want to commit the following refinement:
>
> diff --git a/sys/compat/freebsd32/freebsd32_misc.c b/sys/compat/freebsd32/freebsd32_misc.c
> index 6638ec8..fc2932b 100644
> --- a/sys/compat/freebsd32/freebsd32_misc.c
> +++ b/sys/compat/freebsd32/freebsd32_misc.c
> @@ -445,7 +445,7 @@ freebsd32_mprotect(struct thread *td, struct freebsd32_mprotect_args *uap)
>        ap.len = uap->len;
>        ap.prot = uap->prot;
>  #if defined(__amd64__) || defined(__ia64__)
> -       if (ap.prot & PROT_READ)
> +       if (i386_read_exec && (ap.prot & PROT_READ) != 0)
>                ap.prot |= PROT_EXEC;
>  #endif
>        return (sys_mprotect(td, &ap));
> @@ -536,7 +536,7 @@ freebsd32_mmap(struct thread *td, struct freebsd32_mmap_args *uap)
>  #endif
>
>  #if defined(__amd64__) || defined(__ia64__)
> -       if (prot & PROT_READ)
> +       if (i386_read_exec && (prot & PROT_READ))
>                prot |= PROT_EXEC;
>  #endif
>
> diff --git a/sys/kern/imgact_elf.c b/sys/kern/imgact_elf.c
> index 669c652..9970386 100644
> --- a/sys/kern/imgact_elf.c
> +++ b/sys/kern/imgact_elf.c
> @@ -118,11 +118,24 @@ static int elf_legacy_coredump = 0;
>  SYSCTL_INT(_debug, OID_AUTO, __elfN(legacy_coredump), CTLFLAG_RW,
>     &elf_legacy_coredump, 0, "");
>
> -static int __elfN(nxstack) = 0;
> +static int __elfN(nxstack) =
> +#if defined(__amd64__) || defined(__powerpc__) /* both 64 and 32 bit */
> +       1;
> +#else
> +       0;
> +#endif
>  SYSCTL_INT(__CONCAT(_kern_elf, __ELF_WORD_SIZE), OID_AUTO,
>     nxstack, CTLFLAG_RW, &__elfN(nxstack), 0,
>     __XSTRING(__CONCAT(ELF, __ELF_WORD_SIZE)) ": enable non-executable stack");
>
> +#if __ELF_WORD_SIZE == 32
> +#if defined(__amd64__) || defined(__ia64__)
> +int i386_read_exec = 0;
> +SYSCTL_INT(_kern_elf32, OID_AUTO, read_exec, CTLFLAG_RW, &i386_read_exec, 0,
> +    "enable execution from readable segments");
> +#endif
> +#endif
> +
>  static Elf_Brandinfo *elf_brand_list[MAX_BRANDS];
>
>  #define        trunc_page_ps(va, ps)   ((va) & ~(ps - 1))
> @@ -1666,7 +1679,7 @@ __elfN(trans_prot)(Elf_Word flags)
>                prot |= VM_PROT_READ;
>  #if __ELF_WORD_SIZE == 32
>  #if defined(__amd64__) || defined(__ia64__)
> -       if (flags & PF_R)
> +       if (i386_read_exec && (flags & PF_R))
>                prot |= VM_PROT_EXECUTE;
>  #endif
>  #endif
> diff --git a/sys/sys/sysent.h b/sys/sys/sysent.h
> index 6a4b485..3694ceb 100644
> --- a/sys/sys/sysent.h
> +++ b/sys/sys/sysent.h
> @@ -151,6 +152,10 @@ extern struct sysentvec null_sysvec;
>  extern struct sysent sysent[];
>  extern const char *syscallnames[];
>
> +#if defined(__amd64__) || defined(__ia64__)
> +extern int i386_read_exec;
> +#endif
> +
>  #define        NO_SYSCALL (-1)
>
>  struct module;
> diff --git a/sys/vm/vm_unix.c b/sys/vm/vm_unix.c
> index d4ea3b7..253ab77 100644
> --- a/sys/vm/vm_unix.c
> +++ b/sys/vm/vm_unix.c
> @@ -141,7 +141,7 @@ sys_obreak(td, uap)
>                prot = VM_PROT_RW;
>  #ifdef COMPAT_FREEBSD32
>  #if defined(__amd64__) || defined(__ia64__)
> -               if (SV_PROC_FLAG(td->td_proc, SV_ILP32))
> +               if (i386_read_exec && SV_PROC_FLAG(td->td_proc, SV_ILP32))
>                        prot |= VM_PROT_EXECUTE;
>  #endif
>  #endif

    Would you want to be able to toggle this feature at runtime after
you've fully booted your kernel? It seems like it would be kind of
dangerous to implement it on a system wide basis with part of the
pages NX and the others not NX -- in particular if multiple processes
are modifying the stack simultaneously.
    Also, would the sysctl be atomic enough for this to be a good idea?
    Personally I think that it would be better if this was a tunable
(and a readonly sysctl so folks could programmatically detect whether
or not NX was on), but that might just be me..
Thanks,
-Garrett