diff mbox series

[v2] riscv: mm: Proper page permissions after initmem free

Message ID 20221115090641.258476-1-bjorn@kernel.org (mailing list archive)
State Accepted
Commit 6fdd5d2f8c2f54b7fad4ff4df2a19542aeaf6102
Delegated to: Palmer Dabbelt
Headers show
Series [v2] riscv: mm: Proper page permissions after initmem free | expand

Checks

Context Check Description
conchuod/cover_letter success Single patches do not need cover letters
conchuod/patch_count success Link
conchuod/tree_selection success Guessed tree name to be fixes
conchuod/fixes_present success Fixes tag present in non-next series
conchuod/kdoc success Errors and warnings before: 0 this patch: 0
conchuod/dtb_warn_rv64 success Errors and warnings before: 0 this patch: 0
conchuod/build_rv32_defconfig success Build OK
conchuod/checkpatch success total: 0 errors, 0 warnings, 0 checks, 15 lines checked
conchuod/source_inline success Was 0 now: 0
conchuod/build_warn_rv64 success Errors and warnings before: 0 this patch: 0
conchuod/build_rv64_nommu_k210_defconfig success Build OK
conchuod/verify_signedoff success Signed-off-by tag matches author and committer
conchuod/header_inline success No static functions without inline keyword in header files
conchuod/verify_fixes success Fixes tag looks correct
conchuod/module_param success Was 0 now: 0
conchuod/build_rv64_nommu_virt_defconfig success Build OK

Commit Message

Björn Töpel Nov. 15, 2022, 9:06 a.m. UTC
From: Björn Töpel <bjorn@rivosinc.com>

64-bit RISC-V kernels have the kernel image mapped separately to alias
the linear map. The linear map and the kernel image map are documented
as "direct mapping" and "kernel" respectively in [1].

At image load time, the linear map corresponding to the kernel image
is set to PAGE_READ permission, and the kernel image map is set to
PAGE_READ|PAGE_EXEC.

When the initmem is freed, the pages in the linear map should be
restored to PAGE_READ|PAGE_WRITE, whereas the corresponding pages in
the kernel image map should be restored to PAGE_READ, by removing the
PAGE_EXEC permission.

This is not the case. For 64-bit kernels, only the linear map is
restored to its proper page permissions at initmem free, and not the
kernel image map.

In practise this results in that the kernel can potentially jump to
dead __init code, and start executing invalid instructions, without
getting an exception.

Restore the freed initmem properly, by setting both the kernel image
map to the correct permissions.

[1] Documentation/riscv/vm-layout.rst

Fixes: e5c35fa04019 ("riscv: Map the kernel with correct permissions the first time")
Signed-off-by: Björn Töpel <bjorn@rivosinc.com>
---
v2: * Do not set the kernel image map to PAGE_WRITE. (Alex)
    * Massaged the commit message a bit.
    
Samuel, I removed your Reviewed-by:/Tested-by: for the v2.
---
 arch/riscv/kernel/setup.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)


base-commit: 22dce2b89d6043d5c3f68384285fff5506109317

Comments

Alexandre Ghiti Nov. 15, 2022, 2:53 p.m. UTC | #1
Hi Björn,

