Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220621163757.760304-1-roberto.sassu@huawei.com>
 <20220621163757.760304-3-roberto.sassu@huawei.com> <20220621223248.f6wgyewajw6x4lgr@macbook-pro-3.dhcp.thefacebook.com>
 <796b55c79be142cab6a22dd281fdb9fa@huawei.com> <f2d3da08e7774df9b44cc648dda7d0b8@huawei.com>
In-Reply-To: <f2d3da08e7774df9b44cc648dda7d0b8@huawei.com>
From:   Alexei Starovoitov <alexei.starovoitov@gmail.com>
Date:   Thu, 23 Jun 2022 13:54:13 -0700
Message-ID: <CAADnVQKVx9o1PcCV_F3ywJCzDTPtQG4MTKM2BmwdCwNvyxdNPg@mail.gmail.com>
Subject: Re: [PATCH v5 2/5] bpf: Add bpf_lookup_user_key() and bpf_key_put() helpers
To:     Roberto Sassu <roberto.sassu@huawei.com>
Cc:     "ast@kernel.org" <ast@kernel.org>,
        "daniel@iogearbox.net" <daniel@iogearbox.net>,
        "andrii@kernel.org" <andrii@kernel.org>,
        "kpsingh@kernel.org" <kpsingh@kernel.org>,
        "john.fastabend@gmail.com" <john.fastabend@gmail.com>,
        "songliubraving@fb.com" <songliubraving@fb.com>,
        "kafai@fb.com" <kafai@fb.com>, "yhs@fb.com" <yhs@fb.com>,
        "dhowells@redhat.com" <dhowells@redhat.com>,
        "keyrings@vger.kernel.org" <keyrings@vger.kernel.org>,
        "bpf@vger.kernel.org" <bpf@vger.kernel.org>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kselftest@vger.kernel.org" <linux-kselftest@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Jun 23, 2022 at 5:36 AM Roberto Sassu <roberto.sassu@huawei.com> wrote:
>
> > From: Roberto Sassu [mailto:roberto.sassu@huawei.com]
> > Sent: Wednesday, June 22, 2022 9:12 AM
> > > From: Alexei Starovoitov [mailto:alexei.starovoitov@gmail.com]
> > > Sent: Wednesday, June 22, 2022 12:33 AM
> > > On Tue, Jun 21, 2022 at 06:37:54PM +0200, Roberto Sassu wrote:
> > > > Add the bpf_lookup_user_key() and bpf_key_put() helpers, to respectively
> > > > search a key with a given serial, and release the reference count of the
> > > > found key.
> > > >
> > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > > ---
> > > >  include/uapi/linux/bpf.h       | 16 ++++++++++++
> > > >  kernel/bpf/bpf_lsm.c           | 46 ++++++++++++++++++++++++++++++++++
> > > >  kernel/bpf/verifier.c          |  6 +++--
> > > >  scripts/bpf_doc.py             |  2 ++
> > > >  tools/include/uapi/linux/bpf.h | 16 ++++++++++++
> > > >  5 files changed, 84 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > > > index e81362891596..7bbcf2cd105d 100644
> > > > --- a/include/uapi/linux/bpf.h
> > > > +++ b/include/uapi/linux/bpf.h
> > > > @@ -5325,6 +5325,20 @@ union bpf_attr {
> > > >   *               **-EACCES** if the SYN cookie is not valid.
> > > >   *
> > > >   *               **-EPROTONOSUPPORT** if CONFIG_IPV6 is not builtin.
> > > > + *
> > > > + * struct key *bpf_lookup_user_key(u32 serial, unsigned long flags)
> > > > + *       Description
> > > > + *               Search a key with a given *serial* and the provided *flags*, and
> > > > + *               increment the reference count of the key.
> > >
> > > Why passing 'flags' is ok to do?
> > > Please think through every line of the patch.
> >
> > To be honest, I thought about it. Probably yes, I should do some
> > sanitization, like I did for the keyring ID. When I checked
> > lookup_user_key(), I saw that flags are checked individually, so
> > an arbitrary value passed to the helper should not cause harm.
> > Will do sanitization, if you prefer. It is just that we have to keep
> > the eBPF code in sync with key flag definition (unless we have
> > a 'last' flag).
>
> I'm not sure that having a helper for lookup_user_key() alone is
> correct. By having separate helpers for lookup and usage of the
> key, nothing would prevent an eBPF program to ask for a
> permission to pass the access control check, and then use the
> key for something completely different from what it requested.
>
> Looking at how lookup_user_key() is used in security/keys/keyctl.c,
> it seems clear that it should be used together with the operation
> that needs to be performed. Only in this way, the key permission
> would make sense.

lookup is roughly equivalent to open when all permission checks are done.
And using the key is read/write.

> What do you think (also David)?
>
> Thanks
>
> Roberto
>
> HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
> Managing Director: Li Peng, Yang Xi, Li He

Please use a different email server and get rid of this.