Re: [PATCH bpf 1/1] xsk: use a 32-bit compare in xsk_map_gen_lookup

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The patch addresses a 64-bit sign-extension bounds check bypass in `xsk_map_gen_lookup` by replacing `BPF_JMP_IMM` with `BPF_JMP32_IMM`, but fails to apply the exact same necessary fix to identical vulnerable patterns in `kernel/bpf/arraymap.c` and `kernel/bpf/fixups.c`. `BPF_JMP_IMM` sign-extends its 32-bit immediate argument to 64 bits before performing the comparison. A `max_entries` value >= `0x80000000` becomes a large negative number in a 64-bit context, meaning the unsigned `BPF_JGE` check against a zero-extended 32-bit index will never jump, skipping the bounds check entirely.
--

commit c7773394d23ffdacd886a0d61cfe827025fc2bfb
Author: Zhiling Zou <zhilinz@nebusec.ai>

xsk: use a 32-bit compare in xsk_map_gen_lookup

This commit fixes a bounds check bypass in xsk_map_gen_lookup() by
switching to a 32-bit compare for the max_entries check. BPF immediates
are sign-extended to 64 bits, which previously caused large max_entries
values to become large negative numbers in a 64-bit context, bypassing
the bounds check.

Cc: stable@vger.kernel.org

> diff --git a/net/xdp/xskmap.c b/net/xdp/xskmap.c
> index 3bff346308d0f..bf00d6463c191 100644
> --- a/net/xdp/xskmap.c
> +++ b/net/xdp/xskmap.c
> @@ -124,7 +124,7 @@ static int xsk_map_gen_lookup(struct bpf_map *map, struct bpf_insn *insn_buf)
>  	struct bpf_insn *insn = insn_buf;
>  
>  	*insn++ = BPF_LDX_MEM(BPF_W, ret, index, 0);
> -	*insn++ = BPF_JMP_IMM(BPF_JGE, ret, map->max_entries, 5);
> +	*insn++ = BPF_JMP32_IMM(BPF_JGE, ret, map->max_entries, 5);

[Severity: High]
This is a pre-existing issue, but while you are fixing this sign-extension
bounds check bypass here, should the exact same fix be applied to identical
vulnerable patterns in kernel/bpf/arraymap.c and kernel/bpf/fixups.c?

For example, in kernel/bpf/arraymap.c:array_map_gen_lookup():

	*insn++ = BPF_JMP_IMM(BPF_JGE, ret, map->max_entries, 4);
	*insn++ = BPF_ALU32_IMM(BPF_AND, ret, array->index_mask);
} else {
	*insn++ = BPF_JMP_IMM(BPF_JGE, ret, map->max_entries, 3);
}

The same pattern exists in kernel/bpf/arraymap.c:percpu_array_map_gen_lookup():

	*insn++ = BPF_JMP_IMM(BPF_JGE, BPF_REG_0, map->max_entries, 6);
	*insn++ = BPF_ALU32_IMM(BPF_AND, BPF_REG_0, array->index_mask);
} else {
	*insn++ = BPF_JMP_IMM(BPF_JGE, BPF_REG_0, map->max_entries, 5);
}

And also in kernel/bpf/arraymap.c:array_of_map_gen_lookup() as well as
kernel/bpf/fixups.c:fixup_bpf_calls() for tail calls.

Since BPF_JMP_IMM sign-extends its 32-bit immediate argument to 64 bits,
a max_entries value >= 0x80000000 becomes a large negative number in a
64-bit context. 

This means the unsigned BPF_JGE check against a zero-extended 32-bit index 
will never jump, skipping the bounds check entirely. For array maps with 
bypass_spec_v1 == true, does this completely bypass memory bounds checks?

Similarly, for tail calls in fixup_bpf_calls(), does this bypass the
architectural bounds check and defeat the speculative execution mitigation
since the bound check branch is never taken for out-of-bound indices?

>  	*insn++ = BPF_ALU64_IMM(BPF_LSH, ret, ilog2(sizeof(struct xsk_sock *)));
>  	*insn++ = BPF_ALU64_IMM(BPF_ADD, mp, offsetof(struct xsk_map, xsk_map));
>  	*insn++ = BPF_ALU64_REG(BPF_ADD, ret, mp);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/7d2cb8e8dfaa9eb8fdff85156987a60960787dc3.1789056660.git.zhilinz@nebusec.ai?part=1