Re: [PATCH bpf-next] bpf: fix symmetric register-form NULL-pointer check in check_cond_jmp_op()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] bpf: symmetric NULL check fix is unreachable for unprivileged programs
- [Low] bpf: multi-line comments violate subsystem style guidelines
--

commit e76939eba1acc9b00715cb7f09c40f2d50cb02ae
Author: Rahad Bhuiya <rahadbhuiya2021@gmail.com>

bpf: fix symmetric register-form NULL-pointer check in check_cond_jmp_op()
    
This commit addresses an issue in the BPF verifier where symmetric NULL pointer
checks (e.g., if (0 == ptr)) were not propagating the nullness state correctly.
It adds logic to handle the case where the destination register is zero and
the source register is a nullable pointer, updating the branches accordingly.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 353bde9ae2274..e4ae8f6481620 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c

[ ... ]

> @@ -18248,6 +18248,26 @@ static int check_cond_jmp_op(struct bpf_verifier_env *env,
>  				      opcode == BPF_JNE);
>  		mark_ptr_or_null_regs(other_branch, insn->dst_reg,
>  				      opcode == BPF_JEQ);
> +	} else if (!is_jmp32 && BPF_SRC(insn->code) == BPF_X &&
> +		   (opcode == BPF_JEQ || opcode == BPF_JNE) &&
> +		   type_may_be_null(src_reg->type) &&
> +		   bpf_register_is_null(dst_reg)) {

[Severity: Medium]
Will this fix be reachable for unprivileged BPF programs?

In unprivileged mode (env->allow_ptr_leaks == false), an early security check
in check_cond_jmp_op() rejects conditional jumps where src_reg is a pointer,
unless both operands are packet pointers:

    if (!(reg_is_pkt_pointer_any(dst_reg) && reg_is_pkt_pointer_any(src_reg)) &&
        is_pointer_value(env, insn->src_reg)) {
        verbose(env, "R%d pointer comparison prohibited\n", insn->src_reg);
        return -EACCES;
    }

Because this check happens before the newly added block, unprivileged BPF
programs attempting symmetric NULL checks might still be unconditionally
rejected, rendering the fix incomplete for those users.

> +		/* Symmetric case: src_reg is the nullable pointer, dst_reg is
> +		 * a register whose value is known to be zero.  This arises when

[Severity: Low]
This isn't a bug, but does this multi-line comment violate the BPF subsystem
style guidelines?

The BPF guidelines mandate that multi-line comments must have the opening /*
on its own line, with the comment text starting on the subsequent line.

> +		 * the programmer writes the comparison in reversed operand order,
> +		 * e.g. "if (0 == map_val)" or "if (r0 == map_val)" where r0 has
> +		 * been proven zero by the verifier.
> +		 *
> +		 * The zero is a property of this execution path, so dst_reg
> +		 * must be marked precise before we propagate nullness.
> +		 */
> +		err = mark_chain_precision(env, insn->dst_reg);
> +		if (err)
> +			return err;

[ ... ]

> diff --git a/tools/testing/selftests/bpf/progs/verifier_null_ptr_symmetric.c b/tools/testing/selftests/bpf/progs/verifier_null_ptr_symmetric.c
> new file mode 100644
> index 0000000000000..475aa3dc3bd25
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/verifier_null_ptr_symmetric.c
> @@ -0,0 +1,162 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Verifier selftests for symmetric register-form NULL-pointer checks.
> + *
> + * The BPF verifier's check_cond_jmp_op() handles "if R == 0" NULL checks by

[Severity: Low]
This isn't a bug, but does this comment also violate the multi-line comment
style for the BPF subsystem?

The opening /* should be on its own line, with the text beginning on the
subsequent line.

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