NACK/Cmt: [SRU][J][PATCH 1/1] riscv: prevent pt_regs corruption for secondary idle threads

[Thibault Ferrante]

On 12-09-2024 08:48, Koichiro Den wrote:
> From: Sergey Matyukevich <sergey.matyukevich at syntacore.com>
> 
> Top of the kernel thread stack should be reserved for pt_regs. However
> this is not the case for the idle threads of the secondary boot harts.
> Their stacks overlap with their pt_regs, so both may get corrupted.
> 
> Similar issue has been fixed for the primary hart, see c7cdd96eca28
> ("riscv: prevent stack corruption by reserving task_pt_regs(p) early").
> However that fix was not propagated to the secondary harts. The problem
> has been noticed in some CPU hotplug tests with V enabled. The function
> smp_callin stored several registers on stack, corrupting top of pt_regs
> structure including status field. As a result, kernel attempted to save
> or restore inexistent V context.
> 
> Fixes: 9a2451f18663 ("RISC-V: Avoid using per cpu array for ordered booting")
> Fixes: 2875fe056156 ("RISC-V: Add cpu_ops and modify default booting method")
> Signed-off-by: Sergey Matyukevich <sergey.matyukevich at syntacore.com>
> Reviewed-by: Alexandre Ghiti <alexghiti at rivosinc.com>
> Link: https://lore.kernel.org/r/20240523084327.2013211-1-geomatsi@gmail.com
> Signed-off-by: Palmer Dabbelt <palmer at rivosinc.com>
> (backported from commit a638b0461b58aa3205cd9d5f14d6f703d795b4af)
> [koichiroden: Adjusted context due to missing commits:
This is misleading, it's not just context that is adjusted, you dropped patch modifications.
You should explicitly mention that you dropped modification of the original patch targeting arch/riscv/kernel/cpu_ops_sbi.c
due to missing commits.
>   commit c78f94f35cf6 ("RISC-V: Use __cpu_up_stack/task_pointer only for spinwait method")
>   commit 9a2451f18663 ("RISC-V: Avoid using per cpu array for ordered booting")]
> CVE-2024-38667
> Signed-off-by: Koichiro Den <koichiro.den at canonical.com>
> ---
>   arch/riscv/kernel/cpu_ops.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/arch/riscv/kernel/cpu_ops.c b/arch/riscv/kernel/cpu_ops.c
> index 1985884fe829..c54a98960373 100644
> --- a/arch/riscv/kernel/cpu_ops.c
> +++ b/arch/riscv/kernel/cpu_ops.c
> @@ -28,8 +28,7 @@ void cpu_update_secondary_bootdata(unsigned int cpuid,
>   
>   	/* Make sure tidle is updated */
>   	smp_mb();
> -	WRITE_ONCE(__cpu_up_stack_pointer[hartid],
> -		   task_stack_page(tidle) + THREAD_SIZE);
> +	WRITE_ONCE(__cpu_up_stack_pointer[hartid], task_pt_regs(tidle));
>   	WRITE_ONCE(__cpu_up_task_pointer[hartid], tidle);
>   }
>   


-- 
--
Thibault