On 15/11/2022 10:06, Björn Töpel wrote:
> From: Björn Töpel <bjorn@rivosinc.com>
>
> 64-bit RISC-V kernels have the kernel image mapped separately to alias
> the linear map. The linear map and the kernel image map are documented
> as "direct mapping" and "kernel" respectively in [1].
>
> At image load time, the linear map corresponding to the kernel image
> is set to PAGE_READ permission, and the kernel image map is set to
> PAGE_READ|PAGE_EXEC.
>
> When the initmem is freed, the pages in the linear map should be
> restored to PAGE_READ|PAGE_WRITE, whereas the corresponding pages in
> the kernel image map should be restored to PAGE_READ, by removing the
> PAGE_EXEC permission.
>
> This is not the case. For 64-bit kernels, only the linear map is
> restored to its proper page permissions at initmem free, and not the
> kernel image map.
>
> In practise this results in that the kernel can potentially jump to
> dead __init code, and start executing invalid instructions, without
> getting an exception.
>
> Restore the freed initmem properly, by setting both the kernel image
> map to the correct permissions.
>
> [1] Documentation/riscv/vm-layout.rst
>
> Fixes: e5c35fa04019 ("riscv: Map the kernel with correct permissions the first time")
> Signed-off-by: Björn Töpel <bjorn@rivosinc.com>
> ---
> v2: * Do not set the kernel image map to PAGE_WRITE. (Alex)
>      * Massaged the commit message a bit.
>      
> Samuel, I removed your Reviewed-by:/Tested-by: for the v2.
> ---
>   arch/riscv/kernel/setup.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> index 67ec1fadcfe2..86acd690d529 100644
> --- a/arch/riscv/kernel/setup.c
> +++ b/arch/riscv/kernel/setup.c
> @@ -322,10 +322,11 @@ subsys_initcall(topology_init);
>   
>   void free_initmem(void)
>   {
> -	if (IS_ENABLED(CONFIG_STRICT_KERNEL_RWX))
> -		set_kernel_memory(lm_alias(__init_begin), lm_alias(__init_end),
> -				  IS_ENABLED(CONFIG_64BIT) ?
> -					set_memory_rw : set_memory_rw_nx);
> +	if (IS_ENABLED(CONFIG_STRICT_KERNEL_RWX)) {
> +		set_kernel_memory(lm_alias(__init_begin), lm_alias(__init_end), set_memory_rw_nx);
> +		if (IS_ENABLED(CONFIG_64BIT))
> +			set_kernel_memory(__init_begin, __init_end, set_memory_nx);
> +	}
>   
>   	free_initmem_default(POISON_FREE_INITMEM);
>   }


This looks good to me, I tested it on both defconfig and rv32_defconfig 
on qemu, so you can add:

Reviewed-by: Alexandre Ghiti <alex@ghiti.fr>
Tested-by: Alexandre Ghiti <alex@ghiti.fr>

Thanks,

Alex


> base-commit: 22dce2b89d6043d5c3f68384285fff5506109317
patchwork-bot+linux-riscv@kernel.org Nov. 30, 2022, 5:50 a.m. UTC | #2
Hello:

This patch was applied to riscv/linux.git (fixes)
by Palmer Dabbelt <palmer@rivosinc.com>:

On Tue, 15 Nov 2022 10:06:40 +0100 you wrote:
> From: Björn Töpel <bjorn@rivosinc.com>
> 
> 64-bit RISC-V kernels have the kernel image mapped separately to alias
> the linear map. The linear map and the kernel image map are documented
> as "direct mapping" and "kernel" respectively in [1].
> 
> At image load time, the linear map corresponding to the kernel image
> is set to PAGE_READ permission, and the kernel image map is set to
> PAGE_READ|PAGE_EXEC.
> 
> [...]

Here is the summary with links:
  - [v2] riscv: mm: Proper page permissions after initmem free
    https://git.kernel.org/riscv/c/6fdd5d2f8c2f

You are awesome, thank you!
diff mbox series

Patch

diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
index 67ec1fadcfe2..86acd690d529 100644
--- a/arch/riscv/kernel/setup.c
+++ b/arch/riscv/kernel/setup.c
@@ -322,10 +322,11 @@  subsys_initcall(topology_init);
 
 void free_initmem(void)
 {
-	if (IS_ENABLED(CONFIG_STRICT_KERNEL_RWX))
-		set_kernel_memory(lm_alias(__init_begin), lm_alias(__init_end),
-				  IS_ENABLED(CONFIG_64BIT) ?
-					set_memory_rw : set_memory_rw_nx);
+	if (IS_ENABLED(CONFIG_STRICT_KERNEL_RWX)) {
+		set_kernel_memory(lm_alias(__init_begin), lm_alias(__init_end), set_memory_rw_nx);
+		if (IS_ENABLED(CONFIG_64BIT))
+			set_kernel_memory(__init_begin, __init_end, set_memory_nx);
+	}
 
 	free_initmem_default(POISON_FREE_INITMEM);
 }