Re: [PATCH v5 9/9] riscv: add STRICT_KERNEL_RWX support

From: Damien Le Moal <Damien.LeMoal@wdc.com>
To: Palmer Dabbelt <palmer@dabbelt.com>,
	"zong.li@sifive.com" <zong.li@sifive.com>
Cc: "aou@eecs.berkeley.edu" <aou@eecs.berkeley.edu>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"mhiramat@kernel.org" <mhiramat@kernel.org>,
	Paul Walmsley <paul.walmsley@sifive.com>,
	"linux-riscv@lists.infradead.org"
	<linux-riscv@lists.infradead.org>
Subject: Re: [PATCH v5 9/9] riscv: add STRICT_KERNEL_RWX support
Date: Sun, 19 Apr 2020 23:50:53 +0000	[thread overview]
Message-ID: <BY5PR04MB69001DE1B20907D2BA8F0BEDE7D70@BY5PR04MB6900.namprd04.prod.outlook.com> (raw)
In-Reply-To: mhng-09f91ec8-5821-41ad-a743-3842ca10f9e2@palmerdabbelt-glaptop1

On 2020/04/18 9:30, Palmer Dabbelt wrote:
> On Wed, 08 Apr 2020 00:57:04 PDT (-0700), zong.li@sifive.com wrote:
>> The commit contains that make text section as non-writable, rodata
>> section as read-only, and data section as non-executable.
>>
>> The init section should be changed to non-executable.
>>
>> Signed-off-by: Zong Li <zong.li@sifive.com>
>> ---
>>  arch/riscv/Kconfig                  |  1 +
>>  arch/riscv/include/asm/set_memory.h |  8 ++++++
>>  arch/riscv/mm/init.c                | 44 +++++++++++++++++++++++++++++
>>  3 files changed, 53 insertions(+)
>>
>> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
>> index 1e1efc998baf..58b556167d59 100644
>> --- a/arch/riscv/Kconfig
>> +++ b/arch/riscv/Kconfig
>> @@ -61,6 +61,7 @@ config RISCV
>>  	select ARCH_HAS_GIGANTIC_PAGE
>>  	select ARCH_HAS_SET_DIRECT_MAP
>>  	select ARCH_HAS_SET_MEMORY
>> +	select ARCH_HAS_STRICT_KERNEL_RWX

This should be:

	select ARCH_HAS_STRICT_KERNEL_RWX if !MMU

This option does not make sense for the no MMU case and more importantly, it
ends up generating gigantic binary images which breaks things like the K210 support.

Palmer,

I sent a patch to fix that. You did not reply/comment on it.

>>  	select ARCH_WANT_HUGE_PMD_SHARE if 64BIT
>>  	select SPARSEMEM_STATIC if 32BIT
>>  	select ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT if MMU
>> diff --git a/arch/riscv/include/asm/set_memory.h b/arch/riscv/include/asm/set_memory.h
>> index 4c5bae7ca01c..c38df4771c09 100644
>> --- a/arch/riscv/include/asm/set_memory.h
>> +++ b/arch/riscv/include/asm/set_memory.h
>> @@ -22,6 +22,14 @@ static inline int set_memory_x(unsigned long addr, int numpages) { return 0; }
>>  static inline int set_memory_nx(unsigned long addr, int numpages) { return 0; }
>>  #endif
>>
>> +#ifdef CONFIG_STRICT_KERNEL_RWX
>> +void set_kernel_text_ro(void);
>> +void set_kernel_text_rw(void);
>> +#else
>> +static inline void set_kernel_text_ro(void) { }
>> +static inline void set_kernel_text_rw(void) { }
>> +#endif
>> +
>>  int set_direct_map_invalid_noflush(struct page *page);
>>  int set_direct_map_default_noflush(struct page *page);
>>
>> diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
>> index fab855963c73..b55be44ff9bd 100644
>> --- a/arch/riscv/mm/init.c
>> +++ b/arch/riscv/mm/init.c
>> @@ -12,6 +12,7 @@
>>  #include <linux/sizes.h>
>>  #include <linux/of_fdt.h>
>>  #include <linux/libfdt.h>
>> +#include <linux/set_memory.h>
>>
>>  #include <asm/fixmap.h>
>>  #include <asm/tlbflush.h>
>> @@ -477,6 +478,17 @@ static void __init setup_vm_final(void)
>>  	csr_write(CSR_SATP, PFN_DOWN(__pa_symbol(swapper_pg_dir)) | SATP_MODE);
>>  	local_flush_tlb_all();
>>  }
>> +
>> +void free_initmem(void)
>> +{
>> +	unsigned long init_begin = (unsigned long)__init_begin;
>> +	unsigned long init_end = (unsigned long)__init_end;
>> +
>> +	/* Make the region as non-execuatble. */
>> +	set_memory_nx(init_begin, (init_end - init_begin) >> PAGE_SHIFT);
>> +	free_initmem_default(POISON_FREE_INITMEM);
>> +}
>> +
>>  #else
>>  asmlinkage void __init setup_vm(uintptr_t dtb_pa)
>>  {
>> @@ -488,6 +500,38 @@ static inline void setup_vm_final(void)
>>  }
>>  #endif /* CONFIG_MMU */
>>
>> +#ifdef CONFIG_STRICT_KERNEL_RWX
>> +void set_kernel_text_rw(void)
>> +{
>> +	unsigned long text_start = (unsigned long)_text;
>> +	unsigned long text_end = (unsigned long)_etext;
>> +
>> +	set_memory_rw(text_start, (text_end - text_start) >> PAGE_SHIFT);
>> +}
>> +
>> +void set_kernel_text_ro(void)
>> +{
>> +	unsigned long text_start = (unsigned long)_text;
>> +	unsigned long text_end = (unsigned long)_etext;
>> +
>> +	set_memory_ro(text_start, (text_end - text_start) >> PAGE_SHIFT);
>> +}
>> +
>> +void mark_rodata_ro(void)
>> +{
>> +	unsigned long text_start = (unsigned long)_text;
>> +	unsigned long text_end = (unsigned long)_etext;
>> +	unsigned long rodata_start = (unsigned long)__start_rodata;
>> +	unsigned long data_start = (unsigned long)_data;
>> +	unsigned long max_low = (unsigned long)(__va(PFN_PHYS(max_low_pfn)));
>> +
>> +	set_memory_ro(text_start, (text_end - text_start) >> PAGE_SHIFT);
>> +	set_memory_ro(rodata_start, (data_start - rodata_start) >> PAGE_SHIFT);
>> +	set_memory_nx(rodata_start, (data_start - rodata_start) >> PAGE_SHIFT);
>> +	set_memory_nx(data_start, (max_low - data_start) >> PAGE_SHIFT);
>> +}
>> +#endif
>> +
>>  void __init paging_init(void)
>>  {
>>  	setup_vm_final();
> 
> Reviewed-by: Palmer Dabbelt <palmerdabbelt@google.com>
> 
> 

-- 
Damien Le Moal
Western Digital Research