From: Guo Ren <[email protected]>
The kernel would panic when probed for an illegal position. eg:
(CONFIG_RISCV_ISA_C=n)
echo 'p:hello kernel_clone+0x16 a0=%a0' >> kprobe_events
echo 1 > events/kprobes/hello/enable
cat trace
Kernel panic - not syncing: stack-protector: Kernel stack
is corrupted in: __do_sys_newfstatat+0xb8/0xb8
CPU: 0 PID: 111 Comm: sh Not tainted
6.2.0-rc1-00027-g2d398fe49a4d #490
Hardware name: riscv-virtio,qemu (DT)
Call Trace:
[<ffffffff80007268>] dump_backtrace+0x38/0x48
[<ffffffff80c5e83c>] show_stack+0x50/0x68
[<ffffffff80c6da28>] dump_stack_lvl+0x60/0x84
[<ffffffff80c6da6c>] dump_stack+0x20/0x30
[<ffffffff80c5ecf4>] panic+0x160/0x374
[<ffffffff80c6db94>] generic_handle_arch_irq+0x0/0xa8
[<ffffffff802deeb0>] sys_newstat+0x0/0x30
[<ffffffff800158c0>] sys_clone+0x20/0x30
[<ffffffff800039e8>] ret_from_syscall+0x0/0x4
---[ end Kernel panic - not syncing: stack-protector:
Kernel stack is corrupted in: __do_sys_newfstatat+0xb8/0xb8 ]---
That is because the kprobe's ebreak instruction broke the kernel's
original code. The user should guarantee the correction of the probe
position, but it couldn't make the kernel panic.
This patch adds arch_check_kprobe in arch_prepare_kprobe to prevent an
illegal position (Such as the middle of an instruction).
Fixes: c22b0bcb1dd0 ("riscv: Add kprobes supported")
Signed-off-by: Guo Ren <[email protected]>
Signed-off-by: Guo Ren <[email protected]>
---
arch/riscv/kernel/probes/kprobes.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/arch/riscv/kernel/probes/kprobes.c b/arch/riscv/kernel/probes/kprobes.c
index f21592d20306..475989f06d6d 100644
--- a/arch/riscv/kernel/probes/kprobes.c
+++ b/arch/riscv/kernel/probes/kprobes.c
@@ -48,6 +48,21 @@ static void __kprobes arch_simulate_insn(struct kprobe *p, struct pt_regs *regs)
post_kprobe_handler(p, kcb, regs);
}
+static bool __kprobes arch_check_kprobe(struct kprobe *p)
+{
+ unsigned long tmp = (unsigned long)p->addr - p->offset;
+ unsigned long addr = (unsigned long)p->addr;
+
+ while (tmp <= addr) {
+ if (tmp == addr)
+ return true;
+
+ tmp += GET_INSN_LENGTH(*(kprobe_opcode_t *)tmp);
+ }
+
+ return false;
+}
+
int __kprobes arch_prepare_kprobe(struct kprobe *p)
{
unsigned long probe_addr = (unsigned long)p->addr;
@@ -55,6 +70,9 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
if (probe_addr & 0x1)
return -EILSEQ;
+ if (!arch_check_kprobe(p))
+ return -EILSEQ;
+
/* copy instruction */
p->opcode = *p->addr;
--
2.36.1
在 2023/1/26 21:05, [email protected] 写道:
> From: Guo Ren <[email protected]>
>
> The kernel would panic when probed for an illegal position. eg:
>
> (CONFIG_RISCV_ISA_C=n)
>
> echo 'p:hello kernel_clone+0x16 a0=%a0' >> kprobe_events
> echo 1 > events/kprobes/hello/enable
> cat trace
>
> Kernel panic - not syncing: stack-protector: Kernel stack
> is corrupted in: __do_sys_newfstatat+0xb8/0xb8
> CPU: 0 PID: 111 Comm: sh Not tainted
> 6.2.0-rc1-00027-g2d398fe49a4d #490
> Hardware name: riscv-virtio,qemu (DT)
> Call Trace:
> [<ffffffff80007268>] dump_backtrace+0x38/0x48
> [<ffffffff80c5e83c>] show_stack+0x50/0x68
> [<ffffffff80c6da28>] dump_stack_lvl+0x60/0x84
> [<ffffffff80c6da6c>] dump_stack+0x20/0x30
> [<ffffffff80c5ecf4>] panic+0x160/0x374
> [<ffffffff80c6db94>] generic_handle_arch_irq+0x0/0xa8
> [<ffffffff802deeb0>] sys_newstat+0x0/0x30
> [<ffffffff800158c0>] sys_clone+0x20/0x30
> [<ffffffff800039e8>] ret_from_syscall+0x0/0x4
> ---[ end Kernel panic - not syncing: stack-protector:
> Kernel stack is corrupted in: __do_sys_newfstatat+0xb8/0xb8 ]---
>
> That is because the kprobe's ebreak instruction broke the kernel's
> original code. The user should guarantee the correction of the probe
> position, but it couldn't make the kernel panic.
>
> This patch adds arch_check_kprobe in arch_prepare_kprobe to prevent an
> illegal position (Such as the middle of an instruction).
>
> Fixes: c22b0bcb1dd0 ("riscv: Add kprobes supported")
> Signed-off-by: Guo Ren <[email protected]>
> Signed-off-by: Guo Ren <[email protected]>
> ---
> arch/riscv/kernel/probes/kprobes.c | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
> diff --git a/arch/riscv/kernel/probes/kprobes.c b/arch/riscv/kernel/probes/kprobes.c
> index f21592d20306..475989f06d6d 100644
> --- a/arch/riscv/kernel/probes/kprobes.c
> +++ b/arch/riscv/kernel/probes/kprobes.c
> @@ -48,6 +48,21 @@ static void __kprobes arch_simulate_insn(struct kprobe *p, struct pt_regs *regs)
> post_kprobe_handler(p, kcb, regs);
> }
>
> +static bool __kprobes arch_check_kprobe(struct kprobe *p)
> +{
> + unsigned long tmp = (unsigned long)p->addr - p->offset;
> + unsigned long addr = (unsigned long)p->addr;
> +
> + while (tmp <= addr) {
> + if (tmp == addr)
> + return true;
> +
> + tmp += GET_INSN_LENGTH(*(kprobe_opcode_t *)tmp);
> + }
> +
> + return false;
> +}
LGTM.
I have submit a patch to fix the same problem, found at:
https://lore.kernel.org/lkml/[email protected]/
So this boundary check is necessary no matter CONFIG_RISCV_ISA_C is enable or not, right?
> +
> int __kprobes arch_prepare_kprobe(struct kprobe *p)
> {
> unsigned long probe_addr = (unsigned long)p->addr;
> @@ -55,6 +70,9 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
> if (probe_addr & 0x1)
> return -EILSEQ;
>
> + if (!arch_check_kprobe(p))
> + return -EILSEQ;
> +
> /* copy instruction */
> p->opcode = *p->addr;
>
--
BR,
Liao, Chang
On Sat, Jan 28, 2023 at 10:55 AM liaochang (A) <[email protected]> wrote:
>
>
>
> 在 2023/1/26 21:05, [email protected] 写道:
> > From: Guo Ren <[email protected]>
> >
> > The kernel would panic when probed for an illegal position. eg:
> >
> > (CONFIG_RISCV_ISA_C=n)
> >
> > echo 'p:hello kernel_clone+0x16 a0=%a0' >> kprobe_events
> > echo 1 > events/kprobes/hello/enable
> > cat trace
> >
> > Kernel panic - not syncing: stack-protector: Kernel stack
> > is corrupted in: __do_sys_newfstatat+0xb8/0xb8
> > CPU: 0 PID: 111 Comm: sh Not tainted
> > 6.2.0-rc1-00027-g2d398fe49a4d #490
> > Hardware name: riscv-virtio,qemu (DT)
> > Call Trace:
> > [<ffffffff80007268>] dump_backtrace+0x38/0x48
> > [<ffffffff80c5e83c>] show_stack+0x50/0x68
> > [<ffffffff80c6da28>] dump_stack_lvl+0x60/0x84
> > [<ffffffff80c6da6c>] dump_stack+0x20/0x30
> > [<ffffffff80c5ecf4>] panic+0x160/0x374
> > [<ffffffff80c6db94>] generic_handle_arch_irq+0x0/0xa8
> > [<ffffffff802deeb0>] sys_newstat+0x0/0x30
> > [<ffffffff800158c0>] sys_clone+0x20/0x30
> > [<ffffffff800039e8>] ret_from_syscall+0x0/0x4
> > ---[ end Kernel panic - not syncing: stack-protector:
> > Kernel stack is corrupted in: __do_sys_newfstatat+0xb8/0xb8 ]---
> >
> > That is because the kprobe's ebreak instruction broke the kernel's
> > original code. The user should guarantee the correction of the probe
> > position, but it couldn't make the kernel panic.
> >
> > This patch adds arch_check_kprobe in arch_prepare_kprobe to prevent an
> > illegal position (Such as the middle of an instruction).
> >
> > Fixes: c22b0bcb1dd0 ("riscv: Add kprobes supported")
> > Signed-off-by: Guo Ren <[email protected]>
> > Signed-off-by: Guo Ren <[email protected]>
> > ---
> > arch/riscv/kernel/probes/kprobes.c | 18 ++++++++++++++++++
> > 1 file changed, 18 insertions(+)
> >
> > diff --git a/arch/riscv/kernel/probes/kprobes.c b/arch/riscv/kernel/probes/kprobes.c
> > index f21592d20306..475989f06d6d 100644
> > --- a/arch/riscv/kernel/probes/kprobes.c
> > +++ b/arch/riscv/kernel/probes/kprobes.c
> > @@ -48,6 +48,21 @@ static void __kprobes arch_simulate_insn(struct kprobe *p, struct pt_regs *regs)
> > post_kprobe_handler(p, kcb, regs);
> > }
> >
> > +static bool __kprobes arch_check_kprobe(struct kprobe *p)
> > +{
> > + unsigned long tmp = (unsigned long)p->addr - p->offset;
> > + unsigned long addr = (unsigned long)p->addr;
> > +
> > + while (tmp <= addr) {
> > + if (tmp == addr)
> > + return true;
> > +
> > + tmp += GET_INSN_LENGTH(*(kprobe_opcode_t *)tmp);
> > + }
> > +
> > + return false;
> > +}
>
> LGTM.
>
> I have submit a patch to fix the same problem, found at:
Oh, I missed that patch. Our goal is the same.
But it would be best if you reused p->offset, not
kallsyms_lookup_size_offset, the p->addr added by _kprobe_addr
(kernel/kprobes.c), not just kallsyms_lookup_size_offset. Sure, it
works around for the current riscv, but that's not correct.
>
> https://lore.kernel.org/lkml/[email protected]/
>
> So this boundary check is necessary no matter CONFIG_RISCV_ISA_C is enable or not, right?
Yes, my panic example in the commit log is based on the
!CONFIG_RISCV_ISA_C, you couldn't miss that.
>
>
> > +
> > int __kprobes arch_prepare_kprobe(struct kprobe *p)
> > {
> > unsigned long probe_addr = (unsigned long)p->addr;
> > @@ -55,6 +70,9 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
> > if (probe_addr & 0x1)
> > return -EILSEQ;
> >
> > + if (!arch_check_kprobe(p))
> > + return -EILSEQ;
> > +
> > /* copy instruction */
> > p->opcode = *p->addr;
> >
>
> --
> BR,
> Liao, Chang
--
Best Regards
Guo Ren
On Sat, Jan 28, 2023 at 11:46 AM Guo Ren <[email protected]> wrote:
>
> On Sat, Jan 28, 2023 at 10:55 AM liaochang (A) <[email protected]> wrote:
> >
> >
> >
> > 在 2023/1/26 21:05, [email protected] 写道:
> > > From: Guo Ren <[email protected]>
> > >
> > > The kernel would panic when probed for an illegal position. eg:
> > >
> > > (CONFIG_RISCV_ISA_C=n)
> > >
> > > echo 'p:hello kernel_clone+0x16 a0=%a0' >> kprobe_events
> > > echo 1 > events/kprobes/hello/enable
> > > cat trace
> > >
> > > Kernel panic - not syncing: stack-protector: Kernel stack
> > > is corrupted in: __do_sys_newfstatat+0xb8/0xb8
> > > CPU: 0 PID: 111 Comm: sh Not tainted
> > > 6.2.0-rc1-00027-g2d398fe49a4d #490
> > > Hardware name: riscv-virtio,qemu (DT)
> > > Call Trace:
> > > [<ffffffff80007268>] dump_backtrace+0x38/0x48
> > > [<ffffffff80c5e83c>] show_stack+0x50/0x68
> > > [<ffffffff80c6da28>] dump_stack_lvl+0x60/0x84
> > > [<ffffffff80c6da6c>] dump_stack+0x20/0x30
> > > [<ffffffff80c5ecf4>] panic+0x160/0x374
> > > [<ffffffff80c6db94>] generic_handle_arch_irq+0x0/0xa8
> > > [<ffffffff802deeb0>] sys_newstat+0x0/0x30
> > > [<ffffffff800158c0>] sys_clone+0x20/0x30
> > > [<ffffffff800039e8>] ret_from_syscall+0x0/0x4
> > > ---[ end Kernel panic - not syncing: stack-protector:
> > > Kernel stack is corrupted in: __do_sys_newfstatat+0xb8/0xb8 ]---
> > >
> > > That is because the kprobe's ebreak instruction broke the kernel's
> > > original code. The user should guarantee the correction of the probe
> > > position, but it couldn't make the kernel panic.
> > >
> > > This patch adds arch_check_kprobe in arch_prepare_kprobe to prevent an
> > > illegal position (Such as the middle of an instruction).
> > >
> > > Fixes: c22b0bcb1dd0 ("riscv: Add kprobes supported")
> > > Signed-off-by: Guo Ren <[email protected]>
> > > Signed-off-by: Guo Ren <[email protected]>
> > > ---
> > > arch/riscv/kernel/probes/kprobes.c | 18 ++++++++++++++++++
> > > 1 file changed, 18 insertions(+)
> > >
> > > diff --git a/arch/riscv/kernel/probes/kprobes.c b/arch/riscv/kernel/probes/kprobes.c
> > > index f21592d20306..475989f06d6d 100644
> > > --- a/arch/riscv/kernel/probes/kprobes.c
> > > +++ b/arch/riscv/kernel/probes/kprobes.c
> > > @@ -48,6 +48,21 @@ static void __kprobes arch_simulate_insn(struct kprobe *p, struct pt_regs *regs)
> > > post_kprobe_handler(p, kcb, regs);
> > > }
> > >
> > > +static bool __kprobes arch_check_kprobe(struct kprobe *p)
> > > +{
> > > + unsigned long tmp = (unsigned long)p->addr - p->offset;
> > > + unsigned long addr = (unsigned long)p->addr;
> > > +
> > > + while (tmp <= addr) {
> > > + if (tmp == addr)
> > > + return true;
> > > +
> > > + tmp += GET_INSN_LENGTH(*(kprobe_opcode_t *)tmp);
> > > + }
> > > +
> > > + return false;
> > > +}
> >
> > LGTM.
> >
> > I have submit a patch to fix the same problem, found at:
> Oh, I missed that patch. Our goal is the same.
>
> But it would be best if you reused p->offset, not
> kallsyms_lookup_size_offset, the p->addr added by _kprobe_addr
> (kernel/kprobes.c), not just kallsyms_lookup_size_offset. Sure, it
> works around for the current riscv, but that's not correct.
Sorry, the above description is a little bit confusing. What I mean is that:
The p->addr = func_entry + p->offset. Not kallsyms_lookup_size_offset.
Your patch could get the wrong func_entry.
>
> >
> > https://lore.kernel.org/lkml/[email protected]/
> >
> > So this boundary check is necessary no matter CONFIG_RISCV_ISA_C is enable or not, right?
> Yes, my panic example in the commit log is based on the
> !CONFIG_RISCV_ISA_C, you couldn't miss that.
>
> >
> >
> > > +
> > > int __kprobes arch_prepare_kprobe(struct kprobe *p)
> > > {
> > > unsigned long probe_addr = (unsigned long)p->addr;
> > > @@ -55,6 +70,9 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
> > > if (probe_addr & 0x1)
> > > return -EILSEQ;
> > >
> > > + if (!arch_check_kprobe(p))
> > > + return -EILSEQ;
> > > +
> > > /* copy instruction */
> > > p->opcode = *p->addr;
> > >
> >
> > --
> > BR,
> > Liao, Chang
>
>
>
> --
> Best Regards
> Guo Ren
--
Best Regards
Guo Ren
On Sat, Jan 28, 2023 at 10:55 AM liaochang (A) <[email protected]> wrote:
>
>
>
> 在 2023/1/26 21:05, [email protected] 写道:
> > From: Guo Ren <[email protected]>
> >
> > The kernel would panic when probed for an illegal position. eg:
> >
> > (CONFIG_RISCV_ISA_C=n)
> >
> > echo 'p:hello kernel_clone+0x16 a0=%a0' >> kprobe_events
> > echo 1 > events/kprobes/hello/enable
> > cat trace
> >
> > Kernel panic - not syncing: stack-protector: Kernel stack
> > is corrupted in: __do_sys_newfstatat+0xb8/0xb8
> > CPU: 0 PID: 111 Comm: sh Not tainted
> > 6.2.0-rc1-00027-g2d398fe49a4d #490
> > Hardware name: riscv-virtio,qemu (DT)
> > Call Trace:
> > [<ffffffff80007268>] dump_backtrace+0x38/0x48
> > [<ffffffff80c5e83c>] show_stack+0x50/0x68
> > [<ffffffff80c6da28>] dump_stack_lvl+0x60/0x84
> > [<ffffffff80c6da6c>] dump_stack+0x20/0x30
> > [<ffffffff80c5ecf4>] panic+0x160/0x374
> > [<ffffffff80c6db94>] generic_handle_arch_irq+0x0/0xa8
> > [<ffffffff802deeb0>] sys_newstat+0x0/0x30
> > [<ffffffff800158c0>] sys_clone+0x20/0x30
> > [<ffffffff800039e8>] ret_from_syscall+0x0/0x4
> > ---[ end Kernel panic - not syncing: stack-protector:
> > Kernel stack is corrupted in: __do_sys_newfstatat+0xb8/0xb8 ]---
> >
> > That is because the kprobe's ebreak instruction broke the kernel's
> > original code. The user should guarantee the correction of the probe
> > position, but it couldn't make the kernel panic.
> >
> > This patch adds arch_check_kprobe in arch_prepare_kprobe to prevent an
> > illegal position (Such as the middle of an instruction).
> >
> > Fixes: c22b0bcb1dd0 ("riscv: Add kprobes supported")
> > Signed-off-by: Guo Ren <[email protected]>
> > Signed-off-by: Guo Ren <[email protected]>
> > ---
> > arch/riscv/kernel/probes/kprobes.c | 18 ++++++++++++++++++
> > 1 file changed, 18 insertions(+)
> >
> > diff --git a/arch/riscv/kernel/probes/kprobes.c b/arch/riscv/kernel/probes/kprobes.c
> > index f21592d20306..475989f06d6d 100644
> > --- a/arch/riscv/kernel/probes/kprobes.c
> > +++ b/arch/riscv/kernel/probes/kprobes.c
> > @@ -48,6 +48,21 @@ static void __kprobes arch_simulate_insn(struct kprobe *p, struct pt_regs *regs)
> > post_kprobe_handler(p, kcb, regs);
> > }
> >
> > +static bool __kprobes arch_check_kprobe(struct kprobe *p)
> > +{
> > + unsigned long tmp = (unsigned long)p->addr - p->offset;
> > + unsigned long addr = (unsigned long)p->addr;
> > +
> > + while (tmp <= addr) {
> > + if (tmp == addr)
> > + return true;
> > +
> > + tmp += GET_INSN_LENGTH(*(kprobe_opcode_t *)tmp);
> > + }
> > +
> > + return false;
> > +}
>
> LGTM.
>
> I have submit a patch to fix the same problem, found at:
>
> https://lore.kernel.org/lkml/[email protected]/
I have a question on your OPTKPROBE patch:
https://lore.kernel.org/lkml/[email protected]/
@@ -490,7 +573,14 @@ int arch_prepare_optimized_kprobe(struct
optimized_kprobe *op,
* to detour buffer, ra is used to form JR jumping back from detour
* buffer.
*/
- find_free_registers(orig, op, &rd, &ra);
+ if (used_reg == ALL_REG_OCCUPIED) {
+ find_free_registers(orig, op, &rd, &ra);
+ } else {
+ /* Choose one unused caller-saved register. */
+ rd = ffz(used_reg);
+ ra = rd;
+ }
+
if (rd == 0 || ra == 0) {
^^^^^^^^^^^^^^^^^^
After no opt_used_dst_reg & no free caller-saved register (Of cause,
it's very rare for no available tmp regs):
Why not try:
0: REG_S ra, -SZREG(sp)
4: auipc ra, ?
8: jalr ?(ra)
12: REG_L ra, -SZREG(sp)
Besides taking up more instruction slots, does it have other problems?
>
> So this boundary check is necessary no matter CONFIG_RISCV_ISA_C is enable or not, right?
>
>
> > +
> > int __kprobes arch_prepare_kprobe(struct kprobe *p)
> > {
> > unsigned long probe_addr = (unsigned long)p->addr;
> > @@ -55,6 +70,9 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
> > if (probe_addr & 0x1)
> > return -EILSEQ;
> >
> > + if (!arch_check_kprobe(p))
> > + return -EILSEQ;
> > +
> > /* copy instruction */
> > p->opcode = *p->addr;
> >
>
> --
> BR,
> Liao, Chang
--
Best Regards
Guo Ren
在 2023/1/28 11:52, Guo Ren 写道:
> On Sat, Jan 28, 2023 at 11:46 AM Guo Ren <[email protected]> wrote:
>>
>> On Sat, Jan 28, 2023 at 10:55 AM liaochang (A) <[email protected]> wrote:
>>>
>>>
>>>
>>> 在 2023/1/26 21:05, [email protected] 写道:
>>>> From: Guo Ren <[email protected]>
>>>>
>>>> The kernel would panic when probed for an illegal position. eg:
>>>>
>>>> (CONFIG_RISCV_ISA_C=n)
>>>>
>>>> echo 'p:hello kernel_clone+0x16 a0=%a0' >> kprobe_events
>>>> echo 1 > events/kprobes/hello/enable
>>>> cat trace
>>>>
>>>> Kernel panic - not syncing: stack-protector: Kernel stack
>>>> is corrupted in: __do_sys_newfstatat+0xb8/0xb8
>>>> CPU: 0 PID: 111 Comm: sh Not tainted
>>>> 6.2.0-rc1-00027-g2d398fe49a4d #490
>>>> Hardware name: riscv-virtio,qemu (DT)
>>>> Call Trace:
>>>> [<ffffffff80007268>] dump_backtrace+0x38/0x48
>>>> [<ffffffff80c5e83c>] show_stack+0x50/0x68
>>>> [<ffffffff80c6da28>] dump_stack_lvl+0x60/0x84
>>>> [<ffffffff80c6da6c>] dump_stack+0x20/0x30
>>>> [<ffffffff80c5ecf4>] panic+0x160/0x374
>>>> [<ffffffff80c6db94>] generic_handle_arch_irq+0x0/0xa8
>>>> [<ffffffff802deeb0>] sys_newstat+0x0/0x30
>>>> [<ffffffff800158c0>] sys_clone+0x20/0x30
>>>> [<ffffffff800039e8>] ret_from_syscall+0x0/0x4
>>>> ---[ end Kernel panic - not syncing: stack-protector:
>>>> Kernel stack is corrupted in: __do_sys_newfstatat+0xb8/0xb8 ]---
>>>>
>>>> That is because the kprobe's ebreak instruction broke the kernel's
>>>> original code. The user should guarantee the correction of the probe
>>>> position, but it couldn't make the kernel panic.
>>>>
>>>> This patch adds arch_check_kprobe in arch_prepare_kprobe to prevent an
>>>> illegal position (Such as the middle of an instruction).
>>>>
>>>> Fixes: c22b0bcb1dd0 ("riscv: Add kprobes supported")
>>>> Signed-off-by: Guo Ren <[email protected]>
>>>> Signed-off-by: Guo Ren <[email protected]>
>>>> ---
>>>> arch/riscv/kernel/probes/kprobes.c | 18 ++++++++++++++++++
>>>> 1 file changed, 18 insertions(+)
>>>>
>>>> diff --git a/arch/riscv/kernel/probes/kprobes.c b/arch/riscv/kernel/probes/kprobes.c
>>>> index f21592d20306..475989f06d6d 100644
>>>> --- a/arch/riscv/kernel/probes/kprobes.c
>>>> +++ b/arch/riscv/kernel/probes/kprobes.c
>>>> @@ -48,6 +48,21 @@ static void __kprobes arch_simulate_insn(struct kprobe *p, struct pt_regs *regs)
>>>> post_kprobe_handler(p, kcb, regs);
>>>> }
>>>>
>>>> +static bool __kprobes arch_check_kprobe(struct kprobe *p)
>>>> +{
>>>> + unsigned long tmp = (unsigned long)p->addr - p->offset;
>>>> + unsigned long addr = (unsigned long)p->addr;
>>>> +
>>>> + while (tmp <= addr) {
>>>> + if (tmp == addr)
>>>> + return true;
>>>> +
>>>> + tmp += GET_INSN_LENGTH(*(kprobe_opcode_t *)tmp);
>>>> + }
>>>> +
>>>> + return false;
>>>> +}
>>>
>>> LGTM.
>>>
>>> I have submit a patch to fix the same problem, found at:
>> Oh, I missed that patch. Our goal is the same.
>>
>> But it would be best if you reused p->offset, not
>> kallsyms_lookup_size_offset, the p->addr added by _kprobe_addr
>> (kernel/kprobes.c), not just kallsyms_lookup_size_offset. Sure, it
>> works around for the current riscv, but that's not correct.
> Sorry, the above description is a little bit confusing. What I mean is that:
> The p->addr = func_entry + p->offset. Not kallsyms_lookup_size_offset.
> Your patch could get the wrong func_entry.
According to my testing and debuggin on QEMU, I think both your patch and mine is able
to find the correct func_entry, because _kprobe_addr also uses kallsyms_lookup_size_offset
to get the first instruction address of given symbol. Of course, i prefere to your patch because it reuse p->offset.
Thanks.
>
>>
>>>
>>> https://lore.kernel.org/lkml/[email protected]/
>>>
>>> So this boundary check is necessary no matter CONFIG_RISCV_ISA_C is enable or not, right?
>> Yes, my panic example in the commit log is based on the
>> !CONFIG_RISCV_ISA_C, you couldn't miss that.
>>
>>>
>>>
>>>> +
>>>> int __kprobes arch_prepare_kprobe(struct kprobe *p)
>>>> {
>>>> unsigned long probe_addr = (unsigned long)p->addr;
>>>> @@ -55,6 +70,9 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
>>>> if (probe_addr & 0x1)
>>>> return -EILSEQ;
>>>>
>>>> + if (!arch_check_kprobe(p))
>>>> + return -EILSEQ;
>>>> +
>>>> /* copy instruction */
>>>> p->opcode = *p->addr;
>>>>
>>>
>>> --
>>> BR,
>>> Liao, Chang
>>
>>
>>
>> --
>> Best Regards
>> Guo Ren
>
>
>
--
BR,
Liao, Chang
在 2023/1/28 13:22, Guo Ren 写道:
> On Sat, Jan 28, 2023 at 10:55 AM liaochang (A) <[email protected]> wrote:
>>
>>
>>
>> 在 2023/1/26 21:05, [email protected] 写道:
>>> From: Guo Ren <[email protected]>
>>>
>>> The kernel would panic when probed for an illegal position. eg:
>>>
>>> (CONFIG_RISCV_ISA_C=n)
>>>
>>> echo 'p:hello kernel_clone+0x16 a0=%a0' >> kprobe_events
>>> echo 1 > events/kprobes/hello/enable
>>> cat trace
>>>
>>> Kernel panic - not syncing: stack-protector: Kernel stack
>>> is corrupted in: __do_sys_newfstatat+0xb8/0xb8
>>> CPU: 0 PID: 111 Comm: sh Not tainted
>>> 6.2.0-rc1-00027-g2d398fe49a4d #490
>>> Hardware name: riscv-virtio,qemu (DT)
>>> Call Trace:
>>> [<ffffffff80007268>] dump_backtrace+0x38/0x48
>>> [<ffffffff80c5e83c>] show_stack+0x50/0x68
>>> [<ffffffff80c6da28>] dump_stack_lvl+0x60/0x84
>>> [<ffffffff80c6da6c>] dump_stack+0x20/0x30
>>> [<ffffffff80c5ecf4>] panic+0x160/0x374
>>> [<ffffffff80c6db94>] generic_handle_arch_irq+0x0/0xa8
>>> [<ffffffff802deeb0>] sys_newstat+0x0/0x30
>>> [<ffffffff800158c0>] sys_clone+0x20/0x30
>>> [<ffffffff800039e8>] ret_from_syscall+0x0/0x4
>>> ---[ end Kernel panic - not syncing: stack-protector:
>>> Kernel stack is corrupted in: __do_sys_newfstatat+0xb8/0xb8 ]---
>>>
>>> That is because the kprobe's ebreak instruction broke the kernel's
>>> original code. The user should guarantee the correction of the probe
>>> position, but it couldn't make the kernel panic.
>>>
>>> This patch adds arch_check_kprobe in arch_prepare_kprobe to prevent an
>>> illegal position (Such as the middle of an instruction).
>>>
>>> Fixes: c22b0bcb1dd0 ("riscv: Add kprobes supported")
>>> Signed-off-by: Guo Ren <[email protected]>
>>> Signed-off-by: Guo Ren <[email protected]>
>>> ---
>>> arch/riscv/kernel/probes/kprobes.c | 18 ++++++++++++++++++
>>> 1 file changed, 18 insertions(+)
>>>
>>> diff --git a/arch/riscv/kernel/probes/kprobes.c b/arch/riscv/kernel/probes/kprobes.c
>>> index f21592d20306..475989f06d6d 100644
>>> --- a/arch/riscv/kernel/probes/kprobes.c
>>> +++ b/arch/riscv/kernel/probes/kprobes.c
>>> @@ -48,6 +48,21 @@ static void __kprobes arch_simulate_insn(struct kprobe *p, struct pt_regs *regs)
>>> post_kprobe_handler(p, kcb, regs);
>>> }
>>>
>>> +static bool __kprobes arch_check_kprobe(struct kprobe *p)
>>> +{
>>> + unsigned long tmp = (unsigned long)p->addr - p->offset;
>>> + unsigned long addr = (unsigned long)p->addr;
>>> +
>>> + while (tmp <= addr) {
>>> + if (tmp == addr)
>>> + return true;
>>> +
>>> + tmp += GET_INSN_LENGTH(*(kprobe_opcode_t *)tmp);
>>> + }
>>> +
>>> + return false;
>>> +}
>>
>> LGTM.
>>
>> I have submit a patch to fix the same problem, found at:
>>
>> https://lore.kernel.org/lkml/[email protected]/
> I have a question on your OPTKPROBE patch:
> https://lore.kernel.org/lkml/[email protected]/
>
> @@ -490,7 +573,14 @@ int arch_prepare_optimized_kprobe(struct
> optimized_kprobe *op,
> * to detour buffer, ra is used to form JR jumping back from detour
> * buffer.
> */
> - find_free_registers(orig, op, &rd, &ra);
> + if (used_reg == ALL_REG_OCCUPIED) {
> + find_free_registers(orig, op, &rd, &ra);
> + } else {
> + /* Choose one unused caller-saved register. */
> + rd = ffz(used_reg);
> + ra = rd;
> + }
> +
> if (rd == 0 || ra == 0) {
> ^^^^^^^^^^^^^^^^^^
> After no opt_used_dst_reg & no free caller-saved register (Of cause,
> it's very rare for no available tmp regs):
Thanks for reviewing.
If this case occurs, it means all 32 integer registers are used as the source in opcode,
I have no deep knowledge about the code generation and register allocation of riscv compiler,
but guess it is not possible to generate such style of instructions sequence for one really
meaningful kernel function.
>
> Why not try:
>
> 0: REG_S ra, -SZREG(sp)
> 4: auipc ra, ?
> 8: jalr ?(ra)
> 12: REG_L ra, -SZREG(sp)
>
> Besides taking up more instruction slots, does it have other problems?
I think it okay in some term, two points need to pay attention:
1. It needs to update the 'ra' entry in stack if ra is modified in detour buffer.
2. If sp is modified in detour buffer, #12 might return a illegal value, it is a common
case when set kprobe at function prologue and epilogue.
>
>>
>> So this boundary check is necessary no matter CONFIG_RISCV_ISA_C is enable or not, right?
>>
>>
>>> +
>>> int __kprobes arch_prepare_kprobe(struct kprobe *p)
>>> {
>>> unsigned long probe_addr = (unsigned long)p->addr;
>>> @@ -55,6 +70,9 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
>>> if (probe_addr & 0x1)
>>> return -EILSEQ;
>>>
>>> + if (!arch_check_kprobe(p))
>>> + return -EILSEQ;
>>> +
>>> /* copy instruction */
>>> p->opcode = *p->addr;
>>>
>>
>> --
>> BR,
>> Liao, Chang
>
>
>
--
BR,
Liao, Chang
[email protected] writes:
> From: Guo Ren <[email protected]>
>
> The kernel would panic when probed for an illegal position. eg:
>
> (CONFIG_RISCV_ISA_C=n)
>
> echo 'p:hello kernel_clone+0x16 a0=%a0' >> kprobe_events
> echo 1 > events/kprobes/hello/enable
> cat trace
>
> Kernel panic - not syncing: stack-protector: Kernel stack
> is corrupted in: __do_sys_newfstatat+0xb8/0xb8
> CPU: 0 PID: 111 Comm: sh Not tainted
> 6.2.0-rc1-00027-g2d398fe49a4d #490
> Hardware name: riscv-virtio,qemu (DT)
> Call Trace:
> [<ffffffff80007268>] dump_backtrace+0x38/0x48
> [<ffffffff80c5e83c>] show_stack+0x50/0x68
> [<ffffffff80c6da28>] dump_stack_lvl+0x60/0x84
> [<ffffffff80c6da6c>] dump_stack+0x20/0x30
> [<ffffffff80c5ecf4>] panic+0x160/0x374
> [<ffffffff80c6db94>] generic_handle_arch_irq+0x0/0xa8
> [<ffffffff802deeb0>] sys_newstat+0x0/0x30
> [<ffffffff800158c0>] sys_clone+0x20/0x30
> [<ffffffff800039e8>] ret_from_syscall+0x0/0x4
> ---[ end Kernel panic - not syncing: stack-protector:
> Kernel stack is corrupted in: __do_sys_newfstatat+0xb8/0xb8 ]---
>
> That is because the kprobe's ebreak instruction broke the kernel's
> original code. The user should guarantee the correction of the probe
> position, but it couldn't make the kernel panic.
>
> This patch adds arch_check_kprobe in arch_prepare_kprobe to prevent an
> illegal position (Such as the middle of an instruction).
Nice!
@liaochang Will you remove your patch from the OPTPROBE series?
> Fixes: c22b0bcb1dd0 ("riscv: Add kprobes supported")
> Signed-off-by: Guo Ren <[email protected]>
> Signed-off-by: Guo Ren <[email protected]>
> ---
> arch/riscv/kernel/probes/kprobes.c | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
> diff --git a/arch/riscv/kernel/probes/kprobes.c b/arch/riscv/kernel/probes/kprobes.c
> index f21592d20306..475989f06d6d 100644
> --- a/arch/riscv/kernel/probes/kprobes.c
> +++ b/arch/riscv/kernel/probes/kprobes.c
> @@ -48,6 +48,21 @@ static void __kprobes arch_simulate_insn(struct kprobe *p, struct pt_regs *regs)
> post_kprobe_handler(p, kcb, regs);
> }
>
> +static bool __kprobes arch_check_kprobe(struct kprobe *p)
> +{
> + unsigned long tmp = (unsigned long)p->addr - p->offset;
> + unsigned long addr = (unsigned long)p->addr;
> +
> + while (tmp <= addr) {
> + if (tmp == addr)
> + return true;
> +
> + tmp += GET_INSN_LENGTH(*(kprobe_opcode_t *)tmp);
kprobe_opcode_t is u32; This can trigger a misaligned load, right?
> + }
> +
> + return false;
> +}
> +
> int __kprobes arch_prepare_kprobe(struct kprobe *p)
> {
> unsigned long probe_addr = (unsigned long)p->addr;
> @@ -55,6 +70,9 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
> if (probe_addr & 0x1)
> return -EILSEQ;
>
> + if (!arch_check_kprobe(p))
> + return -EILSEQ;
> +
> /* copy instruction */
> p->opcode = *p->addr;
Not related to your patch, but this can also trigger a misaligned load.
Björn
On Tue, Jan 31, 2023 at 8:32 PM Björn Töpel <[email protected]> wrote:
>
> [email protected] writes:
>
> > From: Guo Ren <[email protected]>
> >
> > The kernel would panic when probed for an illegal position. eg:
> >
> > (CONFIG_RISCV_ISA_C=n)
> >
> > echo 'p:hello kernel_clone+0x16 a0=%a0' >> kprobe_events
> > echo 1 > events/kprobes/hello/enable
> > cat trace
> >
> > Kernel panic - not syncing: stack-protector: Kernel stack
> > is corrupted in: __do_sys_newfstatat+0xb8/0xb8
> > CPU: 0 PID: 111 Comm: sh Not tainted
> > 6.2.0-rc1-00027-g2d398fe49a4d #490
> > Hardware name: riscv-virtio,qemu (DT)
> > Call Trace:
> > [<ffffffff80007268>] dump_backtrace+0x38/0x48
> > [<ffffffff80c5e83c>] show_stack+0x50/0x68
> > [<ffffffff80c6da28>] dump_stack_lvl+0x60/0x84
> > [<ffffffff80c6da6c>] dump_stack+0x20/0x30
> > [<ffffffff80c5ecf4>] panic+0x160/0x374
> > [<ffffffff80c6db94>] generic_handle_arch_irq+0x0/0xa8
> > [<ffffffff802deeb0>] sys_newstat+0x0/0x30
> > [<ffffffff800158c0>] sys_clone+0x20/0x30
> > [<ffffffff800039e8>] ret_from_syscall+0x0/0x4
> > ---[ end Kernel panic - not syncing: stack-protector:
> > Kernel stack is corrupted in: __do_sys_newfstatat+0xb8/0xb8 ]---
> >
> > That is because the kprobe's ebreak instruction broke the kernel's
> > original code. The user should guarantee the correction of the probe
> > position, but it couldn't make the kernel panic.
> >
> > This patch adds arch_check_kprobe in arch_prepare_kprobe to prevent an
> > illegal position (Such as the middle of an instruction).
>
> Nice!
>
> @liaochang Will you remove your patch from the OPTPROBE series?
>
> > Fixes: c22b0bcb1dd0 ("riscv: Add kprobes supported")
> > Signed-off-by: Guo Ren <[email protected]>
> > Signed-off-by: Guo Ren <[email protected]>
> > ---
> > arch/riscv/kernel/probes/kprobes.c | 18 ++++++++++++++++++
> > 1 file changed, 18 insertions(+)
> >
> > diff --git a/arch/riscv/kernel/probes/kprobes.c b/arch/riscv/kernel/probes/kprobes.c
> > index f21592d20306..475989f06d6d 100644
> > --- a/arch/riscv/kernel/probes/kprobes.c
> > +++ b/arch/riscv/kernel/probes/kprobes.c
> > @@ -48,6 +48,21 @@ static void __kprobes arch_simulate_insn(struct kprobe *p, struct pt_regs *regs)
> > post_kprobe_handler(p, kcb, regs);
> > }
> >
> > +static bool __kprobes arch_check_kprobe(struct kprobe *p)
> > +{
> > + unsigned long tmp = (unsigned long)p->addr - p->offset;
> > + unsigned long addr = (unsigned long)p->addr;
> > +
> > + while (tmp <= addr) {
> > + if (tmp == addr)
> > + return true;
> > +
> > + tmp += GET_INSN_LENGTH(*(kprobe_opcode_t *)tmp);
>
> kprobe_opcode_t is u32; This can trigger a misaligned load, right?
>
> > + }
> > +
> > + return false;
> > +}
> > +
> > int __kprobes arch_prepare_kprobe(struct kprobe *p)
> > {
> > unsigned long probe_addr = (unsigned long)p->addr;
> > @@ -55,6 +70,9 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
> > if (probe_addr & 0x1)
> > return -EILSEQ;
> >
> > + if (!arch_check_kprobe(p))
> > + return -EILSEQ;
> > +
> > /* copy instruction */
> > p->opcode = *p->addr;
>
> Not related to your patch, but this can also trigger a misaligned load.
Yes, it would trigger a misaligned load.
What's the problem of that?
>
>
> Björn
--
Best Regards
Guo Ren
On Tue, Jan 31, 2023 at 8:32 PM Björn Töpel <[email protected]> wrote:
>
> [email protected] writes:
>
> > From: Guo Ren <[email protected]>
> >
> > The kernel would panic when probed for an illegal position. eg:
> >
> > (CONFIG_RISCV_ISA_C=n)
> >
> > echo 'p:hello kernel_clone+0x16 a0=%a0' >> kprobe_events
> > echo 1 > events/kprobes/hello/enable
> > cat trace
> >
> > Kernel panic - not syncing: stack-protector: Kernel stack
> > is corrupted in: __do_sys_newfstatat+0xb8/0xb8
> > CPU: 0 PID: 111 Comm: sh Not tainted
> > 6.2.0-rc1-00027-g2d398fe49a4d #490
> > Hardware name: riscv-virtio,qemu (DT)
> > Call Trace:
> > [<ffffffff80007268>] dump_backtrace+0x38/0x48
> > [<ffffffff80c5e83c>] show_stack+0x50/0x68
> > [<ffffffff80c6da28>] dump_stack_lvl+0x60/0x84
> > [<ffffffff80c6da6c>] dump_stack+0x20/0x30
> > [<ffffffff80c5ecf4>] panic+0x160/0x374
> > [<ffffffff80c6db94>] generic_handle_arch_irq+0x0/0xa8
> > [<ffffffff802deeb0>] sys_newstat+0x0/0x30
> > [<ffffffff800158c0>] sys_clone+0x20/0x30
> > [<ffffffff800039e8>] ret_from_syscall+0x0/0x4
> > ---[ end Kernel panic - not syncing: stack-protector:
> > Kernel stack is corrupted in: __do_sys_newfstatat+0xb8/0xb8 ]---
> >
> > That is because the kprobe's ebreak instruction broke the kernel's
> > original code. The user should guarantee the correction of the probe
> > position, but it couldn't make the kernel panic.
> >
> > This patch adds arch_check_kprobe in arch_prepare_kprobe to prevent an
> > illegal position (Such as the middle of an instruction).
>
> Nice!
>
> @liaochang Will you remove your patch from the OPTPROBE series?
>
> > Fixes: c22b0bcb1dd0 ("riscv: Add kprobes supported")
> > Signed-off-by: Guo Ren <[email protected]>
> > Signed-off-by: Guo Ren <[email protected]>
> > ---
> > arch/riscv/kernel/probes/kprobes.c | 18 ++++++++++++++++++
> > 1 file changed, 18 insertions(+)
> >
> > diff --git a/arch/riscv/kernel/probes/kprobes.c b/arch/riscv/kernel/probes/kprobes.c
> > index f21592d20306..475989f06d6d 100644
> > --- a/arch/riscv/kernel/probes/kprobes.c
> > +++ b/arch/riscv/kernel/probes/kprobes.c
> > @@ -48,6 +48,21 @@ static void __kprobes arch_simulate_insn(struct kprobe *p, struct pt_regs *regs)
> > post_kprobe_handler(p, kcb, regs);
> > }
> >
> > +static bool __kprobes arch_check_kprobe(struct kprobe *p)
> > +{
> > + unsigned long tmp = (unsigned long)p->addr - p->offset;
> > + unsigned long addr = (unsigned long)p->addr;
> > +
> > + while (tmp <= addr) {
> > + if (tmp == addr)
> > + return true;
> > +
> > + tmp += GET_INSN_LENGTH(*(kprobe_opcode_t *)tmp);
>
> kprobe_opcode_t is u32; This can trigger a misaligned load, right?
>
> > + }
> > +
> > + return false;
> > +}
> > +
> > int __kprobes arch_prepare_kprobe(struct kprobe *p)
> > {
> > unsigned long probe_addr = (unsigned long)p->addr;
> > @@ -55,6 +70,9 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
> > if (probe_addr & 0x1)
> > return -EILSEQ;
> >
> > + if (!arch_check_kprobe(p))
> > + return -EILSEQ;
> > +
> > /* copy instruction */
> > p->opcode = *p->addr;
>
> Not related to your patch, but this can also trigger a misaligned load.
After rereading the spec, misaligned load/store is not mandatory
supported. (Although my machines and qemu are correct)
So I need fixup:
diff --git a/arch/riscv/kernel/probes/kprobes.c
b/arch/riscv/kernel/probes/kprobes.c
index 27f8960c321c..0c016d496746 100644
--- a/arch/riscv/kernel/probes/kprobes.c
+++ b/arch/riscv/kernel/probes/kprobes.c
@@ -58,12 +58,14 @@ static bool __kprobes arch_check_kprobe(struct kprobe *p)
{
unsigned long tmp = (unsigned long)p->addr - p->offset;
unsigned long addr = (unsigned long)p->addr;
+ kprobe_opcode_t opcode;
while (tmp <= addr) {
if (tmp == addr)
return true;
- tmp += GET_INSN_LENGTH(*(kprobe_opcode_t *)tmp);
+ memcpy(&opcode, (void *)tmp, sizeof(kprobe_opcode_t));
+ tmp += GET_INSN_LENGTH(opcode);
}
return false;
@@ -80,7 +82,7 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
return -EILSEQ;
/* copy instruction */
- p->opcode = *p->addr;
+ memcpy(&p->opcode, p->addr, sizeof(kprobe_opcode_t));
/* decode instruction */
switch (riscv_probe_decode_insn(p->addr, &p->ainsn.api)) {
>
>
> Björn
--
Best Regards
Guo Ren
在 2023/1/31 20:32, Björn Töpel 写道:
> [email protected] writes:
>
>> From: Guo Ren <[email protected]>
>>
>> The kernel would panic when probed for an illegal position. eg:
>>
>> (CONFIG_RISCV_ISA_C=n)
>>
>> echo 'p:hello kernel_clone+0x16 a0=%a0' >> kprobe_events
>> echo 1 > events/kprobes/hello/enable
>> cat trace
>>
>> Kernel panic - not syncing: stack-protector: Kernel stack
>> is corrupted in: __do_sys_newfstatat+0xb8/0xb8
>> CPU: 0 PID: 111 Comm: sh Not tainted
>> 6.2.0-rc1-00027-g2d398fe49a4d #490
>> Hardware name: riscv-virtio,qemu (DT)
>> Call Trace:
>> [<ffffffff80007268>] dump_backtrace+0x38/0x48
>> [<ffffffff80c5e83c>] show_stack+0x50/0x68
>> [<ffffffff80c6da28>] dump_stack_lvl+0x60/0x84
>> [<ffffffff80c6da6c>] dump_stack+0x20/0x30
>> [<ffffffff80c5ecf4>] panic+0x160/0x374
>> [<ffffffff80c6db94>] generic_handle_arch_irq+0x0/0xa8
>> [<ffffffff802deeb0>] sys_newstat+0x0/0x30
>> [<ffffffff800158c0>] sys_clone+0x20/0x30
>> [<ffffffff800039e8>] ret_from_syscall+0x0/0x4
>> ---[ end Kernel panic - not syncing: stack-protector:
>> Kernel stack is corrupted in: __do_sys_newfstatat+0xb8/0xb8 ]---
>>
>> That is because the kprobe's ebreak instruction broke the kernel's
>> original code. The user should guarantee the correction of the probe
>> position, but it couldn't make the kernel panic.
>>
>> This patch adds arch_check_kprobe in arch_prepare_kprobe to prevent an
>> illegal position (Such as the middle of an instruction).
>
> Nice!
>
> @liaochang Will you remove your patch from the OPTPROBE series?
Sure, i will remove it.
>
>> Fixes: c22b0bcb1dd0 ("riscv: Add kprobes supported")
>> Signed-off-by: Guo Ren <[email protected]>
>> Signed-off-by: Guo Ren <[email protected]>
>> ---
>> arch/riscv/kernel/probes/kprobes.c | 18 ++++++++++++++++++
>> 1 file changed, 18 insertions(+)
>>
>> diff --git a/arch/riscv/kernel/probes/kprobes.c b/arch/riscv/kernel/probes/kprobes.c
>> index f21592d20306..475989f06d6d 100644
>> --- a/arch/riscv/kernel/probes/kprobes.c
>> +++ b/arch/riscv/kernel/probes/kprobes.c
>> @@ -48,6 +48,21 @@ static void __kprobes arch_simulate_insn(struct kprobe *p, struct pt_regs *regs)
>> post_kprobe_handler(p, kcb, regs);
>> }
>>
>> +static bool __kprobes arch_check_kprobe(struct kprobe *p)
>> +{
>> + unsigned long tmp = (unsigned long)p->addr - p->offset;
>> + unsigned long addr = (unsigned long)p->addr;
>> +
>> + while (tmp <= addr) {
>> + if (tmp == addr)
>> + return true;
>> +
>> + tmp += GET_INSN_LENGTH(*(kprobe_opcode_t *)tmp);
>
> kprobe_opcode_t is u32; This can trigger a misaligned load, right?
I think it depends on the hardware implementation, event an EEI may guarantee that misaligned
loads and stores are fully supported, hardware will requires additional sychronizatin to ensure
atomicity, so it is better to use a load instruction whose effective address is naturally aligned.
From "Volum I: RISC-V Unprivileged ISA 2.6":
"..., Loads and stores whose effective address is not naturally aligned to the referenced datatype
(i.e, the effective address is not divisible by the size of the access in bytes) have behavior
dependenet on the EEI."
>
>> + }
>> +
>> + return false;
>> +}
>> +
>> int __kprobes arch_prepare_kprobe(struct kprobe *p)
>> {
>> unsigned long probe_addr = (unsigned long)p->addr;
>> @@ -55,6 +70,9 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
>> if (probe_addr & 0x1)
>> return -EILSEQ;
>>
>> + if (!arch_check_kprobe(p))
>> + return -EILSEQ;
>> +
>> /* copy instruction */
>> p->opcode = *p->addr;
>
> Not related to your patch, but this can also trigger a misaligned load.
>
>
> Björn
--
BR,
Liao, Chang
Guo Ren <[email protected]> writes:
>> > + if (!arch_check_kprobe(p))
>> > + return -EILSEQ;
>> > +
>> > /* copy instruction */
>> > p->opcode = *p->addr;
>>
>> Not related to your patch, but this can also trigger a misaligned load.
> After rereading the spec, misaligned load/store is not mandatory
> supported. (Although my machines and qemu are correct)
Yes. It would be great if it could be turned on by QEMU (you can with
Spike).
> So I need fixup:
>
> diff --git a/arch/riscv/kernel/probes/kprobes.c
> b/arch/riscv/kernel/probes/kprobes.c
> index 27f8960c321c..0c016d496746 100644
> --- a/arch/riscv/kernel/probes/kprobes.c
> +++ b/arch/riscv/kernel/probes/kprobes.c
> @@ -58,12 +58,14 @@ static bool __kprobes arch_check_kprobe(struct kprobe *p)
> {
> unsigned long tmp = (unsigned long)p->addr - p->offset;
> unsigned long addr = (unsigned long)p->addr;
> + kprobe_opcode_t opcode;
>
> while (tmp <= addr) {
> if (tmp == addr)
> return true;
>
> - tmp += GET_INSN_LENGTH(*(kprobe_opcode_t *)tmp);
> + memcpy(&opcode, (void *)tmp, sizeof(kprobe_opcode_t));
> + tmp += GET_INSN_LENGTH(opcode);
I'd prefer sizeof(opcode).
> }
>
> return false;
> @@ -80,7 +82,7 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
> return -EILSEQ;
>
> /* copy instruction */
> - p->opcode = *p->addr;
> + memcpy(&p->opcode, p->addr, sizeof(kprobe_opcode_t));
Same comment as above, and this would ideally be a separate patch.
Björn
On Wed, Feb 1, 2023 at 5:30 PM Björn Töpel <[email protected]> wrote:
>
> Guo Ren <[email protected]> writes:
>
> >> > + if (!arch_check_kprobe(p))
> >> > + return -EILSEQ;
> >> > +
> >> > /* copy instruction */
> >> > p->opcode = *p->addr;
> >>
> >> Not related to your patch, but this can also trigger a misaligned load.
> > After rereading the spec, misaligned load/store is not mandatory
> > supported. (Although my machines and qemu are correct)
>
> Yes. It would be great if it could be turned on by QEMU (you can with
> Spike).
>
> > So I need fixup:
> >
> > diff --git a/arch/riscv/kernel/probes/kprobes.c
> > b/arch/riscv/kernel/probes/kprobes.c
> > index 27f8960c321c..0c016d496746 100644
> > --- a/arch/riscv/kernel/probes/kprobes.c
> > +++ b/arch/riscv/kernel/probes/kprobes.c
> > @@ -58,12 +58,14 @@ static bool __kprobes arch_check_kprobe(struct kprobe *p)
> > {
> > unsigned long tmp = (unsigned long)p->addr - p->offset;
> > unsigned long addr = (unsigned long)p->addr;
> > + kprobe_opcode_t opcode;
> >
> > while (tmp <= addr) {
> > if (tmp == addr)
> > return true;
> >
> > - tmp += GET_INSN_LENGTH(*(kprobe_opcode_t *)tmp);
> > + memcpy(&opcode, (void *)tmp, sizeof(kprobe_opcode_t));
> > + tmp += GET_INSN_LENGTH(opcode);
>
> I'd prefer sizeof(opcode).
prefer sizeof(opcode) = 4
GET_INSN_LENGTH(opcode) returns 2 or 4;
>
> > }
> >
> > return false;
> > @@ -80,7 +82,7 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
> > return -EILSEQ;
> >
> > /* copy instruction */
> > - p->opcode = *p->addr;
> > + memcpy(&p->opcode, p->addr, sizeof(kprobe_opcode_t));
>
> Same comment as above, and this would ideally be a separate patch.
Yes
>
>
> Björn
--
Best Regards
Guo Ren
Guo Ren <[email protected]> writes:
>> > - tmp += GET_INSN_LENGTH(*(kprobe_opcode_t *)tmp);
>> > + memcpy(&opcode, (void *)tmp, sizeof(kprobe_opcode_t));
>> > + tmp += GET_INSN_LENGTH(opcode);
>>
>> I'd prefer sizeof(opcode).
> prefer sizeof(opcode) = 4
>
> GET_INSN_LENGTH(opcode) returns 2 or 4;
Sorry for being unclear. I was referring to the sizeof in the memcpy.