| Message ID | 20210325152146.188654-1-lmb@cloudflare.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf] bpf: link: refuse non-zero file_flags in BPF_OBJ_GET | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for bpf |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 5 maintainers not CCed: yhs@fb.com kpsingh@kernel.org kafai@fb.com john.fastabend@gmail.com songliubraving@fb.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 1 this patch: 1 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 8 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 1 this patch: 1 |
| netdev/header_inline | success | Link |
On Thu, Mar 25, 2021 at 8:22 AM Lorenz Bauer <lmb@cloudflare.com> wrote: > > Invoking BPF_OBJ_GET on a pinned bpf_link checks the path access > permissions based on file_flags, but the returned fd ignores flags. > This means that any user can acquire a "read-write" fd for a pinned > link with mode 0664 by invoking BPF_OBJ_GET with BPF_F_RDONLY in > file_flags. The fd can be used to invoke BPF_LINK_DETACH, etc. > > Fix this by refusing non-zero flags in BPF_OBJ_GET. Since zero flags > imply O_RDWR this requires users to have read-write access to the > pinned file, which matches the behaviour of the link primitive. > > libbpf doesn't expose a way to set file_flags for links, so this > change is unlikely to break users. > > Fixes: 70ed506c3bbc ("bpf: Introduce pinnable bpf_link abstraction") > Signed-off-by: Lorenz Bauer <lmb@cloudflare.com> > --- Makes sense, but see below about details. Also, should we do the same for BPF programs as well? I guess they don't have a "write operation", once loaded, but still... > kernel/bpf/inode.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/kernel/bpf/inode.c b/kernel/bpf/inode.c > index 1576ff331ee4..2f9e8115ad58 100644 > --- a/kernel/bpf/inode.c > +++ b/kernel/bpf/inode.c > @@ -547,7 +547,7 @@ int bpf_obj_get_user(const char __user *pathname, int flags) > else if (type == BPF_TYPE_MAP) > ret = bpf_map_new_fd(raw, f_flags); > else if (type == BPF_TYPE_LINK) > - ret = bpf_link_new_fd(raw); > + ret = (flags) ? -EINVAL : bpf_link_new_fd(raw); nit: unnecessary () I wonder if EACCESS would make more sense here? And check f_flags, not flags: if (f_flags != O_RDWR) ret = -EACCESS; else ret = bpf_link_new_fd(raw); ? > else > return -ENOENT; > > -- > 2.27.0 >
On Fri, 26 Mar 2021 at 04:43, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > Makes sense, but see below about details. > > Also, should we do the same for BPF programs as well? I guess they > don't have a "write operation", once loaded, but still... I asked myself the same question, I don't have a good answer. Right now it seems like no harm is done, but this will probably bite us again in the future. Would you want to backport this? We'd have to target commit 6e71b04a8224 ("bpf: Add file mode configuration into bpf maps") I think, which appeared in 4.14 (?). Maybe it's better to just refuse the flag in bpf-next? > > > kernel/bpf/inode.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/kernel/bpf/inode.c b/kernel/bpf/inode.c > > index 1576ff331ee4..2f9e8115ad58 100644 > > --- a/kernel/bpf/inode.c > > +++ b/kernel/bpf/inode.c > > @@ -547,7 +547,7 @@ int bpf_obj_get_user(const char __user *pathname, int flags) > > else if (type == BPF_TYPE_MAP) > > ret = bpf_map_new_fd(raw, f_flags); > > else if (type == BPF_TYPE_LINK) > > - ret = bpf_link_new_fd(raw); > > + ret = (flags) ? -EINVAL : bpf_link_new_fd(raw); > > nit: unnecessary () > > > I wonder if EACCESS would make more sense here? My thinking was: the access mode is fine if we get to this place, but the code in question doesn't support that particular flag. EINVAL seemed more appropriate. Happy to change it if you prefer. >And check f_flags, not flags: > > if (f_flags != O_RDWR) > ret = -EACCESS; > else > ret = bpf_link_new_fd(raw); I'll respin with f_flags. I'd prefer keeping the ternary operator version though, since this should ease backporting.
diff --git a/kernel/bpf/inode.c b/kernel/bpf/inode.c index 1576ff331ee4..2f9e8115ad58 100644 --- a/kernel/bpf/inode.c +++ b/kernel/bpf/inode.c @@ -547,7 +547,7 @@ int bpf_obj_get_user(const char __user *pathname, int flags) else if (type == BPF_TYPE_MAP) ret = bpf_map_new_fd(raw, f_flags); else if (type == BPF_TYPE_LINK) - ret = bpf_link_new_fd(raw); + ret = (flags) ? -EINVAL : bpf_link_new_fd(raw); else return -ENOENT;
Invoking BPF_OBJ_GET on a pinned bpf_link checks the path access permissions based on file_flags, but the returned fd ignores flags. This means that any user can acquire a "read-write" fd for a pinned link with mode 0664 by invoking BPF_OBJ_GET with BPF_F_RDONLY in file_flags. The fd can be used to invoke BPF_LINK_DETACH, etc. Fix this by refusing non-zero flags in BPF_OBJ_GET. Since zero flags imply O_RDWR this requires users to have read-write access to the pinned file, which matches the behaviour of the link primitive. libbpf doesn't expose a way to set file_flags for links, so this change is unlikely to break users. Fixes: 70ed506c3bbc ("bpf: Introduce pinnable bpf_link abstraction") Signed-off-by: Lorenz Bauer <lmb@cloudflare.com> --- kernel/bpf/inode.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)