Re: [PATCH v5 bpf-next 0/5] Add bpf_getxattr

From: KP Singh <kpsingh@kernel.org>
To: Dave Chinner <david@fromorbit.com>
Cc: Christian Brauner <brauner@kernel.org>,
	bpf@vger.kernel.org, linux-security-module@vger.kernel.org,
	linux-fsdevel@vger.kernel.org,
	Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Andrii Nakryiko <andrii@kernel.org>,
	Benjamin Tissoires <benjamin.tissoires@redhat.com>,
	Yosry Ahmed <yosryahmed@google.com>
Subject: Re: [PATCH v5 bpf-next 0/5] Add bpf_getxattr
Date: Wed, 29 Jun 2022 04:05:48 +0200	[thread overview]
Message-ID: <CACYkzJ70uqVJr5EnM0i03Lu+zkuSsXOXcOLQoUS6HZPqH=skpQ@mail.gmail.com> (raw)
In-Reply-To: <CACYkzJ7dyY7m8gmHkhWQ0CCLLbq9BaYBzMnjV2eWADYoys9T8w@mail.gmail.com>

On Wed, Jun 29, 2022 at 4:00 AM KP Singh <kpsingh@kernel.org> wrote:
>
> On Wed, Jun 29, 2022 at 3:36 AM Dave Chinner <david@fromorbit.com> wrote:
> >
> > On Tue, Jun 28, 2022 at 07:21:42PM +0200, KP Singh wrote:
> > > On Tue, Jun 28, 2022 at 7:20 PM KP Singh <kpsingh@kernel.org> wrote:
> > > > On Tue, Jun 28, 2022 at 7:13 PM Christian Brauner <brauner@kernel.org> wrote:
> > > > > On Tue, Jun 28, 2022 at 04:19:43PM +0000, KP Singh wrote:
> > > > > > v4 -> v5
> > > > > >
> > > > > > - Fixes suggested by Andrii
> > > > > >
> > > > > > v3 -> v4
> > > > > >
> > > > > > - Fixed issue incorrect increment of arg counter
> > > > > > - Removed __weak and noinline from kfunc definiton
> > > > > > - Some other minor fixes.
> > > > > >
> > > > > > v2 -> v3
> > > > > >
> > > > > > - Fixed missing prototype error
> > > > > > - Fixes suggested by other Joanne and Kumar.
> > > > > >
> > > > > > v1 -> v2
> > > > > >
> > > > > > - Used kfuncs as suggested by Alexei
> > > > > > - Used Benjamin Tissoires' patch from the HID v4 series to add a
> > > > > >   sleepable kfunc set (I sent the patch as a part of this series as it
> > > > > >   seems to have been dropped from v5) and acked it. Hope this is okay.
> > > > > > - Added support for verifying string constants to kfuncs
> > > > >
> > > > > Hm, I mean this isn't really giving any explanation as to why you are
> > > > > doing this. There's literally not a single sentence about the rationale?
> > > > > Did you accidently forget to put that into the cover letter? :)
> > > >
> > > >
> > > > Yes, actually I did forget to copy paste :)
> > > >
> > > > Foundation for building more complex security policies using the
> > > > BPF LSM as presented in LSF/MM/BPF:
> > > >
> > > > http://vger.kernel.org/bpfconf2022_material/lsfmmbpf2022-xattr.pdf\
> > >
> > > And my copy paste skills are getting worse (with the back-slash removed):
> > >
> > > http://vger.kernel.org/bpfconf2022_material/lsfmmbpf2022-xattr.pdf
> >
> > There's literally zero information in that link, so I still have no
> > clue on what this does and how it interacts with filesystem xattr
> > code.
>
> This is literally a wrapper around __vfs_getxattr which is an exported
> symbol. So, the interaction with the xattr code is the same as
> __vfs_getxattr interacts currently.
>
> ssize_t bpf_getxattr(struct dentry *dentry, struct inode *inode,
> const char *name, void *value, int value__sz)
> {
> return __vfs_getxattr(dentry, inode, name, value, value__sz);
> }
>
> The reason for the wrapper is that the BPF verifier offers
> extra checks on the arguments passed.
>
> https://lore.kernel.org/bpf/20210325015240.1550074-1-kafai@fb.com/T/
>
> has more information on the kfunc support.
>
> >
> > So for those of us who have zero clue as to what you are trying to
> > do, please write a cover letter containing a non-zero amount of
> > information.  i.e.  a description of the problem, the threat model
> > being addressed, the design of the infrastructure that needs this
> > hook, document assumptions that have been made (e.g. for
> > accessing inode metadata atomically from random bpf contexts), what
>
> The intention is to use this in BPF programs which can only be loaded
> with CAP_SYS_ADMIN.
> We are currently planning on limiting the usage of this kfunc
> to the sleepable LSM hooks listed here:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/tree/kernel/bpf/bpf_lsm.c#n169
>
> > xattr namespace(s) this hook should belong/be constrained to,
> > whether you're going to ask for a setxattr hook next, etc.
>
> Fair point, I will resend the series with the details.
>
> >
> > At minimum this is going to need a bunch of documentation for people
> > to understand how to use this - where can I find that?
>
> There are a bunch of examples in selftests on how to use kfuncs in BPF
> and we added a selftests (there is a simple selftests added with this patch
> too).
>
> As to how we will use xattrs to create security policies or use this
> functionality for
> logging, this is work in progress.

In any case, I will update the cover letter with some use-cases
we ideated over in LSF/MM/BPF in the next version.

- KP

>
> Cheers,
> - KP
>
> >
> > Cheers,
> >
> > Dave.
> > --
> > Dave Chinner
> > david@fromorbit.com