Pinned link access mode troubles

Pinned link access mode troubles

[Lorenz Bauer]

Hi list,

BPF_OBJ_GET allows specifying BPF_F_RDONLY or BPF_F_WRONLY for
file_flags. They are used to check that the current user has the
necessary permissions in bpf_obj_do_get:

    ret = path_permission(&path, ACC_MODE(flags));
    if (ret)
        goto out;

The map code additionally uses the flags in bpf_map_new_fd to attach
the permissions to the fd. Programs and links ignore flags (from
bpf_obj_get_user):

    if (type == BPF_TYPE_PROG)
        ret = bpf_prog_new_fd(raw);
    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);
    else
        return -ENOENT;

For programs this probably isn't too exciting, since AFAIK they are
immutable from the user space. The same isn't true for links. Given a
link that is pinned to a bpffs for which my user only has read access,
I can call BPF_LINK_UPDATE and BPF_LINK_DETACH. To me this seems to
break the privilege model. This is a real issue in our code base since
we pin a link with 0664, which means that anybody on the system can
detach our link. I can work around this by using 0660 mode for links,
but I think there are several issues that need fixing:

1. BPF_OBJ_GET doesn't return an error when flags aren't useful, like
in the program case.
2. BPF_OBJ_GET returns an fd that allows destructive actions even if
BPF_F_RDONLY is passed.

Based on some git archaeology I think we are in luck: the code in
question was introduced in commit 70ed506c3bbc ("bpf: Introduce
pinnable bpf_link abstraction") and has changed very little from what
I can see, so backporting should be doable. Additionally, it seems
like libbpf doesn't provide a way to specify file_flags when loading
pinned objects. So the likelihood of breaking users is very low.

I'd like to propose the following changes:

1. Return an error from BPF_OBJ_GET If file_flags is not 0 for
programs and links. This we can backport.
2. (optional) Add code to respect BPF_F_RDONLY, etc. for links. This
requires adding a new interface to libbpf.

Opinions?

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

www.cloudflare.com

Re: Pinned link access mode troubles

[Andrii Nakryiko]

On Wed, Mar 24, 2021 at 11:45 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
>
> Hi list,
>
> BPF_OBJ_GET allows specifying BPF_F_RDONLY or BPF_F_WRONLY for
> file_flags. They are used to check that the current user has the
> necessary permissions in bpf_obj_do_get:
>
>     ret = path_permission(&path, ACC_MODE(flags));
>     if (ret)
>         goto out;
>
> The map code additionally uses the flags in bpf_map_new_fd to attach
> the permissions to the fd. Programs and links ignore flags (from
> bpf_obj_get_user):
>
>     if (type == BPF_TYPE_PROG)
>         ret = bpf_prog_new_fd(raw);
>     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);
>     else
>         return -ENOENT;
>
> For programs this probably isn't too exciting, since AFAIK they are

Oops, reviewed your patch before I got to this email. I think for BPF
programs it might be good to reject non-O_RDWR flags as well, at least
until we will have more nuanced read/write permissions checks.

> immutable from the user space. The same isn't true for links. Given a
> link that is pinned to a bpffs for which my user only has read access,
> I can call BPF_LINK_UPDATE and BPF_LINK_DETACH. To me this seems to
> break the privilege model. This is a real issue in our code base since
> we pin a link with 0664, which means that anybody on the system can
> detach our link. I can work around this by using 0660 mode for links,
> but I think there are several issues that need fixing:
>
> 1. BPF_OBJ_GET doesn't return an error when flags aren't useful, like
> in the program case.
> 2. BPF_OBJ_GET returns an fd that allows destructive actions even if
> BPF_F_RDONLY is passed.
>
> Based on some git archaeology I think we are in luck: the code in
> question was introduced in commit 70ed506c3bbc ("bpf: Introduce
> pinnable bpf_link abstraction") and has changed very little from what
> I can see, so backporting should be doable. Additionally, it seems
> like libbpf doesn't provide a way to specify file_flags when loading
> pinned objects. So the likelihood of breaking users is very low.
>
> I'd like to propose the following changes:
>
> 1. Return an error from BPF_OBJ_GET If file_flags is not 0 for
> programs and links. This we can backport.
> 2. (optional) Add code to respect BPF_F_RDONLY, etc. for links. This
> requires adding a new interface to libbpf.
>
> Opinions?
>
> --
> Lorenz Bauer  |  Systems Engineer
> 6th Floor, County Hall/The Riverside Building, SE1 7PB, UK
>
> www.cloudflare.com