Hi Huacai,
Thanks for your review.
On Wed, Jul 19 2023 at 11:17:14 PM +0800, Huacai Chen wrote:
> Hi, Enze,
>
> On Wed, Jul 19, 2023 at 4:34 PM Enze Li <lienze@xxxxxxxxxx> wrote:
>>
>> Currently, executing arch_stack_walk can only get the full stack
>> information including NMI. This is because the implementation
>> of arch_stack_walk is forced to ignore the information passed by the
>> regs parameter and use the current stack information instead.
>>
>> For some detection systems like KFENCE, only partial stack information
>> is needed. In particular, the stack frame where the interrupt occurred.
>>
>> To support KFENCE, this patch modifies the implementation of the
>> arch_stack_walk function so that if this function is called with the
>> regs argument passed, it retains all the stack information in regs and
>> uses it to provide accurate information.
>>
>> Before the patch applied, I get,
>> [ 1.531195 ] ==================================================================
>> [ 1.531442 ] BUG: KFENCE: out-of-bounds read in stack_trace_save_regs+0x48/0x6c
>> [ 1.531442 ]
>> [ 1.531900 ] Out-of-bounds read at 0xffff800012267fff (1B left of kfence-#12):
>> [ 1.532046 ] stack_trace_save_regs+0x48/0x6c
>> [ 1.532169 ] kfence_report_error+0xa4/0x528
>> [ 1.532276 ] kfence_handle_page_fault+0x124/0x270
>> [ 1.532388 ] no_context+0x50/0x94
>> [ 1.532453 ] do_page_fault+0x1a8/0x36c
>> [ 1.532524 ] tlb_do_page_fault_0+0x118/0x1b4
>> [ 1.532623 ] test_out_of_bounds_read+0xa0/0x1d8
>> [ 1.532745 ] kunit_generic_run_threadfn_adapter+0x1c/0x28
>> [ 1.532854 ] kthread+0x124/0x130
>> [ 1.532922 ] ret_from_kernel_thread+0xc/0xa4
>> <snip>
>>
>> With this patch applied, I get the correct stack information.
>> [ 1.320220 ] ==================================================================
>> [ 1.320401 ] BUG: KFENCE: out-of-bounds read in test_out_of_bounds_read+0xa8/0x1d8
>> [ 1.320401 ]
>> [ 1.320898 ] Out-of-bounds read at 0xffff800012257fff (1B left of kfence-#10):
>> [ 1.321134 ] test_out_of_bounds_read+0xa8/0x1d8
>> [ 1.321264 ] kunit_generic_run_threadfn_adapter+0x1c/0x28
>> [ 1.321392 ] kthread+0x124/0x130
>> [ 1.321459 ] ret_from_kernel_thread+0xc/0xa4
>> <snip>
>>
>> Signed-off-by: Enze Li <lienze@xxxxxxxxxx>
>> ---
>> arch/loongarch/kernel/stacktrace.c | 16 ++++++++++------
>> 1 file changed, 10 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/loongarch/kernel/stacktrace.c b/arch/loongarch/kernel/stacktrace.c
>> index 2463d2fea21f..21f60811e26f 100644
>> --- a/arch/loongarch/kernel/stacktrace.c
>> +++ b/arch/loongarch/kernel/stacktrace.c
>> @@ -18,16 +18,20 @@ void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie,
>> struct pt_regs dummyregs;
>> struct unwind_state state;
>>
>> - regs = &dummyregs;
>> -
>> if (task == current) {
>> - regs->regs[3] = (unsigned long)__builtin_frame_address(0);
>> - regs->csr_era = (unsigned long)__builtin_return_address(0);
>> + if (regs)
>> + memcpy(&dummyregs, regs, sizeof(*regs));
>> + else {
>> + dummyregs.regs[3] = (unsigned long)__builtin_frame_address(0);
>> + dummyregs.csr_era = (unsigned long)__builtin_return_address(0);
>> + }
>> } else {
> When "task != current", we don't need to handle the "regs != NULL" case?
>
> Huacai
>
So far, I have not encountered this situation. I'm not sure what
problems would arise from extending the modifications with "task !=
current".
However, these modifications now are sufficient for the KFENCE
system. I would suggest that we don't modify other parts until we
encounter problems. This way, we can forge ahead steadily.
Best Regards,
Enze
>> - regs->regs[3] = thread_saved_fp(task);
>> - regs->csr_era = thread_saved_ra(task);
>> + dummyregs.regs[3] = thread_saved_fp(task);
>> + dummyregs.csr_era = thread_saved_ra(task);
>> }
>>
>> + regs = &dummyregs;
>> +
>> regs->regs[1] = 0;
>> for (unwind_start(&state, task, regs);
>> !unwind_done(&state) && !unwind_error(&state); unwind_next_frame(&state)) {
>> --
>> 2.34.1
>>
>>
