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

[Koichiro Den]

On Fri, Sep 13, 2024 at 02:17:50PM +0200, Thibault Ferrante wrote:
> 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.

Thanks for review. How's this?

--(snip)--
(backported from commit a638b0461b58aa3205cd9d5f14d6f703d795b4af)
[koichiroden: Sparse HART id support added many changes on upstream:
 (https://lore.kernel.org/all/20220120090918.2646626-1-atishp@rivosinc.com/)
 and the primary fix commmit a638b0461b58 depends on them. Directly
 conflicting commits from the series are as follows:
 9a2451f18663 ("RISC-V: Avoid using per cpu array for ordered booting")
 c78f94f35cf6 ("RISC-V: Use __cpu_up_stack/task_pointer only for
 spinwait method").
 We opted not to backport the entire series, minimizing changes around
 the primary security fix. This indicates that the fix is needed only
 for __cpu_up_stack_pointer, which still serves dual purposes for both
 spinwait and ordered methods, without supporting Sparse HART id.]
--(snip)--

-Koichiro Den

> >   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
> 
> -- 
> kernel-team mailing list
> kernel-team at lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team