Re: [RFC PATCH bpf-next 4/7] bpf: Add kfuncs with bodies for common operations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] bpf: bpf_extract64 lacks bounds checking, leading to UB and incorrect results
--

commit 585ec9b342241a745cc98fb7ad012ebceacf697c
Author: Yusheng Zheng <yunwei356@gmail.com>

bpf: Add kfuncs with bodies for common operations

This commit adds kfuncs with bodies for common operation families, including
bpf_extract64 for bit field extraction. They are placed in
kernel/bpf/insn_kfuncs/ and can be inlined by the x86-64 JIT.

[1] https://arxiv.org/abs/2606.24213

> diff --git a/kernel/bpf/insn_kfuncs/bpf_insn_kfuncs.c b/kernel/bpf/insn_kfuncs/bpf_insn_kfuncs.c
> new file mode 100644
> --- /dev/null
> +++ b/kernel/bpf/insn_kfuncs/bpf_insn_kfuncs.c

[ ... ]

> +__bpf_kfunc u64 bpf_select64(u64 cond, u64 a, u64 b)
> +{
> +	return cond ? a : b;
> +}
> +
> +__bpf_kfunc u64 bpf_extract64(u64 x, u32 start__k, u32 len__k)
> +{
> +	return x << (64 - start__k - len__k) >> (64 - len__k);
> +}

[Severity: Medium]
Will this shift operation cause undefined behavior when len__k is 0 or when
start__k + len__k is greater than 64?

In C, shifting a 64-bit value by 64 or more is undefined behavior. For
example, when len__k is 0, 64 - len__k evaluates to 64. Even when this is
masked by the architecture or BPF semantics, a zero-length extract might
incorrectly return the unmasked value instead of 0, and out-of-bounds
extracts could return garbage.

> +
> +__bpf_kfunc u64 bpf_load_be64(const void *p, s32 off__k)
> +{

[ ... ]

> +/* the jump lands within the body, here on its last instruction */
> +static const struct bpf_insn select64_body[] = {
> +	BPF_JMP_IMM(BPF_JNE, BPF_REG_1, 0, 1),
> +	BPF_MOV64_REG(BPF_REG_2, BPF_REG_3),
> +	BPF_MOV64_REG(BPF_REG_0, BPF_REG_2),
> +};
> +
> +/* x << (64 - start - len) >> (64 - len) */
> +static const struct bpf_insn extract64_body[] = {
> +	BPF_MOV32_IMM(BPF_REG_4, 64),
> +	BPF_ALU32_REG(BPF_SUB, BPF_REG_4, BPF_REG_2),
> +	BPF_ALU32_REG(BPF_SUB, BPF_REG_4, BPF_REG_3),
> +	BPF_MOV64_REG(BPF_REG_0, BPF_REG_1),
> +	BPF_ALU64_REG(BPF_LSH, BPF_REG_0, BPF_REG_4),
> +	BPF_MOV32_IMM(BPF_REG_4, 64),
> +	BPF_ALU32_REG(BPF_SUB, BPF_REG_4, BPF_REG_3),
> +	BPF_ALU64_REG(BPF_RSH, BPF_REG_0, BPF_REG_4),
> +};

[Severity: Medium]
Does this BPF body also lack bounds checking for the extraction lengths?

If a BPF program calls bpf_extract64 with invalid arguments and falls back
to these BPF instructions, this logic mimics the C code above. This appears
to mask a shift of 64 down to 0, which would return an incorrect non-zero
value for 0-length extracts instead of 0.

> +
> +/* the offset is an s32 */
> +static const struct bpf_insn load_be64_body[] = {

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