Re: [PATCH bpf-next 2/2] selftests/bpf: Extend test fs_kfuncs to cover security.bpf xattr names

[Christian Brauner]

On Wed, Oct 16, 2024 at 03:51:55PM +0200, Jan Kara wrote:
> On Tue 15-10-24 05:52:02, Song Liu wrote:
> > > On Oct 14, 2024, at 10:25 PM, Christoph Hellwig <hch@xxxxxxxxxxxxx> wrote:
> > > On Tue, Oct 15, 2024 at 05:21:48AM +0000, Song Liu wrote:
> > >>>> Extend test_progs fs_kfuncs to cover different xattr names. Specifically:
> > >>>> xattr name "user.kfuncs", "security.bpf", and "security.bpf.xxx" can be
> > >>>> read from BPF program with kfuncs bpf_get_[file|dentry]_xattr(); while
> > >>>> "security.bpfxxx" and "security.selinux" cannot be read.
> > >>> 
> > >>> So you read code from untrusted user.* xattrs?  How can you carve out
> > >>> that space and not known any pre-existing userspace cod uses kfuncs
> > >>> for it's own purpose?
> > >> 
> > >> I don't quite follow the comment here. 
> > >> 
> > >> Do you mean user.* xattrs are untrusted (any user can set it), so we 
> > >> should not allow BPF programs to read them? Or do you mean xattr 
> > >> name "user.kfuncs" might be taken by some use space?
> > > 
> > > All of the above.
> > 
> > This is a selftest, "user.kfunc" is picked for this test. The kfuncs
> > (bpf_get_[file|dentry]_xattr) can read any user.* xattrs. 
> > 
> > Reading untrusted xattrs from trust BPF LSM program can be useful. 
> > For example, we can sign a binary with private key, and save the
> > signature in the xattr. Then the kernel can verify the signature
> > and the binary matches the public key. If the xattr is modified by
> > untrusted user space, the BPF program will just deny the access. 
> 
> So I tend to agree with Christoph that e.g. for the above LSM usecase you
> mention, using user. xattr space is a poor design choice because you have
> to very carefully validate any xattr contents (anybody can provide
> malicious content) and more importantly as different similar usecases
> proliferate the chances of name collisions and resulting funcionality
> issues increase. It is similar as if you decided to store some information
> in a specially named file in each directory. If you choose special enough
> name, it will likely work but long-term someone is going to break you :)
> 
> I think that getting user.* xattrs from bpf hooks can still be useful for
> introspection and other tasks so I'm not convinced we should revert that
> functionality but maybe it is too easy to misuse? I'm not really decided.

Reading user.* xattr is fine. If an LSM decides to built a security
model around it then imho that's their business and since that happens
in out-of-tree LSM programs: shrug.