Re: [PATCH bpf] bpf: Fix Null-Pointer Dereference in kernel_clone() via BPF fmod_ret on security_task_alloc

[Jiayuan Chen]

On 4/8/26 5:48 PM, Feng Yang wrote:
> From: Feng Yang <yangfeng@xxxxxxxxxx>


[...]

> diff --git a/include/linux/bpf_lsm.h b/include/linux/bpf_lsm.h
> index 643809cc78c3..434ae335b188 100644
> --- a/include/linux/bpf_lsm.h
> +++ b/include/linux/bpf_lsm.h
> @@ -48,6 +48,8 @@ void bpf_lsm_find_cgroup_shim(const struct bpf_prog *prog, bpf_func_t *bpf_func)
>   
>   int bpf_lsm_get_retval_range(const struct bpf_prog *prog,
>   			     struct bpf_retval_range *range);
> +void bpf_security_get_retval_range(const struct bpf_prog *prog,
> +				   struct bpf_retval_range *range);
>   int bpf_set_dentry_xattr_locked(struct dentry *dentry, const char *name__str,
>   				const struct bpf_dynptr *value_p, int flags);
>   int bpf_remove_dentry_xattr_locked(struct dentry *dentry, const char *name__str);
> @@ -91,6 +93,12 @@ static inline int bpf_lsm_get_retval_range(const struct bpf_prog *prog,
>   {
>   	return -EOPNOTSUPP;
>   }
> +
> +static inline void bpf_security_get_retval_range(const struct bpf_prog *prog,
> +						 struct bpf_retval_range *range)
> +{
> +}
> +
>   static inline int bpf_set_dentry_xattr_locked(struct dentry *dentry, const char *name__str,
>   					      const struct bpf_dynptr *value_p, int flags)
>   {
> diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
> index c5c925f00202..537af87b3d5d 100644
> --- a/kernel/bpf/bpf_lsm.c
> +++ b/kernel/bpf/bpf_lsm.c
> @@ -448,3 +448,29 @@ int bpf_lsm_get_retval_range(const struct bpf_prog *prog,
>   	}
>   	return 0;
>   }
> +
> +/* hooks return 0 or 1 */
> +BTF_SET_START(bool_security_hooks)
> +#ifdef CONFIG_SECURITY_NETWORK_XFRM
> +BTF_ID(func, security_xfrm_state_pol_flow_match)
> +#endif
> +#ifdef CONFIG_AUDIT
> +BTF_ID(func, security_audit_rule_known)
> +#endif
> +BTF_ID(func, security_inode_xattr_skipcap)
> +BTF_SET_END(bool_security_hooks)
> +
> +/* Similar to bpf_lsm_get_retval_range,
> + * ensure that the return values of fmod_ret are valid.
> + */
> +void bpf_security_get_retval_range(const struct bpf_prog *prog,
> +				   struct bpf_retval_range *retval_range)
> +{
> +	if (btf_id_set_contains(&bool_security_hooks, prog->aux->attach_btf_id)) {
> +		retval_range->minval = 0;
> +		retval_range->maxval = 1;
> +	} else {
> +		retval_range->minval = -MAX_ERRNO;
> +		retval_range->maxval = 0;
> +	}
> +}
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 594260c1f382..3bfc67983e12 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -18413,8 +18413,10 @@ static bool return_retval_range(struct bpf_verifier_env *env, struct bpf_retval_
>   			*range = retval_range(0, 0);
>   			break;
>   		case BPF_TRACE_RAW_TP:
> -		case BPF_MODIFY_RETURN:
>   			return false;
> +		case BPF_MODIFY_RETURN:
> +			bpf_security_get_retval_range(env->prog, range);
> +			break;

This is a breaking change. The verifier rejection log should be more 
descriptive
so users can understand why their program is being rejected.

>   		case BPF_TRACE_ITER:
>   		default:
>   			break;

Also, I think a whitelist approach would be better here.
The known danger is specifically those security hooks whose return 
values get fed into ERR_PTR() by callers, such as:
- security_task_alloc
- security_inode_readlink
- security_task_movememory
- security_inode_follow_link
- security_fs_context_submount
- security_dentry_create_files_as
- security_perf_event_alloc
- security_inode_get_acl