linux-kernel - Re: [PATCH] riscv: kprobe: Optimize kprobe with accurate atomicity

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: <CAJF2gTSHQ7J1a-0tTzaLAHeUTqbco5OXJgGDZpgB-SLb+teL2A@mail.gmail.com>
Date:   Tue, 31 Jan 2023 09:09:59 +0800
From:   Guo Ren <guoren@...nel.org>
To:     Björn Töpel <bjorn@...nel.org>
Cc:     "liaochang (A)" <liaochang1@...wei.com>, palmer@...belt.com,
        paul.walmsley@...ive.com, mhiramat@...nel.org,
        conor.dooley@...rochip.com, penberg@...nel.org,
        mark.rutland@....com, linux-riscv@...ts.infradead.org,
        linux-kernel@...r.kernel.org, Guo Ren <guoren@...ux.alibaba.com>
Subject: Re: [PATCH] riscv: kprobe: Optimize kprobe with accurate atomicity

On Tue, Jan 31, 2023 at 9:01 AM Guo Ren <guoren@...nel.org> wrote:
>
> On Mon, Jan 30, 2023 at 11:28 PM Björn Töpel <bjorn@...nel.org> wrote:
> >
> > Guo Ren <guoren@...nel.org> writes:
> >
> > >> In the serie of RISCV OPTPROBES [1], it patches a long-jump instructions pair
> > >> AUIPC/JALR in kernel text, so in order to ensure other CPUs does not execute
> > >> in the instructions that will be modified, it is still need to stop other CPUs
> > >> via patch_text API, or you have any better solution to achieve the purpose?
> > >  - The stop_machine is an expensive way all architectures should
> > > avoid, and you could keep that in your OPTPROBES implementation files
> > > with static functions.
> > >  - The stop_machine couldn't work with PREEMPTION, so your
> > > implementation needs to work with !PREEMPTION.
> >
> > ...and stop_machine() with !PREEMPTION is broken as well, when you're
> > replacing multiple instructions (see Mark's post at [1]). The
> > stop_machine() dance might work when you're replacing *one* instruction,
> > not multiple as in the RISC-V case. I'll expand on this in a comment in
> > the OPTPROBES v6 series.
> >
> > >> >  static void __kprobes arch_prepare_simulate(struct kprobe *p)
> > >> > @@ -114,16 +120,23 @@ void *alloc_insn_page(void)
> > >> >  /* install breakpoint in text */
> > >> >  void __kprobes arch_arm_kprobe(struct kprobe *p)
> > >> >  {
> > >> > -     if ((p->opcode & __INSN_LENGTH_MASK) == __INSN_LENGTH_32)
> > >> > -             patch_text(p->addr, __BUG_INSN_32);
> > >> > -     else
> > >> > -             patch_text(p->addr, __BUG_INSN_16);
> > >> > +#ifdef CONFIG_RISCV_ISA_C
> > >> > +     u32 opcode = __BUG_INSN_16;
> > >> > +#else
> > >> > +     u32 opcode = __BUG_INSN_32;
> > >> > +#endif
> > >> > +     patch_text_nosync(p->addr, &opcode, GET_INSN_LENGTH(opcode));
> > >>
> > >> Sounds good, but it will leave some RVI instruction truncated in kernel text,
> > >> i doubt kernel behavior depends on the rest of the truncated instruction, well,
> > >> it needs more strict testing to prove my concern :)
> > > I do this on purpose, and it doesn't cause any problems. Don't worry;
> > > IFU hw must enforce the fetch sequence, and there is no way to execute
> > > broken instructions even in the speculative execution path.
> >
> > This is stretching reality a bit much. ARMv8, e.g., has a chapter in the
> > Arm ARM [2] Appendix B "Concurrent modification and execution of
> > instructions" (CMODX). *Some* instructions can be replaced concurrently,
> > and others cannot without caution. Assuming that that all RISC-V
> > implementations can, is a stretch. RISC-V hasn't even specified the
> > behavior of CMODX (which is problematic).
> Here we only use one sw/sh instruction to store a 32bit/16bit aligned element:
>
> INSN_0 <- ebreak (16bit/32bit aligned)
> INSN_1
> INSN_2
>
> The ebreak would cause an exception which implies a huge fence here.
> No machine could give a speculative execution for the ebreak path.

For ARMv7, ebreak is also safe:

---
Concurrent modification and execution of instructions

The ARMv7 architecture limits the set of instructions that can be
executed by one thread of execution as they are being modified by
another thread of execution without requiring explicit
synchronization.
...
The instructions to which this guarantee applies are:
In the Thumb instruction set
The 16-bit encodings of the B, NOP, BKPT, and SVC instructions.
...
In the ARM instruction set
The B, BL, NOP, BKPT, SVC, HVC, and SMC instructions.
---

>
> >
> > If anything it would be more likely that the existing
> > "stop_machine()-to-replace-with-ebreak" works (again, replacing one
> > instruction does not have the !PREEMPTION issues). Then again, no spec,
> > so mostly guessing from my side. :-(
> >
> > Oh, but the existing "ebreak replace" might be broken like [3].
> >
> >
> > Björn
> >
> >
> > [1] https://lore.kernel.org/linux-riscv/Y7%2F6AtX5X0+5qF6Y@FVFF77S0Q05N/
> > [2] https://developer.arm.com/documentation/ddi0487/latest
> > [3] https://lore.kernel.org/linux-riscv/20230126170607.1489141-2-guoren@kernel.org/
>
>
>
> --
> Best Regards
>  Guo Ren



-- 
Best Regards
 Guo Ren