diff mbox series

riscv: mm: Proper page permissions after initmem free

Message ID 20221112113543.3165646-1-bjorn@kernel.org (mailing list archive)
State Superseded
Delegated to: Palmer Dabbelt
Headers show
Series riscv: mm: Proper page permissions after initmem free | expand

Checks

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

Commit Message

Björn Töpel Nov. 12, 2022, 11:35 a.m. UTC 
From: Björn Töpel <bjorn@rivosinc.com>

64-bit RISC-V kernels have the kernel image mapped separately, and in
addition to the linear map. When the kernel is loaded, the linear map
of kernel image is set to PAGE_READ permission, and the kernel map is
set to PAGE_READ and PAGE_EXEC.

When the initmem is freed, the corresponding pages in the linear map
should be restored to PAGE_READ and PAGE_WRITE. The corresponding
pages in the kernel map should also be restored to PAGE_READ and
PAGE_WRITE, by removing the PAGE_EXEC permission, and adding
PAGE_WRITE.

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
kernelmap.

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

Restore the freed initmem properly, by setting both the alias (kernel
map) and the linear map to the correct permissions.

Fixes: e5c35fa04019 ("riscv: Map the kernel with correct permissions the first time")
Signed-off-by: Björn Töpel <bjorn@rivosinc.com>
---
 arch/riscv/kernel/setup.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)


base-commit: 442bcbfd2c5401587b983e34bed0b407214735c3

Comments

Samuel Holland Nov. 13, 2022, 9:21 p.m. UTC | #1 
On 11/12/22 05:35, Björn Töpel wrote:
> From: Björn Töpel <bjorn@rivosinc.com>
> 
> 64-bit RISC-V kernels have the kernel image mapped separately, and in
> addition to the linear map. When the kernel is loaded, the linear map
> of kernel image is set to PAGE_READ permission, and the kernel map is
> set to PAGE_READ and PAGE_EXEC.
> 
> When the initmem is freed, the corresponding pages in the linear map
> should be restored to PAGE_READ and PAGE_WRITE. The corresponding
> pages in the kernel map should also be restored to PAGE_READ and
> PAGE_WRITE, by removing the PAGE_EXEC permission, and adding
> PAGE_WRITE.
> 
> 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
> kernelmap.
> 
> In practise this results in that the kernel can potentially jump to
> dead __init code, and start executing invalid 0xcc instructions,
> without getting an exception.
> 
> Restore the freed initmem properly, by setting both the alias (kernel
> map) and the linear map to the correct permissions.
> 
> Fixes: e5c35fa04019 ("riscv: Map the kernel with correct permissions the first time")
> Signed-off-by: Björn Töpel <bjorn@rivosinc.com>
> ---
>  arch/riscv/kernel/setup.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)

Reviewed-by: Samuel Holland <samuel@sholland.org>
Tested-by: Samuel Holland <samuel@sholland.org> # on D1
Alexandre Ghiti Nov. 14, 2022, 2:37 p.m. UTC | #2 
Hi Björn,

On 12/11/2022 12:35, Björn Töpel wrote:
> From: Björn Töpel <bjorn@rivosinc.com>
>
> 64-bit RISC-V kernels have the kernel image mapped separately, and in
> addition to the linear map. When the kernel is loaded, the linear map
> of kernel image is set to PAGE_READ permission, and the kernel map is
> set to PAGE_READ and PAGE_EXEC.
>
> When the initmem is freed, the corresponding pages in the linear map
> should be restored to PAGE_READ and PAGE_WRITE. The corresponding
> pages in the kernel map should also be restored to PAGE_READ and
> PAGE_WRITE, by removing the PAGE_EXEC permission, and adding
> PAGE_WRITE.
>
> 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
> kernelmap.
>
> In practise this results in that the kernel can potentially jump to
> dead __init code, and start executing invalid 0xcc instructions,
> without getting an exception.
>
> Restore the freed initmem properly, by setting both the alias (kernel
> map) and the linear map to the correct permissions.
>
> Fixes: e5c35fa04019 ("riscv: Map the kernel with correct permissions the first time")
> Signed-off-by: Björn Töpel <bjorn@rivosinc.com>
> ---
>   arch/riscv/kernel/setup.c | 10 ++++++----
>   1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> index ad76bb59b059..361e635070fe 100644
> --- a/arch/riscv/kernel/setup.c
> +++ b/arch/riscv/kernel/setup.c
> @@ -321,10 +321,12 @@ 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)) {
> +		if (IS_ENABLED(CONFIG_64BIT))
> +			set_kernel_memory(lm_alias(__init_begin), lm_alias(__init_end),
> +					  set_memory_rw);
> +		set_kernel_memory(__init_begin, __init_end, set_memory_rw_nx);


That's nits but for 64-bit kernels, do we really want to make the kernel 
mapping writable? I think catching a write access here would be better 
because IMO that should not happen.

Thanks,

Alex


> +	}
>   
>   	free_initmem_default(POISON_FREE_INITMEM);
>   }
>
> base-commit: 442bcbfd2c5401587b983e34bed0b407214735c3
Björn Töpel Nov. 14, 2022, 3:25 p.m. UTC | #3 
Alexandre Ghiti <alex@ghiti.fr> writes:

>> +		set_kernel_memory(__init_begin, __init_end, set_memory_rw_nx);
>
>
> That's nits but for 64-bit kernels, do we really want to make the kernel 
> mapping writable? I think catching a write access here would be better 
> because IMO that should not happen.

Thanks for having a look!

Hmm, so what you're suggesting is a "set_memory_r_nx" for CONFIG_64BIT?
What do other archs do? Is it worth the extra ifdef clutter?


Björn
diff mbox series

Patch

diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
index ad76bb59b059..361e635070fe 100644
--- a/arch/riscv/kernel/setup.c
+++ b/arch/riscv/kernel/setup.c
@@ -321,10 +321,12 @@  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)) {
+		if (IS_ENABLED(CONFIG_64BIT))
+			set_kernel_memory(lm_alias(__init_begin), lm_alias(__init_end),
+					  set_memory_rw);
+		set_kernel_memory(__init_begin, __init_end, set_memory_rw_nx);
+	}
 
 	free_initmem_default(POISON_FREE_INITMEM);
 }