Re: Uninitialized Stack Variable / NULL Pointer Dereference in __sys_socket_create via BPF LSM hooks

[Matt Bobrowski <mattbobrowski@google.com>]

On Tue, Apr 21, 2026 at 09:05:17PM +0800, Xu Kuohai wrote:
> On 4/21/2026 5:59 PM, Kuniyuki Iwashima wrote:
> 
> [...]
> 
> > I think the issue here is that __sock_create() handles
> > a positive retval as error only for two LSM hooks,
> > security_socket_create() and security_socket_post_create(),
> > which is different from __sys_socket_create()'s expectation.
> > 
> > sock_create()(_kern()) callers are supposed to check the retval
> > first before accessing the passed &sock pointer, so I don't
> > think leaving it uninitialised itself is a problem and I'd
> > rather ignore >0 value as allowed.
> > 
> > And I found a good place in verifier to reject the repro.
> > 
> > ---8<---
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 9e4980128151..19cc4ed8c389 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -10448,6 +10448,11 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
> >   				verbose(env, "BPF_LSM_CGROUP that attach to void LSM hooks can't modify return value!\n");
> >   				return -EINVAL;
> >   			}
> > +
> > +			if (regs[BPF_REG_1].s32_max_value > 0) {
> > +				verbose(env, "BPF_LSM_CGROUP can't return a positive value!\n");
> > +				return -EINVAL;
> > +			}
> >   		}
> >   		break;
> >   	case BPF_FUNC_dynptr_data:
> > diff --git a/net/socket.c b/net/socket.c
> > index 22a412fdec07..d40aea527f66 100644
> > --- a/net/socket.c
> > +++ b/net/socket.c
> > @@ -1617,7 +1617,7 @@ int __sock_create(struct net *net, int family, int type, int protocol,
> >   	}
> >   	err = security_socket_create(family, type, protocol, kern);
> > -	if (err)
> > +	if (err < 0)
> >   		return err;
> >   	/*
> > @@ -1685,7 +1685,7 @@ int __sock_create(struct net *net, int family, int type, int protocol,
> >   	 */
> >   	module_put(pf->owner);
> >   	err = security_socket_post_create(sock, family, type, protocol, kern);
> > -	if (err)
> > +	if (err < 0)
> >   		goto out_sock_release;
> >   	*res = sock;
> > ---8<---
> > 
> > 
> 
> Since the value set by bpf_set_retval() is the actual LSM hook return value,
> I think we should check it directly against the hook’s valid return range,
> rather than making a specific change for the socket_create hook.
> 
> The diff below should work:
> 
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -10449,6 +10449,9 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
>         case BPF_FUNC_set_retval:
>                 if (prog_type == BPF_PROG_TYPE_LSM &&
>                     env->prog->expected_attach_type == BPF_LSM_CGROUP) {
> +                       struct bpf_reg_state *reg;
> +                       struct bpf_retval_range range;
> +
>                         if (!env->prog->aux->attach_func_proto->type) {
>                                 /* Make sure programs that attach to void
>                                  * hooks don't try to modify return value.
> @@ -10456,6 +10459,13 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
>                                 verbose(env, "BPF_LSM_CGROUP that attach to void LSM hooks can't modify return value!\n");
>                                 return -EINVAL;
>                         }
> +
> +                       reg = reg_state(env, BPF_REG_1);
> +                       bpf_lsm_get_retval_range(env->prog, &range);
> +                       if (!retval_range_within(range, reg)) {
> +                               verbose_invalid_scalar(env, reg, range, "At bpf_set_retval", "R1");
> +                               return -EINVAL;
> +                       }

Yeah, I think we should definitely add something like this. Can you
send it through as a separate patch please?