Date: Fri, 14 Oct 2011 11:30:47 -0700 From: Garrett Cooper <yanegomi@gmail.com> To: Kostik Belousov <kostikbel@gmail.com> Cc: Marcel Moolenaar <marcel@freebsd.org>, John Baldwin <jhb@freebsd.org>, svn-src-all@freebsd.org, Marcel Moolenaar <marcel@xcllnt.net>, src-committers@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r226343 - head/sys/vm Message-ID: <CAGH67wRh-VfGqSAZ3-z42W--yd6DmTSabsKU56y21YieB=pxew@mail.gmail.com> In-Reply-To: <20111014182443.GP1511@deviant.kiev.zoral.com.ua> References: <201110131620.p9DGKAM2022926@svn.freebsd.org> <20111013190943.GM1511@deviant.kiev.zoral.com.ua> <D6F3D623-23A5-4147-A439-746AD670DE14@xcllnt.net> <201110131707.14466.jhb@freebsd.org> <CC78FD86-8B14-49EE-A2AA-4B1EE186A4BB@xcllnt.net> <20111013225030.GA75054@zim.MIT.EDU> <20111014182443.GP1511@deviant.kiev.zoral.com.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, Oct 14, 2011 at 11:24 AM, Kostik Belousov <kostikbel@gmail.com> wro=
te:
> 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. =A0That 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. =A0In
>> Linux, the flag can be set using an ld option or the execstack(8)
>> command. =A0That 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 freebsd3=
2_mprotect_args *uap)
> =A0 =A0 =A0 =A0ap.len =3D uap->len;
> =A0 =A0 =A0 =A0ap.prot =3D uap->prot;
> =A0#if defined(__amd64__) || defined(__ia64__)
> - =A0 =A0 =A0 if (ap.prot & PROT_READ)
> + =A0 =A0 =A0 if (i386_read_exec && (ap.prot & PROT_READ) !=3D 0)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0ap.prot |=3D PROT_EXEC;
> =A0#endif
> =A0 =A0 =A0 =A0return (sys_mprotect(td, &ap));
> @@ -536,7 +536,7 @@ freebsd32_mmap(struct thread *td, struct freebsd32_mm=
ap_args *uap)
> =A0#endif
>
> =A0#if defined(__amd64__) || defined(__ia64__)
> - =A0 =A0 =A0 if (prot & PROT_READ)
> + =A0 =A0 =A0 if (i386_read_exec && (prot & PROT_READ))
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0prot |=3D PROT_EXEC;
> =A0#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 =3D 0;
> =A0SYSCTL_INT(_debug, OID_AUTO, __elfN(legacy_coredump), CTLFLAG_RW,
> =A0 =A0 &elf_legacy_coredump, 0, "");
>
> -static int __elfN(nxstack) =3D 0;
> +static int __elfN(nxstack) =3D
> +#if defined(__amd64__) || defined(__powerpc__) /* both 64 and 32 bit */
> + =A0 =A0 =A0 1;
> +#else
> + =A0 =A0 =A0 0;
> +#endif
> =A0SYSCTL_INT(__CONCAT(_kern_elf, __ELF_WORD_SIZE), OID_AUTO,
> =A0 =A0 nxstack, CTLFLAG_RW, &__elfN(nxstack), 0,
> =A0 =A0 __XSTRING(__CONCAT(ELF, __ELF_WORD_SIZE)) ": enable non-executabl=
e stack");
>
> +#if __ELF_WORD_SIZE =3D=3D 32
> +#if defined(__amd64__) || defined(__ia64__)
> +int i386_read_exec =3D 0;
> +SYSCTL_INT(_kern_elf32, OID_AUTO, read_exec, CTLFLAG_RW, &i386_read_exec=
, 0,
> + =A0 =A0"enable execution from readable segments");
> +#endif
> +#endif
> +
> =A0static Elf_Brandinfo *elf_brand_list[MAX_BRANDS];
>
> =A0#define =A0 =A0 =A0 =A0trunc_page_ps(va, ps) =A0 ((va) & ~(ps - 1))
> @@ -1666,7 +1679,7 @@ __elfN(trans_prot)(Elf_Word flags)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0prot |=3D VM_PROT_READ;
> =A0#if __ELF_WORD_SIZE =3D=3D 32
> =A0#if defined(__amd64__) || defined(__ia64__)
> - =A0 =A0 =A0 if (flags & PF_R)
> + =A0 =A0 =A0 if (i386_read_exec && (flags & PF_R))
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0prot |=3D VM_PROT_EXECUTE;
> =A0#endif
> =A0#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;
> =A0extern struct sysent sysent[];
> =A0extern const char *syscallnames[];
>
> +#if defined(__amd64__) || defined(__ia64__)
> +extern int i386_read_exec;
> +#endif
> +
> =A0#define =A0 =A0 =A0 =A0NO_SYSCALL (-1)
>
> =A0struct 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)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0prot =3D VM_PROT_RW;
> =A0#ifdef COMPAT_FREEBSD32
> =A0#if defined(__amd64__) || defined(__ia64__)
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (SV_PROC_FLAG(td->td_proc, SV_ILP32))
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (i386_read_exec && SV_PROC_FLAG(td->td_p=
roc, SV_ILP32))
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0prot |=3D VM_PROT_EXECUTE;
> =A0#endif
> =A0#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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAGH67wRh-VfGqSAZ3-z42W--yd6DmTSabsKU56y21YieB=pxew>