| Message ID | 20241216130544.706110-1-ben.dooks@codethink.co.uk (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | riscv: kprobes: fix placement in arch_prepare_ss_slot | expand |
Hi Ben, On 16/12/2024 14:05, Ben Dooks wrote: > The second patch_text_nosync() in arch_prepare_ss_slot should be pointing > after the first instruction, however p->ainsn.api.insn is a pointer to a > probe_opcode_t which is 4 bytes... this means an instruction of length 2 > moves the pointer by 8 bytes. > > This means a kprobe may fail to work and recurse into the bad instruction > handler (which then itself fails as the original lock still seems to be > held?) > > Fixes: b1756750a397f36ddc857 ("riscv: kprobes: Use patch_text_nosync() for insn slots") > Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> > Cc: Samuel Holland <samuel.holland@sifive.com> > --- > arch/riscv/kernel/probes/kprobes.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/riscv/kernel/probes/kprobes.c b/arch/riscv/kernel/probes/kprobes.c > index 380a0e8cecc0..c0738d6c6498 100644 > --- a/arch/riscv/kernel/probes/kprobes.c > +++ b/arch/riscv/kernel/probes/kprobes.c > @@ -30,7 +30,7 @@ static void __kprobes arch_prepare_ss_slot(struct kprobe *p) > p->ainsn.api.restore = (unsigned long)p->addr + len; > > patch_text_nosync(p->ainsn.api.insn, &p->opcode, len); > - patch_text_nosync(p->ainsn.api.insn + len, &insn, GET_INSN_LENGTH(insn)); > + patch_text_nosync((void *)p->ainsn.api.insn + len, &insn, GET_INSN_LENGTH(insn)); > } > > static void __kprobes arch_prepare_simulate(struct kprobe *p) Nam already came up with the same fix a few weeks ago: https://lore.kernel.org/linux-riscv/20241119111056.2554419-1-namcao@linutronix.de/ Thanks anyway! Alex
diff --git a/arch/riscv/kernel/probes/kprobes.c b/arch/riscv/kernel/probes/kprobes.c index 380a0e8cecc0..c0738d6c6498 100644 --- a/arch/riscv/kernel/probes/kprobes.c +++ b/arch/riscv/kernel/probes/kprobes.c @@ -30,7 +30,7 @@ static void __kprobes arch_prepare_ss_slot(struct kprobe *p) p->ainsn.api.restore = (unsigned long)p->addr + len; patch_text_nosync(p->ainsn.api.insn, &p->opcode, len); - patch_text_nosync(p->ainsn.api.insn + len, &insn, GET_INSN_LENGTH(insn)); + patch_text_nosync((void *)p->ainsn.api.insn + len, &insn, GET_INSN_LENGTH(insn)); } static void __kprobes arch_prepare_simulate(struct kprobe *p)
The second patch_text_nosync() in arch_prepare_ss_slot should be pointing after the first instruction, however p->ainsn.api.insn is a pointer to a probe_opcode_t which is 4 bytes... this means an instruction of length 2 moves the pointer by 8 bytes. This means a kprobe may fail to work and recurse into the bad instruction handler (which then itself fails as the original lock still seems to be held?) Fixes: b1756750a397f36ddc857 ("riscv: kprobes: Use patch_text_nosync() for insn slots") Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> Cc: Samuel Holland <samuel.holland@sifive.com> --- arch/riscv/kernel/probes/kprobes.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)