Re: [PATCH v2] bpf: fix NULL pointer dereference in print_reg_state()

[KaFai Wan <kafai.wan@linux.dev>]

On Wed, 2025-09-24 at 23:58 +0530, Brahmajit Das wrote:
> On 25.09.2025 01:38, KaFai Wan wrote:
> > On Wed, 2025-09-24 at 21:10 +0530, Brahmajit Das wrote:
> > > On 24.09.2025 09:32, Alexei Starovoitov wrote:
> > > > On Wed, Sep 24, 2025 at 1:43 AM Brahmajit Das
> > > > <listout@listout.xyz>
> > > > wrote:
> > > > > 
> > > > > Syzkaller reported a general protection fault due to a NULL
> > > > > pointer
> > > > > dereference in print_reg_state() when accessing reg->map_ptr
> > > > > without
> > > > > checking if it is NULL.
> > > > > 
> > > ...snip...
> > > > > -       if (type_is_map_ptr(t)) {
> > > > > +       if (type_is_map_ptr(t) && reg->map_ptr) {
> > > > 
> > > > You ignored earlier feedback.
> > > > Fix the root cause, not the symptom.
> > > > 
> > > > pw-bot: cr
> > > 
> > > I'm not sure if I'm headed the write direction but it seems like
> > > in
> > > check_alu_op, we are calling adjust_scalar_min_max_vals when we
> > > get
> > > an
> > > BPF_NEG as opcode. Which has a call to __mark_reg_known when
> > > opcode
> > > is
> > > BPF_NEG. And __mark_reg_known clears map_ptr with
> > > 
> > > 	/* Clear off and union(map_ptr, range) */
> > > 	memset(((u8 *)reg) + sizeof(reg->type), 0,
> > > 	       offsetof(struct bpf_reg_state, var_off) -
> > > sizeof(reg-
> > > > type));
> > > 
> > 
> > I think you are right. The following code can reproduce the error.
> > 
> > 	asm volatile ("					\
> > 	r0 = %[map_hash_48b] ll;			\
> > 	r0 = -r0;					\
> > 	exit;						\
> > "	:
> > 	: __imm_addr(map_hash_48b)
> > 	: __clobber_all);
> > 
> > 
> > BPF_NEG calls __mark_reg_known(dst_reg, 0) which clears the 'off'
> > and
> > 'union(map_ptr, range)' of dst_reg, but keeps the 'type', which is
> > CONST_PTR_TO_MAP.
> > 
> > Perhaps we can only allow the SCALAR_VALUE type to run BPF_NEG as
> > an
> > opcode, while for other types same as the before BPF_NEG.
> > 
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index e892df386eed..dbf9f1efc6e7 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -15346,13 +15346,15 @@ static bool
> > is_safe_to_compute_dst_reg_range(struct bpf_insn *insn,
> >  	switch (BPF_OP(insn->code)) {
> >  	case BPF_ADD:
> >  	case BPF_SUB:
> > -	case BPF_NEG:
> >  	case BPF_AND:
> >  	case BPF_XOR:
> >  	case BPF_OR:
> >  	case BPF_MUL:
> >  		return true;
> >  
> > +	case BPF_NEG:
> > +		return base_type(src_reg->type) == SCALAR_VALUE;
> > +
> > 
> > 
> > -- 
> > Thanks,
> > KaFai
> 
> Before even going into adjust_scalar_min_max_vals we have a check in
> check_alu_op, which I think is not being respected. Going to expand
> on
> this below as response to Alexei.
> 
> On 24.09.2025 18:28, Alexei Starovoitov wrote:
> > On Wed, Sep 24, 2025 at 4:41 PM Brahmajit Das <listout@listout.xyz>
> > wrote:
> > > 
> > > On 24.09.2025 09:32, Alexei Starovoitov wrote:
> > > > On Wed, Sep 24, 2025 at 1:43 AM Brahmajit Das
> > > > <listout@listout.xyz> wrote:
> > > > > 
> > > > > Syzkaller reported a general protection fault due to a NULL
> > > > > pointer
> > > > > dereference in print_reg_state() when accessing reg->map_ptr
> > > > > without
> > > > > checking if it is NULL.
> > > > > 
> > > ...snip...
> > > > > -       if (type_is_map_ptr(t)) {
> > > > > +       if (type_is_map_ptr(t) && reg->map_ptr) {
> > > > 
> > > > You ignored earlier feedback.
> > > > Fix the root cause, not the symptom.
> > > > 
> > > > pw-bot: cr
> > > 
> > > I'm not sure if I'm headed the write direction but it seems like
> > > in
> > > check_alu_op, we are calling adjust_scalar_min_max_vals when we
> > > get an
> > > BPF_NEG as opcode. Which has a call to __mark_reg_known when
> > > opcode is
> > > BPF_NEG. And __mark_reg_known clears map_ptr with
> > 
> > Looks like we're getting somewhere.
> > It seems the verifier is not clearing reg->type.
> > adjust_scalar_min_max_vals() should be called on scalar types only.
> 
> Right, there is a check in check_alu_op
> 
> 		if (is_pointer_value(env, insn->dst_reg)) {
> 			verbose(env, "R%d pointer arithmetic
> prohibited\n",
> 				insn->dst_reg);
> 			return -EACCES;
> 		}
> 
> is_pointer_value calls __is_pointer_value which takes bool
> allow_ptr_leaks as the first argument. Now for some reason in this
> case
> allow_ptr_leaks is being passed as true, as a result
> __is_pointer_value
> (and in turn is_pointer_value) returns false when even when register
> type is CONST_PTR_TO_MAP.
> 

IIUC, `env->allow_ptr_leaks` set true means privileged mode (
CAP_PERFMON or CAP_SYS_ADMIN ), false for unprivileged mode. 


We can use __is_pointer_value to check if the register type is a
pointer. For pointers, we check as before (before checking BPF_NEG
separately), and for scalars, it remains unchanged. Perhaps this way we
can fix the error.

if (opcode == BPF_NEG) {
	if (__is_pointer_value(false, &regs[insn->dst_reg])) {
		err = check_reg_arg(env, insn->dst_reg, DST_OP);
	} else {
		err = check_reg_arg(env, insn->dst_reg,
DST_OP_NO_MARK);
		err = err ?: adjust_scalar_min_max_vals(env, insn,
						&regs[insn->dst_reg],
						regs[insn->dst_reg]);
	}
} else {


-- 
Thanks,
KaFai