Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210325152146.188654-1-lmb@cloudflare.com> <CAEf4BzbC75N2xHW0kB76AZCbnD+01LA5T+tn4XfBPL=b=xNS4A@mail.gmail.com>
In-Reply-To: <CAEf4BzbC75N2xHW0kB76AZCbnD+01LA5T+tn4XfBPL=b=xNS4A@mail.gmail.com>
From:   Lorenz Bauer <lmb@cloudflare.com>
Date:   Fri, 26 Mar 2021 09:21:28 +0000
Message-ID: <CACAyw9--2Nj6L-r__A=-Xka97ONE8UqiP=Ku7sY+5GHQHsayGA@mail.gmail.com>
Subject: Re: [PATCH bpf] bpf: link: refuse non-zero file_flags in BPF_OBJ_GET
To:     Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc:     Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Andrii Nakryiko <andrii@kernel.org>,
        kernel-team <kernel-team@cloudflare.com>,
        Andrii Nakryiko <andriin@fb.com>,
        Networking <netdev@vger.kernel.org>, bpf <bpf@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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