Date: Fri, 14 Oct 2011 21:53:06 +0300 From: Kostik Belousov <kostikbel@gmail.com> To: Garrett Cooper <yanegomi@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: <20111014185306.GQ1511@deviant.kiev.zoral.com.ua> In-Reply-To: <CAGH67wRh-VfGqSAZ3-z42W--yd6DmTSabsKU56y21YieB=pxew@mail.gmail.com> 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> <CAGH67wRh-VfGqSAZ3-z42W--yd6DmTSabsKU56y21YieB=pxew@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --] On Fri, Oct 14, 2011 at 11:30:47AM -0700, Garrett Cooper wrote: > On Fri, Oct 14, 2011 at 11:24 AM, Kostik Belousov <kostikbel@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 Yes, I do. > 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? It is atomic enough. > 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 [-- Attachment #2 --] -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.18 (FreeBSD) iEYEARECAAYFAk6YhRIACgkQC3+MBN1Mb4h44wCgvaoSL05myzO53pEa1LcERCAx YuAAmgN6sYsD3D4mOGwrmBXbnsvKO7em =/h2b -----END PGP SIGNATURE-----
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20111014185306.GQ1511>