[PATCH bpf] bpf: link: refuse non-zero file_flags in BPF_OBJ_GET

[PATCH bpf] bpf: link: refuse non-zero file_flags in BPF_OBJ_GET

[Lorenz Bauer]

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(-)

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;
 
-- 
2.27.0

Re: [PATCH bpf] bpf: link: refuse non-zero file_flags in BPF_OBJ_GET

[Andrii Nakryiko]

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
>

Re: [PATCH bpf] bpf: link: refuse non-zero file_flags in BPF_OBJ_GET

[Lorenz Bauer]

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.

-- 
Lorenz Bauer  |  Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK

www.cloudflare.com