Re: [PATCH v5 bpf-next 4/5] bpf: Add a bpf_getxattr kfunc

From: KP Singh <kpsingh@kernel.org>
To: Al Viro <viro@zeniv.linux.org.uk>
Cc: 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 4/5] bpf: Add a bpf_getxattr kfunc
Date: Tue, 28 Jun 2022 19:29:31 +0200	[thread overview]
Message-ID: <CACYkzJ4RyrF4dZWwt-Wo75AbE7eK7Q6dbr+b8DHcw7paMeUEwA@mail.gmail.com> (raw)
In-Reply-To: <Yrs4+ThR4ACb5eD/@ZenIV>

On Tue, Jun 28, 2022 at 7:23 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Tue, Jun 28, 2022 at 04:19:47PM +0000, KP Singh wrote:
> > LSMs like SELinux store security state in xattrs. bpf_getxattr enables
> > BPF LSM to implement similar functionality. In combination with
> > bpf_local_storage, xattrs can be used to develop more complex security
> > policies.
> >
> > This kfunc wraps around __vfs_getxattr which can sleep and is,
> > therefore, limited to sleepable programs using the newly added
> > sleepable_set for kfuncs.
>
> "Sleepable" is nowhere near enough - for a trivial example, consider
> what e.g. ext2_xattr_get() does.
>         down_read(&EXT2_I(inode)->xattr_sem);
> in there means that having that thing executed in anything that happens
> to hold ->xattr_sem is a deadlock fodder.
>

We could limit this to sleepable LSM hooks:

https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/tree/kernel/bpf/bpf_lsm.c#n169

and when we have abilities to tag
kernel functions and pointers with the work Yonghong did
(e.g. https://reviews.llvm.org/D113496) we can expand the set.

> "Can't use that in BPF program executed in non-blocking context" is
> *not* sufficient to make it safe.