On Wed, Aug 4, 2021 at 3:35 PM Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote: > > On 7/27/21 3:25 PM, Hengqi Chen wrote: > > 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' arguments. > > This will help tools like BCC's filetop[1]/filelife to get full file path. > > > > [1] https://github.com/iovisor/bcc/issues/3527 > > > > Acked-by: Yonghong Song <yhs@xxxxxx> > > Signed-off-by: Hengqi Chen <hengqi.chen@xxxxxxxxx> > > --- > > kernel/trace/bpf_trace.c | 60 +++++++++++++++++++++++++++++++++++++--- > > 1 file changed, 56 insertions(+), 4 deletions(-) > > > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > > index c5e0b6a64091..e7b24abcf3bf 100644 > > --- a/kernel/trace/bpf_trace.c > > +++ b/kernel/trace/bpf_trace.c > > @@ -849,18 +849,70 @@ 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_bprm_check) > > +BTF_ID(func, security_bprm_committed_creds) > > +BTF_ID(func, security_bprm_committing_creds) > > +BTF_ID(func, security_bprm_creds_for_exec) > > +BTF_ID(func, security_bprm_creds_from_file) > > +BTF_ID(func, security_file_alloc) > > Did you actually try these out, e.g. attaching BPF progs invoking bpf_d_path() to all > these, then generate some workload like kernel build for testing? > > I presume not, since something like security_file_alloc() would crash the kernel. Right > before it's called in __alloc_file() we fetch a struct file from kmemcache, and only > populate f->f_cred there. Most LSMs, for example, only populate their secblob through the > callback. If you call bpf_d_path(&file->f_path, ...) with it, you'll crash in d_path() > when path->dentry->d_op is checked.. given f->f_path is all zeroed structure at that > point. > > Please do your due diligence and invest each of them manually, maybe the best way is > to hack up small selftests for each enabled function that our CI can test run? Bit of a > one-time effort, but at least it ensures that those additions are sane & checked. I think it's actually a pretty fun exercise and a good selftest to have. We can have a selftest which will attach a simple BPF program just to grab a contents of btf_allowlist_d_path (with typeless ksyms, for instance). Then for each BTF ID in there, as a subtest, attach another BPF object with fentry BPF program doing something with d_path. Hengqi, you'd need to have few variants for each possible position of file or path struct (e.g., file as first arg; as second arg; etc, same for hooks working with path directly), but I don't think that's going to be a lot of them. So as Daniel said, a bit of a work, but we'll have a much better confidence that we are not accidentally opening a big kernel crashing loophole. > > > +BTF_ID(func, security_file_fcntl) > > +BTF_ID(func, security_file_free) > > +BTF_ID(func, security_file_ioctl) > > +BTF_ID(func, security_file_lock) > > +BTF_ID(func, security_file_open) > > BTF_ID(func, security_file_permission) > > +BTF_ID(func, security_file_receive) > > +BTF_ID(func, security_file_set_fowner) > > BTF_ID(func, security_inode_getattr) > > -BTF_ID(func, security_file_open) > > +BTF_ID(func, security_sb_mount) > > #endif > > #ifdef CONFIG_SECURITY_PATH > > +BTF_ID(func, security_path_chmod) > > +BTF_ID(func, security_path_chown) > > +BTF_ID(func, security_path_chroot) > > +BTF_ID(func, security_path_link) > > +BTF_ID(func, security_path_mkdir) > > +BTF_ID(func, security_path_mknod) > > +BTF_ID(func, security_path_notify) > > +BTF_ID(func, security_path_rename) > > +BTF_ID(func, security_path_rmdir) > > +BTF_ID(func, security_path_symlink) > > BTF_ID(func, security_path_truncate) > > +BTF_ID(func, security_path_unlink) > > #endif > > -BTF_ID(func, vfs_truncate) > > -BTF_ID(func, vfs_fallocate) > > BTF_ID(func, dentry_open) > > -BTF_ID(func, vfs_getattr) > > BTF_ID(func, filp_close) > > +BTF_ID(func, vfs_cancel_lock) > > +BTF_ID(func, vfs_clone_file_range) > > +BTF_ID(func, vfs_copy_file_range) > > +BTF_ID(func, vfs_dedupe_file_range) > > +BTF_ID(func, vfs_dedupe_file_range_one) > > +BTF_ID(func, vfs_fadvise) > > +BTF_ID(func, vfs_fallocate) > > +BTF_ID(func, vfs_fchmod) > > +BTF_ID(func, vfs_fchown) > > +BTF_ID(func, vfs_fsync) > > +BTF_ID(func, vfs_fsync_range) > > +BTF_ID(func, vfs_getattr) > > +BTF_ID(func, vfs_getattr_nosec) > > +BTF_ID(func, vfs_iocb_iter_read) > > +BTF_ID(func, vfs_iocb_iter_write) > > +BTF_ID(func, vfs_ioctl) > > +BTF_ID(func, vfs_iter_read) > > +BTF_ID(func, vfs_iter_write) > > +BTF_ID(func, vfs_llseek) > > +BTF_ID(func, vfs_lock_file) > > +BTF_ID(func, vfs_open) > > +BTF_ID(func, vfs_read) > > +BTF_ID(func, vfs_readv) > > +BTF_ID(func, vfs_setlease) > > +BTF_ID(func, vfs_setpos) > > +BTF_ID(func, vfs_statfs) > > +BTF_ID(func, vfs_test_lock) > > +BTF_ID(func, vfs_truncate) > > +BTF_ID(func, vfs_utimes) > > +BTF_ID(func, vfs_write) > > +BTF_ID(func, vfs_writev) > > BTF_SET_END(btf_allowlist_d_path) > > > > static bool bpf_d_path_allowed(const struct bpf_prog *prog) > > >
