linux-kernel - Re: [PATCH] riscv: kprobes: Fix incorrect address calculation

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] [day] [month] [year] [list]

Message-ID: <7662be43-16e6-4695-9bc4-077e1dd3ba12@ghiti.fr>
Date: Thu, 28 Nov 2024 14:02:40 +0100
From: Alexandre Ghiti <alex@...ti.fr>
To: Nam Cao <namcao@...utronix.de>, Paul Walmsley <paul.walmsley@...ive.com>,
 Palmer Dabbelt <palmer@...belt.com>, Albert Ou <aou@...s.berkeley.edu>,
 Samuel Holland <samuel.holland@...ive.com>, Björn Töpel
 <bjorn@...osinc.com>, linux-riscv@...ts.infradead.org,
 linux-kernel@...r.kernel.org
Cc: John Ogness <john.ogness@...utronix.de>, stable@...r.kernel.org
Subject: Re: [PATCH] riscv: kprobes: Fix incorrect address calculation

Hi Nam,

On 19/11/2024 12:10, Nam Cao wrote:
> p->ainsn.api.insn is a pointer to u32, therefore arithmetic operations are
> multiplied by four. This is clearly undesirable for this case.
>
> Cast it to (void *) first before any calculation.
>
> Below is a sample before/after. The dumped memory is two kprobe slots, the
> first slot has
>
>    - c.addiw a0, 0x1c (0x7125)
>    - ebreak           (0x00100073)
>
> and the second slot has:
>
>    - c.addiw a0, -4   (0x7135)
>    - ebreak           (0x00100073)
>
> Before this patch:
>
> (gdb) x/16xh 0xff20000000135000
> 0xff20000000135000:	0x7125	0x0000	0x0000	0x0000	0x7135	0x0010	0x0000	0x0000
> 0xff20000000135010:	0x0073	0x0010	0x0000	0x0000	0x0000	0x0000	0x0000	0x0000
>
> After this patch:
>
> (gdb) x/16xh 0xff20000000125000
> 0xff20000000125000:	0x7125	0x0073	0x0010	0x0000	0x7135	0x0073	0x0010	0x0000
> 0xff20000000125010:	0x0000	0x0000	0x0000	0x0000	0x0000	0x0000	0x0000	0x0000
>
> Fixes: b1756750a397 ("riscv: kprobes: Use patch_text_nosync() for insn slots")
> Signed-off-by: Nam Cao <namcao@...utronix.de>
> Cc: stable@...r.kernel.org
> ---
>   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 474a65213657..d2dacea1aedd 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)

This looks good to me, how did you find this issue?

You can add:

Reviewed-by: Alexandre Ghiti <alexghiti@...osinc.com>

Thanks,

Alex