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: <87cz6vtcce.fsf@all.your.base.are.belong.to.us>
Date:   Tue, 31 Jan 2023 08:03:29 +0100
From:   Björn Töpel <bjorn@...nel.org>
To:     Guo Ren <guoren@...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

Guo Ren <guoren@...nel.org> writes:

>> > >> >  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.
> ---

Right, and "B7.7 Concurrent modification and execution of instructions"
Armv8-M ARM (https://developer.arm.com/documentation/ddi0553/latest),
also defines that certain instructions can be concurrently modified.

This is beside the point. We don't have a spec for RISC-V, yet. We're
not even sure we can (in general) replace the lower 16b of an 32b
instruction concurrently. "It's in the Armv8-M spec" is not enough.

I'd love to have a spec defining that, and Derek et al has started
[1]. Slide #99 has CMODX details.

Your patch might be great for some HW (which?), but not enough for
general RISC-V Linux (yet). Until then, the existing stop_machine() way
is unfortunately the way to go.


Björn

[1] https://github.com/riscv/riscv-j-extension/blob/master/id-consistency-proposal.pdf