Re: Pinned link access mode troubles

On Wed, Mar 24, 2021 at 11:45 AM Lorenz Bauer [off-list ref] 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