Re: [RFC] Enable nxstack by default

From: Kostik Belousov <kostikbel_at_gmail.com>
Date: Tue, 18 Oct 2011 21:32:19 +0300
On Tue, Oct 18, 2011 at 01:06:27PM -0400, Arnaud Lacombe wrote:
> Hi,
> 
> On Tue, Oct 18, 2011 at 12:53 PM, Oliver Pinter <oliver.pntr_at_gmail.com> wrote:
> > On 10/18/11, Arnaud Lacombe <lacombar_at_gmail.com> wrote:
> >> Hi,
> >>
> >> On Tue, Oct 18, 2011 at 11:44 AM, Garrett Cooper <yanegomi_at_gmail.com> wrote:
> >>> On Tue, 18 Oct 2011, Arnaud Lacombe wrote:
> >>>
> >>>> Hi,
> >>>>
> >>>> On Tue, Oct 18, 2011 at 5:07 AM, Kostik Belousov <kostikbel_at_gmail.com>
> >>>> wrote:
> >>>>>
> >>>>> On Mon, Oct 17, 2011 at 09:30:56PM +0200, Oliver Pinter wrote:
> >>>>>>
> >>>>>> Hi all!
> >>>>>>
> >>>>>> I think, it's the time to enable the nxstack feature. Any comments,
> >>>>>> pros, cons?
> >>>>>
> >>>>> I dragged the change long enough for it to miss the 9.0.
> >>>>> After the 9.0 is released, I will flip the switch with the following
> >>>>> change.
> >>>>>
> >>>>> diff --git a/sys/kern/imgact_elf.c b/sys/kern/imgact_elf.c
> >>>>> index 8455f48..926fe64 100644
> >>>>> --- a/sys/kern/imgact_elf.c
> >>>>> +++ b/sys/kern/imgact_elf.c
> >>>>> _at__at_ -118,7 +118,12 _at__at_ 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;
> >>>>> +int __elfN(nxstack) =
> >>>>> +#if defined(__amd64__) || defined(__powerpc64__) /* both 64 and 32 bit
> >>>>> */
> >>>>>
> >>>> Why leaving 32bits x86 CPU supporting the NX feature behind ?
> >>>
> >>> Most likely because it was assumed that i386 doesn't fully support it.
> >>> According to ye great Wikipedia, NX support didn't roll into i386 until
> >>> Prescott, which was pretty late in the non-64-bit capable family of CPUs,
> >>> as
> >>> its successor -- Conroe -- was 64-bit. Intel detuned some of the early
> >>> Dual
> >>> Core Pentiums, e.g. the Yonahs to not talk 64-bit. Not sure about AMD.
> >>>
> >>> There are probably more details in binutils, gcc, etc, that I'm missing
> >>> and
> >>> Kostik can expound on.
> >>>
> >> NX support is advertised in the cpuid flags, just add the logic to
> >> handle this interface. Kostik's patch is just incomplete, but he's got
> >> a commit bit so he can commit it as-is, as he will.
> >>
> >> If nonexec_stack becomes the default, it should be on every CPU
> >> supporting the feature, not just the low-hanging one.
> >>
> >> š- Arnaud
> >>
> >
> > the NX detection code already implemented in i386, but this feature
> > required PAE:
> >
> yes, this is the conclusion I reached too. But this does not change
> the fact that the VM should know about that, and this is missing from
> Kostik's patch. I guess the first hunk should read:
> 
> _at__at_ -118,7 +118,12 _at__at_ 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;
> +int __elfN(nxstack) =
> +#if defined(PAE) || defined(__amd64__) || defined(__powerpc64__) /*
> 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");

Your patch is not right, it will cause even more user confusion.
The presence of the PAE kernel does not imply that CPU supports nx.

Below is the updated patch that turns on nxstack by default for the PAE
kernels on NX-capable CPUs. Note that i386 usermode fully supports the
PT_GNU_STACK annotations and cares about not enabling nx on stack pages
unneccessary, but my main target was compat32 on amd64.

The fact that nxstack is not enabled by default does not prevent
administrator from manually enabling the feature.

Since you worried so much about PAE case, I sincerely expect that you
will test the change. Thank you in advance.

diff --git a/sys/i386/i386/initcpu.c b/sys/i386/i386/initcpu.c
index c2daf54..ec77adb 100644
--- a/sys/i386/i386/initcpu.c
+++ b/sys/i386/i386/initcpu.c
_at__at_ -650,6 +650,8 _at__at_ enable_sse(void)
 #endif
 }
 
+extern int elf32_nxstack;
+
 void
 initializecpu(void)
 {
_at__at_ -739,6 +741,7 _at__at_ initializecpu(void)
 			msr = rdmsr(MSR_EFER) | EFER_NXE;
 			wrmsr(MSR_EFER, msr);
 			pg_nx = PG_NX;
+			elf32_nxstack = 1;
 		}
 #endif
 		break;
diff --git a/sys/kern/imgact_elf.c b/sys/kern/imgact_elf.c
index 8455f48..926fe64 100644
--- a/sys/kern/imgact_elf.c
+++ b/sys/kern/imgact_elf.c
_at__at_ -118,7 +118,12 _at__at_ 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;
+int __elfN(nxstack) =
+#if defined(__amd64__) || defined(__powerpc64__) /* 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");
diff --git a/sys/powerpc/aim/mmu_oea64.c b/sys/powerpc/aim/mmu_oea64.c
index 7500462..0e27351 100644
--- a/sys/powerpc/aim/mmu_oea64.c
+++ b/sys/powerpc/aim/mmu_oea64.c
_at__at_ -1445,6 +1445,8 _at__at_ moea64_uma_page_alloc(uma_zone_t zone, int bytes, u_int8_t *flags, int wait)
 	return (void *)va;
 }
 
+extern int elf32_nxstack;
+
 void
 moea64_init(mmu_t mmu)
 {
_at__at_ -1464,6 +1466,8 _at__at_ moea64_init(mmu_t mmu)
 		uma_zone_set_allocf(moea64_mpvo_zone,moea64_uma_page_alloc);
 	}
 
+	elf32_nxstack = 1;
+
 	moea64_initialized = TRUE;
 }
 
diff --git a/sys/powerpc/booke/machdep.c b/sys/powerpc/booke/machdep.c
index c2b5e6f..82a37e1 100644
--- a/sys/powerpc/booke/machdep.c
+++ b/sys/powerpc/booke/machdep.c
_at__at_ -192,6 +192,8 _at__at_ void print_kernel_section_addr(void);
 void print_kenv(void);
 u_int booke_init(uint32_t, uint32_t);
 
+extern int elf32_nxstack;
+
 static void
 cpu_e500_startup(void *dummy)
 {
_at__at_ -227,6 +229,9 _at__at_ cpu_e500_startup(void *dummy)
 	/* Set up buffers, so they can be used to read disk labels. */
 	bufinit();
 	vm_pager_bufferinit();
+
+	/* Cpu supports execution permissions on the pages. */
+	elf32_nxstack = 1;
 }
 
 static char *

Received on Tue Oct 18 2011 - 16:32:38 UTC

This archive was generated by hypermail 2.4.0 : Wed May 19 2021 - 11:40:19 UTC