Re: [PATCH bpf-next 2/7] arm64: insn: Add encoder for ADD/SUB (extended register)

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The encoder for ADD/SUB (extended register) fails to reject `UXTX` and `SXTX` extensions when generating a 32-bit variant, which results in an UNALLOCATED instruction.
--

--- Patch [2]: [PATCH bpf-next 2/7] arm64: insn: Add encoder for ADD/SUB (extended register) ---
commit 286e3f9944297bcaa48fe3c90eb0bde9f1e85a33
Author: Tejun Heo <[email protected]>

arm64: insn: Add encoder for ADD/SUB (extended register)

This commit adds the aarch64_insn_gen_add_sub_extended_reg() function to the
insn library to support encoding the extended-register form of ADD and SUB
instructions. It is modeled on the shifted-register generator and uses the
existing IMM_6 field for the option and imm3 fields.

> diff --git a/arch/arm64/lib/insn.c b/arch/arm64/lib/insn.c
> index 37ce75f7f1f08..e70ac02385153 100644
> --- a/arch/arm64/lib/insn.c
> +++ b/arch/arm64/lib/insn.c

[ ... ]

> +u32 aarch64_insn_gen_add_sub_extended_reg(enum aarch64_insn_register dst,
> +					  enum aarch64_insn_register src,
> +					  enum aarch64_insn_register reg,
> +					  enum aarch64_insn_extend_type extend,
> +					  int shift,
> +					  enum aarch64_insn_variant variant,
> +					  enum aarch64_insn_adsb_type type)
> +{

[ ... ]

> +	switch (variant) {
> +	case AARCH64_INSN_VARIANT_32BIT:
> +		break;

[Severity: Medium]
Should this case validate that the extend parameter is not UXTX or SXTX?

The ARM Architecture Reference Manual indicates that for 32-bit extended
register operations (sf == 0), the option field values 011 (UXTX) and
111 (SXTX) are UNALLOCATED and lead to an UNDEFINED exception at execution
time.

Adding a bounds check here to return AARCH64_BREAK_FAULT would mirror the
existing 32-bit validation checks in aarch64_insn_gen_add_sub_shifted_reg()
and aarch64_insn_gen_data1().

> +	case AARCH64_INSN_VARIANT_64BIT:
> +		insn |= AARCH64_INSN_SF_BIT;
> +		break;
> +	default:
> +		pr_err("%s: unknown variant encoding %d\n", __func__, variant);
> +		return AARCH64_BREAK_FAULT;
> +	}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2