| Message ID | 20200506034050.24806-28-jniethe5@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | b4657f7650babc9bfb41ce875abe41b18604a105 |
| Headers | show |
| Series | Initial Prefixed Instruction support | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (1bc92fe3175eb26ff37e580c0383d7a9abe06835) |
| snowpatch_ozlabs/checkpatch | warning | total: 0 errors, 2 warnings, 0 checks, 26 lines checked |
| snowpatch_ozlabs/needsstable | success | Patch has no Fixes tags |
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 <jniethe5@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 ? > > 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 ? > + 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 >
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 <jniethe5@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 > >
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 <jniethe5@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 ? Good catch! I think we can just skip validation if the instruction is at the beginning of a page. I have a few cleanups in this area - I will post a patchset soon. > >> >> 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 ? Yes. Per the ISA: Bits 0:5 of all prefixes are assigned the primary opcode value 0b000001. 0b000001 is not available for use as a primary opcode for either word instructions or suffixes of prefixed instructions. - Naveen
On Wed, May 19, 2021 at 6:11 PM Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> wrote: > > 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 <jniethe5@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 ? > > Good catch! > I think we can just skip validation if the instruction is at the > beginning of a page. I have a few cleanups in this area - I will post a > patchset soon. Yeah thanks Christophe for noticing that. And thanks Naveen that sounds like it should fix it. > > > > >> > >> 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 ? > > Yes. Per the ISA: > Bits 0:5 of all prefixes are assigned the primary opcode > value 0b000001. 0b000001 is not available for use as a > primary opcode for either word instructions or suffixes > of prefixed instructions. Yep, a prefix will never be a valid word instruction or suffix. > > > - Naveen >
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)); 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)) { + 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
Do not allow inserting breakpoints on the suffix of a prefix instruction in kprobes. Signed-off-by: Jordan Niethe <jniethe5@gmail.com> --- v8: Add this back from v3 --- arch/powerpc/kernel/kprobes.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)