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

[Thibault Ferrante]

On 13-09-2024 17:04, Koichiro Den wrote:
> 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)--
I personally wouldn't go into as much details but that would work.

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



--
Thibault