Re: [PATCH] bpf: disasm: guard print_bpf_insn against BPF_MEMSX | BPF_DW

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The patch adds a redundant check for `BPF_MEMSX` with `BPF_DW` size that is unreachable dead code, because the preceding `if` statement already checks the exact same condition and returns.
--

commit ae7a2255ead6c3e59e5776083e563c01d78b10e7
Author: Utku Erol <utkuerol71@gmail.com>

bpf: disasm: guard print_bpf_insn against BPF_MEMSX | BPF_DW

This patch adds a check in print_bpf_insn() to reject BPF_MEMSX instructions
with BPF_DW size. It intends to prevent an out-of-bounds array access that
occurs on unvalidated instructions during disassembly.

[ ... ]

> The LDX branch checks only that the mode is
> BPF_MEM or BPF_MEMSX; it never rejects BPF_MEMSX | BPF_DW. For that opcode

[Severity: Medium]
Is this description accurate?

Looking at print_bpf_insn(), the preceding check in the baseline code already
explicitly evaluates and rejects the (BPF_MEMSX | BPF_DW) condition.

[ ... ]

> diff --git a/kernel/bpf/disasm.c b/kernel/bpf/disasm.c
> index b1a3fbe3fda53..7c24136268026 100644
> --- a/kernel/bpf/disasm.c
> +++ b/kernel/bpf/disasm.c
> @@ -300,6 +300,10 @@ void print_bpf_insn(const struct bpf_insn_cbs *cbs,
>  			verbose(cbs->private_data, "BUG_ldx_%02x", insn->code);
>  			return;
>  		}
> +		if (BPF_MODE(insn->code) == BPF_MEMSX && BPF_SIZE(insn->code) == BPF_DW) {

[Severity: Medium]
Does this code ever execute?

The immediately preceding if statement checks the exact same condition:

	} else if (class == BPF_LDX) {
		if ((BPF_MODE(insn->code) != BPF_MEM && BPF_MODE(insn->code) != BPF_MEMSX) ||
		    (BPF_MODE(insn->code) == BPF_MEMSX && BPF_SIZE(insn->code) == BPF_DW)) {
			verbose(cbs->private_data, "BUG_ldx_%02x", insn->code);
			return;
		}

Since the previous check unconditionally returns, won't this newly added block
be completely unreachable dead code?

> +			verbose(cbs->private_data, "BUG_ldsx_%02x", insn->code);
> +			return;
> +		}
>  		verbose(cbs->private_data, "(%02x) r%d = *(%s *)(r%d %+d)",
>  			insn->code, insn->dst_reg,
>  			BPF_MODE(insn->code) == BPF_MEM ?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903133434.17860-1-utkuerol71@gmail.com?part=1