linux-kernel - Re: Questions about kprobe handler

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: <1669288280.gcxbuppl5k.naveen@linux.ibm.com>
Date:   Thu, 24 Nov 2022 17:14:27 +0530
From:   "Naveen N. Rao" <naveen.n.rao@...ux.ibm.com>
To:     Jinyang He <hejinyang@...ngson.cn>,
        "Masami Hiramatsu (Google)" <mhiramat@...nel.org>,
        Tiezhu Yang <yangtiezhu@...ngson.cn>
Cc:     Anil S Keshavamurthy <anil.s.keshavamurthy@...el.com>,
        Huacai Chen <chenhuacai@...ngson.cn>,
        "David S. Miller" <davem@...emloft.net>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        linuxppc-dev@...ts.ozlabs.org
Subject: Re: Questions about kprobe handler

Jinyang He wrote:
> 在 2022/11/17 21:09, Masami Hiramatsu (Google) 写道:
> 
>> On Thu, 17 Nov 2022 09:07:37 +0800
>> Tiezhu Yang <yangtiezhu@...ngson.cn> wrote:
>>
>>> Hi KPROBES maintainers,
>>>
>>> There are some differences of kprobe handler implementations on various
>>> archs, the implementations are almost same on arm64, riscv, csky, the
>>> code logic is clear and understandable. But on mips and loongarch (not
>>> upstreamed yet), if get_kprobe() returns NULL, what is the purpose of
>>> the check "if (addr->word != breakpoint_insn.word)", is it necessary?
>>> Can we just return directly? Please take a look, thank you.

Are you seeing any problem with that?

>> Good question!
>>
>> This means that when the software breakpoint was hit on that CPU, but
>> before calling kprobe handler function, the other CPU can remove that
>> kprobe from hash table, becahse the hash table is not locked.
>> In that case, the get_kprobe(addr) will return NULL, and the software
>> breakpoint instruction is already removed (replaced with the original
>> instruction). Thus it is safe to go back. But this is originally
>> implemented for x86, which doesn't need stop_machine() to modify the
>> code. On the other hand, if an architecture which needs stop_machine()
>> to modify code, the above scenario never happen. In that case, you
>> don't need this "if" case.

This has been a problematic area on powerpc. See, for instance, commits:
- 9ed5df69b79a22 ("powerpc/kprobes: Use probe_address() to read instructions")
- 21f8b2fa3ca5b0 ("powerpc/kprobes: Ignore traps that happened in real mode")

I think we should close this race, perhaps by instroducing another 
synchronize_rcu() in unregister_kprobe(), allowing architectures using 
stop_machine() to override that. That would be cleaner.


>>
>> Thank you,
>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/mips/kernel/kprobes.c#n323
>>> 		p = get_kprobe(addr);
>>> 		if (p) {
>>> 			...
>>> 		} else if (addr->word != breakpoint_insn.word) {
> 
> Hi,
> 
> 
> Sorry for the late reply, but I think there should be some public
> comments so that I can get the authoritative response, as offline
> communication with Tiezhu is always one-sided.
> 
> I think the branch you answered here is " if (p)... " rather than
> "else if (addr->word != breakpoint_insn.word)". It is right if we
> not use stop_machine here we need this branch. In fact, Tiezhu
> and Huacai, the maintainer of LoongArch are more concerned
> about the latter why we need compare with the breakpoint_insn.

Masami answered why we need the else part comparing the instruction at 
addr with the breakpoint instruction. It is to check if the breakpoint 
instruction has been removed since we hit it, but before the processor 
reached the kprobe handler.

> 
> The reason I gave as follows, and I show mips code here,
> 
>      p = get_kprobe(addr);
>      if (!p) {
>          if (addr->word != breakpoint_insn.word) {
>              /*
>               * The breakpoint instruction was removed right
>               * after we hit it.  Another cpu has removed
>               * either a probepoint or a debugger breakpoint
>               * at this address.  In either case, no further
>               * handling of this interrupt is appropriate.
>               */
>              ret = 1;
>          }
>          /* Not one of ours: let kernel handle it */
>          goto no_kprobe;
>      }
> 
> ...
> int kprobe_exceptions_notify(struct notifier_block *self,
>                         unsigned long val, void *data)
> {
>      struct die_args *args = (struct die_args *)data;
>      int ret = NOTIFY_DONE;
> 
>      switch (val) {
>      case DIE_BREAK:
>          if (kprobe_handler(args->regs))
>              ret = NOTIFY_STOP;
>          break;
> ...
> 
> The !p means this insn has been moved, and then in most cases the COND

!p means the kprobe has been removed - there may or may not be another 
breakpoint instruction at that address.

> "addr->word != breakpoint_insn.word" is true, we return 1 so that the return
> value in kprobe_exceptions_notify will be changed to NOTIFY_STOP.

We do this since we now know that there is no breakpoint at this 
address, so we should not continue.

> The mips use soft breakpoint like "break hint". How if the original insn
> is same as breakpoint_insn? That is a few case when COND is false. In that
> case, it means we should handle it by other handlers and doesn't change 
> ret to
> keep NOTIFY_DONE as kprobe_exceptions_notify return value.

If there is a breakpoint instruction at that address, we should let the 
kernel continue processing the breakpoint.

> 
> Is this idea reasonable? Thanks!

As another data point, you may want to look at kprobe_handler() in 
arch/powerpc/kernel/kprobes.c . We also handle cases where there is a 
different type of breakpoint instruction.


- Naveen