On Tue, May 18, 2021 at 08:43:39PM +0200, Christophe Leroy wrote:
>
>
> Le 06/05/2020 à 05:40, Jordan Niethe a écrit :
> > Do not allow inserting breakpoints on the suffix of a prefix instruction
> > in kprobes.
> >
> > Signed-off-by: Jordan Niethe <jniet...@gmail.com>
> > ---
> > v8: Add this back from v3
> > ---
> > arch/powerpc/kernel/kprobes.c | 13 +++++++++++++
> > 1 file changed, 13 insertions(+)
> >
> > diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
> > index 33d54b091c70..227510df8c55 100644
> > --- a/arch/powerpc/kernel/kprobes.c
> > +++ b/arch/powerpc/kernel/kprobes.c
> > @@ -106,7 +106,9 @@ kprobe_opcode_t *kprobe_lookup_name(const char *name,
> > unsigned int offset)
> > int arch_prepare_kprobe(struct kprobe *p)
> > {
> > int ret = 0;
> > + struct kprobe *prev;
> > struct ppc_inst insn = ppc_inst_read((struct ppc_inst *)p->addr);
> > + struct ppc_inst prefix = ppc_inst_read((struct ppc_inst *)(p->addr -
> > 1));
>
> What if p->addr is the first word of a page and the previous page is not
> mapped ?
IIRC prefixed instructions can't straddle 64 byte boundaries (or was it
128 bytes?), much less page boundaries.
>
> > if ((unsigned long)p->addr & 0x03) {
> > printk("Attempt to register kprobe at an unaligned address\n");
> > @@ -114,6 +116,17 @@ int arch_prepare_kprobe(struct kprobe *p)
> > } else if (IS_MTMSRD(insn) || IS_RFID(insn) || IS_RFI(insn)) {
> > printk("Cannot register a kprobe on rfi/rfid or mtmsr[d]\n");
> > ret = -EINVAL;
> > + } else if (ppc_inst_prefixed(prefix)) {
>
> If p->addr - 2 contains a valid prefixed instruction, then p->addr - 1
> contains the suffix of that prefixed instruction. Are we sure a suffix can
> never ever be misinterpreted as the prefix of a prefixed instruction ?
>
Prefixes are easy to decode, the 6 MSB are 0b000001 (from memory).
After some digging on the 'net: "All prefixes have the major opcode 1. A
prefix will never be a valid word instruction. A suffix may be an
existing word instruction or a new instruction."
IOW, detecting prefixes is trivial. It's not x86...
Gabriel
>
> > + printk("Cannot register a kprobe on the second word of prefixed
> > instruction\n");
> > + ret = -EINVAL;
> > + }
> > + preempt_disable();
> > + prev = get_kprobe(p->addr - 1);
> > + preempt_enable_no_resched();
> > + if (prev &&
> > + ppc_inst_prefixed(ppc_inst_read((struct ppc_inst
> > *)prev->ainsn.insn))) {
> > + printk("Cannot register a kprobe on the second word of prefixed
> > instruction\n");
> > + ret = -EINVAL;
> > }
> > /* insn must be on a special executable page on ppc64. This is
> >
