Date: Tue, 18 Oct 2011 18:25:49 -0400 From: Arnaud Lacombe <lacombar@gmail.com> To: Kostik Belousov <kostikbel@gmail.com> Cc: Garrett Cooper <yanegomi@gmail.com>, current@freebsd.org, Oliver Pinter <oliver.pntr@gmail.com> Subject: Re: [RFC] Enable nxstack by default Message-ID: <CACqU3MXNpmhwUM-incmeU_vUXZOKaZ=sZmGmUX5WCmdz6kfE7A@mail.gmail.com> In-Reply-To: <20111018183219.GN50300@deviant.kiev.zoral.com.ua> References: <CAPjTQNFCT5LBKwVQFf9FHk4aTzrJ243j2uN1nPmMeFp=cTdMUA@mail.gmail.com> <20111018090750.GG50300@deviant.kiev.zoral.com.ua> <CACqU3MWftO=FG4GbnKCFjTcKg1narJWuYnCwv-Mcu=WGriScwA@mail.gmail.com> <alpine.BSF.2.00.1110180838200.38610@toaster.local> <CACqU3MWOXTMfu0LySukcwAz=NGSzyN=ettiY0fQj3Ehp5MONug@mail.gmail.com> <CAPjTQNE5-kGJ%2BD2c3Z2y-e_h95i5VY0Yc=C26BJ_Oq0n2DNz6A@mail.gmail.com> <CACqU3MXm1P1P2FBMCKhYOC%2BeCn_3QyQmd98b%2B_Kiq98usuqiPA@mail.gmail.com> <20111018183219.GN50300@deviant.kiev.zoral.com.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
Hi,
2011/10/18 Kostik Belousov <kostikbel@gmail.com>:
> 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@gmail.com> =
wrote:
>> > On 10/18/11, Arnaud Lacombe <lacombar@gmail.com> wrote:
>> >> Hi,
>> >>
>> >> On Tue, Oct 18, 2011 at 11:44 AM, Garrett Cooper <yanegomi@gmail.com>=
wrote:
>> >>> On Tue, 18 Oct 2011, Arnaud Lacombe wrote:
>> >>>
>> >>>> Hi,
>> >>>>
>> >>>> On Tue, Oct 18, 2011 at 5:07 AM, Kostik Belousov <kostikbel@gmail.c=
om>
>> >>>> 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 comment=
s,
>> >>>>>> 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 followi=
ng
>> >>>>> 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
>> >>>>> @@ -118,7 +118,12 @@ static int elf_legacy_coredump =3D 0;
>> >>>>> =A0SYSCTL_INT(_debug, OID_AUTO, __elfN(legacy_coredump), CTLFLAG_R=
W,
>> >>>>> =A0 =A0 &elf_legacy_coredump, 0, "");
>> >>>>>
>> >>>>> -static int __elfN(nxstack) =3D 0;
>> >>>>> +int __elfN(nxstack) =3D
>> >>>>> +#if defined(__amd64__) || defined(__powerpc64__) /* both 64 and 3=
2 bit
>> >>>>> */
>> >>>>>
>> >>>> Why leaving 32bits x86 CPU supporting the NX feature behind ?
>> >>>
>> >>> Most likely because it was assumed that i386 doesn't fully support i=
t.
>> >>> According to ye great Wikipedia, NX support didn't roll into i386 un=
til
>> >>> 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 ear=
ly
>> >>> Dual
>> >>> Core Pentiums, e.g. the Yonahs to not talk 64-bit. Not sure about AM=
D.
>> >>>
>> >>> There are probably more details in binutils, gcc, etc, that I'm miss=
ing
>> >>> 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 go=
t
>> >> 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.
>> >>
>> >> =A0- 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:
>>
>> @@ -118,7 +118,12 @@ 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;
>> +int __elfN(nxstack) =3D
>> +#if defined(PAE) || defined(__amd64__) || defined(__powerpc64__) /*
>> 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-executab=
le 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.
>
I will.
Btw, NetBSD has been going down the path of system unit test,
especially of kernel/userland interfaces, and already worked-out the
framework for that. Is that something FreeBSD would be interested in ?
Thanks,
- Arnaud
> 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
> @@ -650,6 +650,8 @@ enable_sse(void)
> =A0#endif
> =A0}
>
> +extern int elf32_nxstack;
> +
> =A0void
> =A0initializecpu(void)
> =A0{
> @@ -739,6 +741,7 @@ initializecpu(void)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0msr =3D rdmsr(MSR_EFER) | =
EFER_NXE;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0wrmsr(MSR_EFER, msr);
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0pg_nx =3D PG_NX;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 elf32_nxstack =3D 1;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0}
> =A0#endif
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0break;
> 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
> @@ -118,7 +118,12 @@ 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;
> +int __elfN(nxstack) =3D
> +#if defined(__amd64__) || defined(__powerpc64__) /* 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");
> 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
> @@ -1445,6 +1445,8 @@ moea64_uma_page_alloc(uma_zone_t zone, int bytes, u=
_int8_t *flags, int wait)
> =A0 =A0 =A0 =A0return (void *)va;
> =A0}
>
> +extern int elf32_nxstack;
> +
> =A0void
> =A0moea64_init(mmu_t mmu)
> =A0{
> @@ -1464,6 +1466,8 @@ moea64_init(mmu_t mmu)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0uma_zone_set_allocf(moea64_mpvo_zone,moea6=
4_uma_page_alloc);
> =A0 =A0 =A0 =A0}
>
> + =A0 =A0 =A0 elf32_nxstack =3D 1;
> +
> =A0 =A0 =A0 =A0moea64_initialized =3D TRUE;
> =A0}
>
> 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
> @@ -192,6 +192,8 @@ void print_kernel_section_addr(void);
> =A0void print_kenv(void);
> =A0u_int booke_init(uint32_t, uint32_t);
>
> +extern int elf32_nxstack;
> +
> =A0static void
> =A0cpu_e500_startup(void *dummy)
> =A0{
> @@ -227,6 +229,9 @@ cpu_e500_startup(void *dummy)
> =A0 =A0 =A0 =A0/* Set up buffers, so they can be used to read disk labels=
. */
> =A0 =A0 =A0 =A0bufinit();
> =A0 =A0 =A0 =A0vm_pager_bufferinit();
> +
> + =A0 =A0 =A0 /* Cpu supports execution permissions on the pages. */
> + =A0 =A0 =A0 elf32_nxstack =3D 1;
> =A0}
>
> =A0static char *
>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CACqU3MXNpmhwUM-incmeU_vUXZOKaZ=sZmGmUX5WCmdz6kfE7A>