Re: [PATCH v6 1/2] riscv: kprobes: reject probes inside LR/SC sequences

[Nam Cao]

Xiaofeng Yuan <yuanxiaofeng@xxxxxxxxxxxxxxxxxx> writes:
> +static __always_inline bool riscv_insn_is_lr(u32 code)
> +{
> +	u32 funct5 = (code >> 27) & 0x1f;
> +	u32 funct3 = (code >> 12) & 0x7;
> +
> +	return (code & RV_INSN_OPCODE_MASK) == RVG_OPCODE_AMO &&
> +	       funct5 == RVG_AMO_FUNCT5_LR &&
> +	       (funct3 == RVG_FUNCT3_AMO_W || funct3 == RVG_FUNCT3_AMO_D) &&
> +	       !(code & GENMASK(24, 20));
> +}
> +
> +static __always_inline bool riscv_insn_is_sc(u32 code)
> +{
> +	u32 funct5 = (code >> 27) & 0x1f;
> +	u32 funct3 = (code >> 12) & 0x7;
> +
> +	return (code & RV_INSN_OPCODE_MASK) == RVG_OPCODE_AMO &&
> +	       funct5 == RVG_AMO_FUNCT5_SC &&
> +	       (funct3 == RVG_FUNCT3_AMO_W || funct3 == RVG_FUNCT3_AMO_D);
> +}

There are already RV_EXTRACT_FUNCT3() and RV_EXTRACT_RS2_REG(), we
should use them to avoid code duplications.

And I think we should also have RV_EXTRACT_FUNCT5() to stay consistent.

> +static int __kprobes arch_check_kprobe(unsigned long addr)
>  {
> -	unsigned long tmp, offset;
> +	unsigned long pc, offset;
> +	int outstanding_lr = -1;
>  
> -	/* start iterating at the closest preceding symbol */
>  	if (!kallsyms_lookup_size_offset(addr, NULL, &offset))
> -		return false;
> +		return -EILSEQ;
>  
> -	tmp = addr - offset;
> +	/* pc starts at the closest preceding symbol, a known instruction boundary */
> +	pc = addr - offset;
>  
> -	while (tmp <= addr) {
> -		if (tmp == addr)
> -			return true;
> +	while (pc < addr) {
> +		u32 insn = get_unaligned((u32 *)pc);
> +		unsigned int len = GET_INSN_LENGTH(insn);
>  
> -		tmp += GET_INSN_LENGTH(*(u16 *)tmp);
> +		if (riscv_insn_is_lr(insn))
> +			outstanding_lr = 0;
> +		else if (riscv_insn_is_sc(insn))
> +			outstanding_lr = -1;
> +		else if (outstanding_lr >= 0 && ++outstanding_lr > MAX_LRSC_GAP)
> +			outstanding_lr = -1;
> +
> +		pc += len;
>  	}

The spec says "The loop comprises only an LR/SC sequence and code to
retry the sequence in the case of failure, and must comprise at most 16
instructions placed sequentially in memory". The 16 instructions limit
includes LR. So I think this implementation is off by one:

    lr     (outstanding_lr=0)
    inst1  (outstanding_lr=1)
    inst2  (outstanding_lr=2)
    ...
    inst16 (outstanding_lr=16)
    inst17 (outstanding_lr=-1)

As it is considering instruction from LR to inst16 to be a loop, so it
allows 17 instructions.

Nam