Re: [PATCH bpf-next v2] bpf: expose bpf_d_path helper to vfs_* and security_* functions

[Yaniv Agman <yanivagman@gmail.com>]

‫בתאריך יום ב׳, 19 ביולי 2021 ב-18:43 מאת ‪Hengqi Chen‬‏
<‪hengqi.chen@gmail.com‬‏>:‬
>
> Add vfs_* and security_* to bpf_d_path allowlist, so that we can use
> bpf_d_path helper to extract full file path from these functions'
> `struct path *` and `struct file *` arguments. This will help tools
> like IOVisor's filetop[2]/filelife to get full file path.
>
> Changes since v1: [1]
>  - Alexei and Yonghong suggested that bpf_d_path helper could also
>    apply to vfs_* and security_file_* kernel functions. Added them.
>
> [1] https://lore.kernel.org/bpf/20210712162424.2034006-1-hengqi.chen@gmail.com/
> [2] https://github.com/iovisor/bcc/issues/3527
>
> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
> ---
>  kernel/trace/bpf_trace.c | 50 ++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 48 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 08906007306d..c784f3c7143f 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -850,16 +850,62 @@ BPF_CALL_3(bpf_d_path, struct path *, path, char *, buf, u32, sz)
>  BTF_SET_START(btf_allowlist_d_path)
>  #ifdef CONFIG_SECURITY
>  BTF_ID(func, security_file_permission)
> -BTF_ID(func, security_inode_getattr)
>  BTF_ID(func, security_file_open)
> +BTF_ID(func, security_file_ioctl)
> +BTF_ID(func, security_file_free)
> +BTF_ID(func, security_file_alloc)
> +BTF_ID(func, security_file_lock)
> +BTF_ID(func, security_file_fcntl)
> +BTF_ID(func, security_file_set_fowner)
> +BTF_ID(func, security_file_receive)
> +BTF_ID(func, security_inode_getattr)
>  #endif
>  #ifdef CONFIG_SECURITY_PATH
>  BTF_ID(func, security_path_truncate)
> +BTF_ID(func, security_path_notify)
> +BTF_ID(func, security_path_unlink)
> +BTF_ID(func, security_path_mkdir)
> +BTF_ID(func, security_path_rmdir)
> +BTF_ID(func, security_path_mknod)
> +BTF_ID(func, security_path_symlink)
> +BTF_ID(func, security_path_link)
> +BTF_ID(func, security_path_rename)
> +BTF_ID(func, security_path_chmod)
> +BTF_ID(func, security_path_chown)
> +BTF_ID(func, security_path_chroot)
>  #endif
>  BTF_ID(func, vfs_truncate)
>  BTF_ID(func, vfs_fallocate)
> -BTF_ID(func, dentry_open)
>  BTF_ID(func, vfs_getattr)
> +BTF_ID(func, vfs_fadvise)
> +BTF_ID(func, vfs_fchmod)
> +BTF_ID(func, vfs_fchown)
> +BTF_ID(func, vfs_open)
> +BTF_ID(func, vfs_setpos)
> +BTF_ID(func, vfs_llseek)
> +BTF_ID(func, vfs_read)
> +BTF_ID(func, vfs_write)
> +BTF_ID(func, vfs_iocb_iter_read)
> +BTF_ID(func, vfs_iter_read)
> +BTF_ID(func, vfs_readv)
> +BTF_ID(func, vfs_iocb_iter_write)
> +BTF_ID(func, vfs_iter_write)
> +BTF_ID(func, vfs_writev)
> +BTF_ID(func, vfs_copy_file_range)
> +BTF_ID(func, vfs_getattr_nosec)
> +BTF_ID(func, vfs_ioctl)
> +BTF_ID(func, vfs_fsync_range)
> +BTF_ID(func, vfs_fsync)
> +BTF_ID(func, vfs_utimes)
> +BTF_ID(func, vfs_statfs)
> +BTF_ID(func, vfs_dedupe_file_range_one)
> +BTF_ID(func, vfs_dedupe_file_range)
> +BTF_ID(func, vfs_clone_file_range)
> +BTF_ID(func, vfs_cancel_lock)
> +BTF_ID(func, vfs_test_lock)
> +BTF_ID(func, vfs_setlease)
> +BTF_ID(func, vfs_lock_file)
> +BTF_ID(func, dentry_open)
>  BTF_ID(func, filp_close)
>  BTF_SET_END(btf_allowlist_d_path)
>
> --
> 2.25.1
>

Thanks for opening this PR!
I was looking for a way to do that in a tool I develop (Tracee), and
had to implement this functionality by myself:
https://github.com/aquasecurity/tracee/blob/main/tracee-ebpf/tracee/tracee.bpf.c#L1494

Maybe It's too much to ask, but I wonder if it will be possible to add
other functions to this allowlist.
Currently, other than vfs_write(v), and security_file_open, we also
need it for security_sb_mount and security_bprm_check.
For security_bprm_check() we extract the path from bprm->file->f_path.
Actually, any securty_* function that has a struct argument from which
it is possible to reach a path struct is a possible candidate for us.
In addition to this, we also use it for the sched_process_exec
tracepoint, but I'm not sure if adding a tracepoint is related here,
as it is not exactly a function.

Yaniv