Re: [PATCH bpf-next] bpf: Mark dentry->d_inode as trusted_or_null

[Matt Bobrowski <mattbobrowski@google.com>]

On Thu, Jun 12, 2025 at 10:28:56PM -0700, Song Liu wrote:
> LSM hooks such as security_path_mknod() and security_inode_rename() have
> access to newly allocated negative dentry, which has NULL d_inode.
> Therefore, it is necessary to do the NULL pointer check for d_inode.
> 
> Also add selftests that checks the verifier enforces the NULL pointer
> check.

Thank you for correcting this. Looks OK to me.

Reviewed-by: Matt Bobrowski <mattbobrowski@google.com>

> Signed-off-by: Song Liu <song@kernel.org>
>
> ---
>  kernel/bpf/verifier.c                          |  5 ++---
>  .../selftests/bpf/progs/verifier_vfs_accept.c  | 18 ++++++++++++++++++
>  .../selftests/bpf/progs/verifier_vfs_reject.c  | 15 +++++++++++++++
>  3 files changed, 35 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 14dd836acb13..5c7775cf1259 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -7104,8 +7104,7 @@ BTF_TYPE_SAFE_TRUSTED(struct file) {
>  	struct inode *f_inode;
>  };
>  
> -BTF_TYPE_SAFE_TRUSTED(struct dentry) {
> -	/* no negative dentry-s in places where bpf can see it */
> +BTF_TYPE_SAFE_TRUSTED_OR_NULL(struct dentry) {
>  	struct inode *d_inode;
>  };
>  
> @@ -7143,7 +7142,6 @@ static bool type_is_trusted(struct bpf_verifier_env *env,
>  	BTF_TYPE_EMIT(BTF_TYPE_SAFE_TRUSTED(struct bpf_iter__task));
>  	BTF_TYPE_EMIT(BTF_TYPE_SAFE_TRUSTED(struct linux_binprm));
>  	BTF_TYPE_EMIT(BTF_TYPE_SAFE_TRUSTED(struct file));
> -	BTF_TYPE_EMIT(BTF_TYPE_SAFE_TRUSTED(struct dentry));
>  
>  	return btf_nested_type_is_trusted(&env->log, reg, field_name, btf_id, "__safe_trusted");
>  }
> @@ -7153,6 +7151,7 @@ static bool type_is_trusted_or_null(struct bpf_verifier_env *env,
>  				    const char *field_name, u32 btf_id)
>  {
>  	BTF_TYPE_EMIT(BTF_TYPE_SAFE_TRUSTED_OR_NULL(struct socket));
> +	BTF_TYPE_EMIT(BTF_TYPE_SAFE_TRUSTED_OR_NULL(struct dentry));
>  
>  	return btf_nested_type_is_trusted(&env->log, reg, field_name, btf_id,
>  					  "__safe_trusted_or_null");
> diff --git a/tools/testing/selftests/bpf/progs/verifier_vfs_accept.c b/tools/testing/selftests/bpf/progs/verifier_vfs_accept.c
> index a7c0a553aa50..3e2d76ee8050 100644
> --- a/tools/testing/selftests/bpf/progs/verifier_vfs_accept.c
> +++ b/tools/testing/selftests/bpf/progs/verifier_vfs_accept.c
> @@ -2,6 +2,7 @@
>  /* Copyright (c) 2024 Google LLC. */
>  
>  #include <vmlinux.h>
> +#include <errno.h>
>  #include <bpf/bpf_helpers.h>
>  #include <bpf/bpf_tracing.h>
>  
> @@ -82,4 +83,21 @@ int BPF_PROG(path_d_path_from_file_argument, struct file *file)
>  	return 0;
>  }
>  
> +SEC("lsm.s/inode_rename")
> +__success
> +int BPF_PROG(inode_rename, struct inode *old_dir, struct dentry *old_dentry,
> +	     struct inode *new_dir, struct dentry *new_dentry,
> +	     unsigned int flags)
> +{
> +	struct inode *inode = new_dentry->d_inode;
> +	ino_t ino;
> +
> +	if (!inode)
> +		return 0;
> +	ino = inode->i_ino;
> +	if (ino == 0)
> +		return -EACCES;
> +	return 0;
> +}
> +
>  char _license[] SEC("license") = "GPL";
> diff --git a/tools/testing/selftests/bpf/progs/verifier_vfs_reject.c b/tools/testing/selftests/bpf/progs/verifier_vfs_reject.c
> index d6d3f4fcb24c..4b392c6c8fc4 100644
> --- a/tools/testing/selftests/bpf/progs/verifier_vfs_reject.c
> +++ b/tools/testing/selftests/bpf/progs/verifier_vfs_reject.c
> @@ -2,6 +2,7 @@
>  /* Copyright (c) 2024 Google LLC. */
>  
>  #include <vmlinux.h>
> +#include <errno.h>
>  #include <bpf/bpf_helpers.h>
>  #include <bpf/bpf_tracing.h>
>  #include <linux/limits.h>
> @@ -158,4 +159,18 @@ int BPF_PROG(path_d_path_kfunc_non_lsm, struct path *path, struct file *f)
>  	return 0;
>  }
>  
> +SEC("lsm.s/inode_rename")
> +__failure __msg("invalid mem access 'trusted_ptr_or_null_'")
> +int BPF_PROG(inode_rename, struct inode *old_dir, struct dentry *old_dentry,
> +	     struct inode *new_dir, struct dentry *new_dentry,
> +	     unsigned int flags)
> +{
> +	struct inode *inode = new_dentry->d_inode;
> +	ino_t ino;
> +
> +	ino = inode->i_ino;
> +	if (ino == 0)
> +		return -EACCES;
> +	return 0;
> +}
>  char _license[] SEC("license") = "GPL";
> -- 
> 2.47.1
>