Re: [PATCH 3/3] powerpc/mm: Mark __init memory no-execute when STRICT_KERNEL_RWX=y

[Christophe LEROY <christophe.leroy@c-s.fr>]

Le 09/08/2017 à 04:29, Michael Ellerman a écrit :
> Christophe LEROY <christophe.leroy@c-s.fr> writes:
>> Le 14/07/2017 à 08:51, Michael Ellerman a écrit :
>>> diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h b/arch/powerpc/include/asm/book3s/64/pgtable.h
>>> index c0737c86a362..3d562b210c65 100644
>>> --- a/arch/powerpc/include/asm/book3s/64/pgtable.h
>>> +++ b/arch/powerpc/include/asm/book3s/64/pgtable.h
>>> @@ -1192,5 +1192,12 @@ static inline const int pud_pfn(pud_t pud)
>>>    	BUILD_BUG();
>>>    	return 0;
>>>    }
>>> +
>>> +#ifdef CONFIG_STRICT_KERNEL_RWX
>>> +void mark_initmem_nx(void);
>>> +#else
>>> +static inline void mark_initmem_nx(void) { }
>>> +#endif
>>> +
>>
>> Why do we want to limit that to CONFIG_STRICT_KERNEL_RWX ?
>> Only the kernel text is marked X, even without CONFIG_STRICT_KERNEL_RWX
>> (at least on PPC32), so I believe we should clear X on init text in any
>> case, shouldn't we ?
> 
> You're right, but ..
> 
> On 64-bit when STRICT_KERNEL_RWX=n we make no effort to ensure the
> start/end of the init text is on a page boundary.
> 
> eg. on 64-bit hash we will typically use a 16M page to map the whole
> kernel, text/data/init_text/etc.

Some of the 32 bit also use some huge mapings like BATs or large pages, 
in which case it is pointless but not harmfull to fix the page tables 
anyway.
At least it is correct for the ones that use regular pages, and kernel 
can also be started with nobats or noltlbs at command line, in which 
case it is usefull to have the page tables correct.

> 
> So yes we *should* always mark it no-execute but in practice we can't
> because it's not page aligned.

On 32 bit it seems to always be aligned to the normal page size, so no 
problem.

> 
> But if that's different on (some?) 32-bit then we could introduce a new
> CONFIG symbol that is enabled in the right cases.

For the time being, I have added an "|| CONFIG_PPC32 " on the ifdef, is 
that OK ?
See https://patchwork.ozlabs.org/patch/796625/

Christophe