Re: [PATCH v2 06/16] riscv: cfi: Use generated instruction headers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Critical] Incorrect target address calculation for auipc+jalr pairs during alternative patching.

Pre-existing issues:
- [High] KCFI failure handler extracts the actual type instead of the expected type, leading to a misleading log and a potential arbitrary kernel memory leak.
--

commit fc627870b9c3f0a09b99b862169f28fa7f3bcda2
Author: Charlie Jenkins <thecharlesjenkins@gmail.com>

riscv: cfi: Use generated instruction headers
    
Migrate the code that is decoding cfi instructions to use the generated
instruction headers. This is part of a broader series that updates
instruction decoding and patching across alternative, kprobe, kgdb, and
cfi subsystems to use the newly generated instruction tables.

> diff --git a/arch/riscv/kernel/alternative.c b/arch/riscv/kernel/alternative.c
> index 7642704c7f184..b26a90eb65cca 100644
> --- a/arch/riscv/kernel/alternative.c
> +++ b/arch/riscv/kernel/alternative.c
[ ... ]
> @@ -78,14 +79,24 @@ static void riscv_alternative_fix_auipc_jalr(void *ptr, u32 auipc_insn,
>  					     u32 jalr_insn, int patch_offset)
>  {
>  	u32 call[2] = { auipc_insn, jalr_insn };
> +	u32 auipc_imm;
>  	s32 imm;
>  
>  	/* get and adjust new target address */
> -	imm = riscv_insn_extract_utype_itype_imm(auipc_insn, jalr_insn);
> +	imm = riscv_insn_auipc_extract_imm(auipc_insn) + riscv_insn_jalr_extract_imm(jalr_insn);
>  	imm -= patch_offset;
>  
> +	/*
> +	 * When the 32-bit immediate is split across auipc and jalr, the
> +	 * constructed immediates need to be treated as individually sign
> +	 * extended numbers. Add the sign bit of the lower 12 bits to the upper
> +	 * 20 bits to undo the bleeding of the sign.
> +	 */
> +	auipc_imm = imm + (BIT(11) << 1);

[Severity: Critical]
Does this calculation unconditionally add 4096 to the immediate?

Since BIT(11) << 1 evaluates to a constant 4096 (0x1000), it looks like
this regression adds 4096 regardless of whether bit 11 is actually set
in the immediate.

For any jump where bit 11 of the offset is 0, won't the auipc instruction
be patched with a value that is 0x1000 too large, causing the execution to
jump to an unintended address?

> +
>  	/* update instructions */
> -	riscv_insn_insert_utype_itype_imm(&call[0], &call[1], imm);
> +	riscv_insn_auipc_insert_imm(&call[0], auipc_imm);
> +	riscv_insn_jalr_insert_imm(&call[1], imm);
>  
>  	/* patch the call place again */
>  	patch_text_nosync(ptr, call, sizeof(u32) * 2);

> diff --git a/arch/riscv/kernel/cfi.c b/arch/riscv/kernel/cfi.c
> index 6ec9dbd7292ee..e38d5f863747a 100644
> --- a/arch/riscv/kernel/cfi.c
> +++ b/arch/riscv/kernel/cfi.c
> @@ -40,16 +40,16 @@ static bool decode_cfi_insn(struct pt_regs *regs, unsigned long *target,
>  	if (!riscv_insn_is_beq(insn))
>  		return false;
>  
> -	*type = (u32)regs_ptr[RV_EXTRACT_RS1_REG(insn)];
> +	*type = (u32)regs_ptr[riscv_insn_beq_extract_xs1(insn)];

[Severity: High]
This is a pre-existing issue, but does this extract the actual type
instead of the expected type?

The compiler sequence for indirect call checks places the actual type in
t1 (rs1) and the expected type in t2 (rs2). By extracting xs1 here, the
KCFI failure handler appears to retrieve the actual type loaded from memory.

Since the actual type is read from -4(<reg>) where <reg> could be an
attacker-controlled function pointer, could this allow leaking arbitrary
kernel memory if CONFIG_CFI_PERMISSIVE is enabled and the warning is printed
to dmesg?

Should this extract xs2 instead to get the expected type?

>  
>  	if (get_kernel_nofault(insn, (void *)regs->epc) ||
>  	    get_kernel_nofault(insn, (void *)regs->epc + GET_INSN_LENGTH(insn)))
>  		return false;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260621-riscv_insn_table-v2-0-7f0810ee2274@gmail.com?part=6