Re: [PATCH] riscv: kprobe: Fixup kernel panic when probing an illegal position

["liaochang (A)" <liaochang1@huawei.com>]

在 2023/1/28 13:22, Guo Ren 写道:
> On Sat, Jan 28, 2023 at 10:55 AM liaochang (A) <liaochang1@huawei.com> wrote:
>>
>>
>>
>> 在 2023/1/26 21:05, guoren@kernel.org 写道:
>>> From: Guo Ren <guoren@linux.alibaba.com>
>>>
>>> The kernel would panic when probed for an illegal position. eg:
>>>
>>> (CONFIG_RISCV_ISA_C=n)
>>>
>>> echo 'p:hello kernel_clone+0x16 a0=%a0' >> kprobe_events
>>> echo 1 > events/kprobes/hello/enable
>>> cat trace
>>>
>>> Kernel panic - not syncing: stack-protector: Kernel stack
>>> is corrupted in: __do_sys_newfstatat+0xb8/0xb8
>>> CPU: 0 PID: 111 Comm: sh Not tainted
>>> 6.2.0-rc1-00027-g2d398fe49a4d #490
>>> Hardware name: riscv-virtio,qemu (DT)
>>> Call Trace:
>>> [<ffffffff80007268>] dump_backtrace+0x38/0x48
>>> [<ffffffff80c5e83c>] show_stack+0x50/0x68
>>> [<ffffffff80c6da28>] dump_stack_lvl+0x60/0x84
>>> [<ffffffff80c6da6c>] dump_stack+0x20/0x30
>>> [<ffffffff80c5ecf4>] panic+0x160/0x374
>>> [<ffffffff80c6db94>] generic_handle_arch_irq+0x0/0xa8
>>> [<ffffffff802deeb0>] sys_newstat+0x0/0x30
>>> [<ffffffff800158c0>] sys_clone+0x20/0x30
>>> [<ffffffff800039e8>] ret_from_syscall+0x0/0x4
>>> ---[ end Kernel panic - not syncing: stack-protector:
>>> Kernel stack is corrupted in: __do_sys_newfstatat+0xb8/0xb8 ]---
>>>
>>> That is because the kprobe's ebreak instruction broke the kernel's
>>> original code. The user should guarantee the correction of the probe
>>> position, but it couldn't make the kernel panic.
>>>
>>> This patch adds arch_check_kprobe in arch_prepare_kprobe to prevent an
>>> illegal position (Such as the middle of an instruction).
>>>
>>> Fixes: c22b0bcb1dd0 ("riscv: Add kprobes supported")
>>> Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
>>> Signed-off-by: Guo Ren <guoren@kernel.org>
>>> ---
>>>  arch/riscv/kernel/probes/kprobes.c | 18 ++++++++++++++++++
>>>  1 file changed, 18 insertions(+)
>>>
>>> diff --git a/arch/riscv/kernel/probes/kprobes.c b/arch/riscv/kernel/probes/kprobes.c
>>> index f21592d20306..475989f06d6d 100644
>>> --- a/arch/riscv/kernel/probes/kprobes.c
>>> +++ b/arch/riscv/kernel/probes/kprobes.c
>>> @@ -48,6 +48,21 @@ static void __kprobes arch_simulate_insn(struct kprobe *p, struct pt_regs *regs)
>>>       post_kprobe_handler(p, kcb, regs);
>>>  }
>>>
>>> +static bool __kprobes arch_check_kprobe(struct kprobe *p)
>>> +{
>>> +     unsigned long tmp  = (unsigned long)p->addr - p->offset;
>>> +     unsigned long addr = (unsigned long)p->addr;
>>> +
>>> +     while (tmp <= addr) {
>>> +             if (tmp == addr)
>>> +                     return true;
>>> +
>>> +             tmp += GET_INSN_LENGTH(*(kprobe_opcode_t *)tmp);
>>> +     }
>>> +
>>> +     return false;
>>> +}
>>
>> LGTM.
>>
>> I have submit a patch to fix the same problem, found at:
>>
>> https://lore.kernel.org/lkml/20230127130541.1250865-11-chenguokai17@mails.ucas.ac.cn/
> I have a question on your OPTKPROBE patch:
> https://lore.kernel.org/lkml/20230127130541.1250865-10-chenguokai17@mails.ucas.ac.cn/
> 
> @@ -490,7 +573,14 @@ int arch_prepare_optimized_kprobe(struct
> optimized_kprobe *op,
>   * to detour buffer, ra is used to form JR jumping back from detour
>   * buffer.
>   */
> - find_free_registers(orig, op, &rd, &ra);
> + if (used_reg == ALL_REG_OCCUPIED) {
> + find_free_registers(orig, op, &rd, &ra);
> + } else {
> + /* Choose one unused caller-saved register. */
> + rd = ffz(used_reg);
> + ra = rd;
> + }
> +
>   if (rd == 0 || ra == 0) {
>   ^^^^^^^^^^^^^^^^^^
> After no opt_used_dst_reg & no free caller-saved register (Of cause,
> it's very rare for no available tmp regs):

Thanks for reviewing.

If this case occurs, it means all 32 integer registers are used as the source in opcode,
I have no deep knowledge about the code generation and register allocation of riscv compiler,
but guess it is not possible to generate such style of instructions sequence for one really
meaningful kernel function.

> 
> Why not try:
> 
>      0: REG_S  ra, -SZREG(sp)
>      4: auipc  ra, ?
>      8: jalr   ?(ra)
>     12: REG_L  ra, -SZREG(sp)
> 
> Besides taking up more instruction slots, does it have other problems?

I think it okay in some term, two points need to pay attention:
1. It needs to update the 'ra' entry in stack if ra is modified in detour buffer.
2. If sp is modified in detour buffer, #12 might return a illegal value, it is a common
   case when set kprobe at function prologue and epilogue.

> 
>>
>> So this boundary check is necessary no matter CONFIG_RISCV_ISA_C is enable or not, right?
>>
>>
>>> +
>>>  int __kprobes arch_prepare_kprobe(struct kprobe *p)
>>>  {
>>>       unsigned long probe_addr = (unsigned long)p->addr;
>>> @@ -55,6 +70,9 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
>>>       if (probe_addr & 0x1)
>>>               return -EILSEQ;
>>>
>>> +     if (!arch_check_kprobe(p))
>>> +             return -EILSEQ;
>>> +
>>>       /* copy instruction */
>>>       p->opcode = *p->addr;
>>>
>>
>> --
>> BR,
>> Liao, Chang
> 
> 
> 

-- 
BR,
Liao, Chang