* [PATCH bpf-next v5 0/2] bpf: expand bpf_d_path helper allowlist
@ 2021-07-27 13:25 Hengqi Chen
2021-07-27 13:25 ` [PATCH bpf-next v5 1/2] tools/resolve_btfids: emit warnings and patch zero id for missing symbols Hengqi Chen
2021-07-27 13:25 ` [PATCH bpf-next v5 2/2] bpf: expose bpf_d_path helper to vfs_* and security_* functions Hengqi Chen
0 siblings, 2 replies; 7+ messages in thread
From: Hengqi Chen @ 2021-07-27 13:25 UTC (permalink / raw)
To: bpf
Cc: ast, daniel, andrii, yhs, john.fastabend, jolsa, yanivagman, hengqi.chen
This patch set adds more functions to bpf_d_path allowlist.
Patch 1 is prep work which updates resolve_btfids to emit warnings
on missing symbols instead of aborting kernel build process.
Patch 2 expands bpf_d_path allowlist.
Changes since v4: [4]
- Addressed Andrii's comments. Update warning message.
Changes since v3: [3]
- Addressed Yonghong's comments. Sort allowlist and add security_bprm_*
Changes since v2: [2]
- Andrii suggested that we should first address an issue of .BTF_ids
before adding more symbols to .BTF_ids. Fixed that.
- Yaniv proposed adding security_sb_mount and security_bprm_check.
Added them.
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://lore.kernel.org/bpf/20210719151753.399227-1-hengqi.chen@gmail.com/
[3] https://lore.kernel.org/bpf/20210725141814.2000828-3-hengqi.chen@gmail.com/
[4] https://lore.kernel.org/bpf/20210725141814.2000828-2-hengqi.chen@gmail.com/
Hengqi Chen (2):
tools/resolve_btfids: emit warnings and patch zero id for missing
symbols
bpf: expose bpf_d_path helper to vfs_* and security_* functions
kernel/trace/bpf_trace.c | 60 ++++++++++++++++++++++++++++++---
tools/bpf/resolve_btfids/main.c | 13 +++----
2 files changed, 63 insertions(+), 10 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH bpf-next v5 1/2] tools/resolve_btfids: emit warnings and patch zero id for missing symbols 2021-07-27 13:25 [PATCH bpf-next v5 0/2] bpf: expand bpf_d_path helper allowlist Hengqi Chen @ 2021-07-27 13:25 ` Hengqi Chen 2021-07-27 13:25 ` [PATCH bpf-next v5 2/2] bpf: expose bpf_d_path helper to vfs_* and security_* functions Hengqi Chen 1 sibling, 0 replies; 7+ messages in thread From: Hengqi Chen @ 2021-07-27 13:25 UTC (permalink / raw) To: bpf Cc: ast, daniel, andrii, yhs, john.fastabend, jolsa, yanivagman, hengqi.chen Kernel functions referenced by .BTF_ids may be changed from global to static and get inlined or get renamed/removed, and thus disappears from BTF. This causes kernel build failure when resolve_btfids do id patch for symbols in .BTF_ids in vmlinux. Update resolve_btfids to emit warning messages and patch zero id for missing symbols instead of aborting kernel build process. Suggested-by: Andrii Nakryiko <andrii@kernel.org> Acked-by: Yonghong Song <yhs@fb.com> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com> --- tools/bpf/resolve_btfids/main.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c index 3ad9301b0f00..de6365b53c9c 100644 --- a/tools/bpf/resolve_btfids/main.c +++ b/tools/bpf/resolve_btfids/main.c @@ -291,7 +291,7 @@ static int compressed_section_fix(Elf *elf, Elf_Scn *scn, GElf_Shdr *sh) sh->sh_addralign = expected; if (gelf_update_shdr(scn, sh) == 0) { - printf("FAILED cannot update section header: %s\n", + pr_err("FAILED cannot update section header: %s\n", elf_errmsg(-1)); return -1; } @@ -317,6 +317,7 @@ static int elf_collect(struct object *obj) elf = elf_begin(fd, ELF_C_RDWR_MMAP, NULL); if (!elf) { + close(fd); pr_err("FAILED cannot create ELF descriptor: %s\n", elf_errmsg(-1)); return -1; @@ -484,7 +485,7 @@ static int symbols_resolve(struct object *obj) err = libbpf_get_error(btf); if (err) { pr_err("FAILED: load BTF from %s: %s\n", - obj->path, strerror(-err)); + obj->btf ?: obj->path, strerror(-err)); return -1; } @@ -555,8 +556,7 @@ static int id_patch(struct object *obj, struct btf_id *id) int i; if (!id->id) { - pr_err("FAILED unresolved symbol %s\n", id->name); - return -EINVAL; + pr_err("WARN: resolve_btfids: unresolved symbol %s\n", id->name); } for (i = 0; i < id->addr_cnt; i++) { @@ -734,8 +734,9 @@ int main(int argc, const char **argv) err = 0; out: - if (obj.efile.elf) + if (obj.efile.elf) { elf_end(obj.efile.elf); - close(obj.efile.fd); + close(obj.efile.fd); + } return err; } -- 2.25.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH bpf-next v5 2/2] bpf: expose bpf_d_path helper to vfs_* and security_* functions 2021-07-27 13:25 [PATCH bpf-next v5 0/2] bpf: expand bpf_d_path helper allowlist Hengqi Chen 2021-07-27 13:25 ` [PATCH bpf-next v5 1/2] tools/resolve_btfids: emit warnings and patch zero id for missing symbols Hengqi Chen @ 2021-07-27 13:25 ` Hengqi Chen 2021-08-04 22:35 ` Daniel Borkmann 1 sibling, 1 reply; 7+ messages in thread From: Hengqi Chen @ 2021-07-27 13:25 UTC (permalink / raw) To: bpf Cc: ast, daniel, andrii, yhs, john.fastabend, jolsa, yanivagman, hengqi.chen 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@fb.com> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com> --- 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) +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) -- 2.25.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next v5 2/2] bpf: expose bpf_d_path helper to vfs_* and security_* functions 2021-07-27 13:25 ` [PATCH bpf-next v5 2/2] bpf: expose bpf_d_path helper to vfs_* and security_* functions Hengqi Chen @ 2021-08-04 22:35 ` Daniel Borkmann 2021-08-04 22:44 ` Andrii Nakryiko 0 siblings, 1 reply; 7+ messages in thread From: Daniel Borkmann @ 2021-08-04 22:35 UTC (permalink / raw) To: Hengqi Chen, bpf Cc: ast, andrii, yhs, john.fastabend, jolsa, yanivagman, kpsingh 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@fb.com> > Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com> > --- > 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. > +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) > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next v5 2/2] bpf: expose bpf_d_path helper to vfs_* and security_* functions 2021-08-04 22:35 ` Daniel Borkmann @ 2021-08-04 22:44 ` Andrii Nakryiko 2021-08-05 2:27 ` Hengqi Chen 0 siblings, 1 reply; 7+ messages in thread From: Andrii Nakryiko @ 2021-08-04 22:44 UTC (permalink / raw) To: Daniel Borkmann Cc: Hengqi Chen, bpf, Alexei Starovoitov, Andrii Nakryiko, Yonghong Song, john fastabend, Jiri Olsa, Yaniv Agman, KP Singh On Wed, Aug 4, 2021 at 3:35 PM Daniel Borkmann <daniel@iogearbox.net> 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@fb.com> > > Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com> > > --- > > 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) > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next v5 2/2] bpf: expose bpf_d_path helper to vfs_* and security_* functions 2021-08-04 22:44 ` Andrii Nakryiko @ 2021-08-05 2:27 ` Hengqi Chen 2021-08-05 17:28 ` KP Singh 0 siblings, 1 reply; 7+ messages in thread From: Hengqi Chen @ 2021-08-05 2:27 UTC (permalink / raw) To: Andrii Nakryiko, Daniel Borkmann Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Yonghong Song, john fastabend, Jiri Olsa, Yaniv Agman, KP Singh On 2021/8/5 6:44 AM, Andrii Nakryiko wrote: > On Wed, Aug 4, 2021 at 3:35 PM Daniel Borkmann <daniel@iogearbox.net> 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@fb.com> >>> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com> >>> --- >>> 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. > Thanks for the review and suggestions. Will test them locally and add selftest for these changes. >> >>> +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) >>> >> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next v5 2/2] bpf: expose bpf_d_path helper to vfs_* and security_* functions 2021-08-05 2:27 ` Hengqi Chen @ 2021-08-05 17:28 ` KP Singh 0 siblings, 0 replies; 7+ messages in thread From: KP Singh @ 2021-08-05 17:28 UTC (permalink / raw) To: Hengqi Chen Cc: Andrii Nakryiko, Daniel Borkmann, bpf, Alexei Starovoitov, Andrii Nakryiko, Yonghong Song, john fastabend, Jiri Olsa, Yaniv Agman On Thu, Aug 5, 2021 at 4:27 AM Hengqi Chen <hengqi.chen@gmail.com> wrote: > > > > On 2021/8/5 6:44 AM, Andrii Nakryiko wrote: > > On Wed, Aug 4, 2021 at 3:35 PM Daniel Borkmann <daniel@iogearbox.net> 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@fb.com> > >>> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com> > >>> --- > >>> 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) You can also look at the set of LSM hooks that are allow listed for bpf_d_path: https://elixir.bootlin.com/linux/latest/source/kernel/bpf/bpf_lsm.c you could refactor this set so that it also adds the corresponding security_* functions for tracing programs. > >> > >> 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. +1 Good idea! > > > > 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. > > > > Thanks for the review and suggestions. > [...] > >> ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-08-05 17:28 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-27 13:25 [PATCH bpf-next v5 0/2] bpf: expand bpf_d_path helper allowlist Hengqi Chen 2021-07-27 13:25 ` [PATCH bpf-next v5 1/2] tools/resolve_btfids: emit warnings and patch zero id for missing symbols Hengqi Chen 2021-07-27 13:25 ` [PATCH bpf-next v5 2/2] bpf: expose bpf_d_path helper to vfs_* and security_* functions Hengqi Chen 2021-08-04 22:35 ` Daniel Borkmann 2021-08-04 22:44 ` Andrii Nakryiko 2021-08-05 2:27 ` Hengqi Chen 2021-08-05 17:28 ` KP Singh
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.