On Wed, Mar 24, 2021 at 11:45 AM Lorenz Bauer <lmb@xxxxxxxxxxxxxx> 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
