linux-kernel - Re: [PATCH] LoongArch: Enhance robust of kprobe

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f83d1048-93f6-6c11-2c2a-98c1e1ea7e9d@loongson.cn>
Date: Wed, 9 Apr 2025 10:17:13 +0800
From: Jinyang He <hejinyang@...ngson.cn>
To: Tiezhu Yang <yangtiezhu@...ngson.cn>, Huacai Chen <chenhuacai@...nel.org>
Cc: loongarch@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] LoongArch: Enhance robust of kprobe

On 2025-04-08 17:27, Tiezhu Yang wrote:

> Currently, interrupts need to be disabled before single-step mode is set,
> it requires that the CSR_PRMD_PIE must be cleared in save_local_irqflag()
> which is called by setup_singlestep(), this is reasonable.
>
> But in the first kprobe breakpoint exception, if the irq is enabled at the
> beginning of do_bp(), it will not be disabled at the end of do_bp() due to
> the CSR_PRMD_PIE has been cleared in save_local_irqflag(). For this case,
> it may corrupt exception context when restoring exception after do_bp() in
> handle_bp(), this is not reasonable.
>
> Based on the above analysis, in order to make sure the irq is disabled at
> the end of do_bp() for the first kprobe breakpoint exception, it is proper
> to disable irq first before clearing CSR_PRMD_PIE in save_local_irqflag().
>
> Fixes: 6d4cc40fb5f5 ("LoongArch: Add kprobes support")
> Co-developed-by: Tianyang Zhang <zhangtianyang@...ngson.cn>
> Signed-off-by: Tianyang Zhang <zhangtianyang@...ngson.cn>
> Signed-off-by: Tiezhu Yang <yangtiezhu@...ngson.cn>
> ---
>   arch/loongarch/kernel/kprobes.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/arch/loongarch/kernel/kprobes.c b/arch/loongarch/kernel/kprobes.c
> index 8ba391cfabb0..6eab97636e6b 100644
> --- a/arch/loongarch/kernel/kprobes.c
> +++ b/arch/loongarch/kernel/kprobes.c
> @@ -113,6 +113,7 @@ NOKPROBE_SYMBOL(set_current_kprobe);
>   static void save_local_irqflag(struct kprobe_ctlblk *kcb,
>   			       struct pt_regs *regs)
>   {
> +	local_irq_disable();
>   	kcb->saved_status = regs->csr_prmd;
>   	regs->csr_prmd &= ~CSR_PRMD_PIE;
>   }

Hi, Tiezhu,

I think the carsh is caused by "irq-triggered re-re-enter" clear
the previous_kprobe status. An example things like,

...
   static void setup_singlestep(struct kprobe *p, struct pt_regs *regs,
                    struct kprobe_ctlblk *kcb, int reenter)
   {
       union loongarch_instruction insn;

       if (reenter) {
           save_previous_kprobe(kcb);
  ===================   <- irq and trigger re-re-enter in its handler
           set_current_kprobe(p);
           kcb->kprobe_status = KPROBE_REENTER;
       } else {
           kcb->kprobe_status = KPROBE_HIT_SS;
       }
...

We should assure the previous_kprobe status not be changed after re-enter.
So this `local_irq_disable` should be set in reenter block begin.
And for !reenter block, `local_irq_disable` may be not needed.

Jinyang