[RFC][PATCH] bpf: Check xattr name/value pair from bpf_lsm_inode_init_security()

[Alexei Starovoitov]

On Mon, Oct 24, 2022 at 8:28 AM Roberto Sassu
<roberto.sassu at huaweicloud.com> wrote:
>
> On Mon, 2022-10-24 at 11:25 +0200, Roberto Sassu wrote:
> > On Sun, 2022-10-23 at 16:36 -0700, Alexei Starovoitov wrote:
> >
> > Sorry, forgot to CC Mimi and linux-integrity.
> >
> > > On Fri, Oct 21, 2022 at 9:57 AM Roberto Sassu
> > > <roberto.sassu at huaweicloud.com> wrote:
> > > > From: Roberto Sassu <roberto.sassu at huawei.com>
> > > >
> > > > BPF LSM allows security modules to directly attach to the security
> > > > hooks,
> > > > with the potential of not meeting the kernel expectation.
> > > >
> > > > This is the case for the inode_init_security hook, for which the
> > > > kernel
> > > > expects that name and value are set if the hook implementation
> > > > returns
> > > > zero.
> > > >
> > > > Consequently, not meeting the kernel expectation can cause the
> > > > kernel to
> > > > crash. One example is evm_protected_xattr_common() which expects
> > > > the
> > > > req_xattr_name parameter to be always not NULL.
> > >
> > > Sounds like a bug in evm_protected_xattr_common.
> >
> > If an LSM implementing the inode_init_security hook returns -EOPNOTSUPP
> > or -ENOMEM, evm_protected_xattr_common() is not going to be executed.
> >
> > This is documented in include/linux/lsm_hooks.h
> >
> > Why it would be a bug in evm_protected_xattr_common()?
> >
> > > > Introduce a level of indirection in BPF LSM, for the
> > > > inode_init_security
> > > > hook, to check the validity of the name and value set by security
> > > > modules.
> > >
> > > Doesn't make sense.
> >
> > Look at this example. The LSM infrastructure has a convention on return
> > values for the hooks (maybe there is something similar for other
> > hooks). The code calling the hooks relies on such conventions. If
> > conventions are not followed a panic occurs.
> >
> > If LSMs go to the kernel, their code is checked for compliance with the
> > conventions. However, this does not happen for security modules
> > attached to the BPF LSM, because BPF LSM directly executes the eBPF
> > programs without further checks.
> >
> > I was able to trigger the panic with this simple eBPF program:
> >
> > SEC("lsm/inode_init_security")
> > int BPF_PROG(test_int_hook, struct inode *inode,
> >        struct inode *dir, const struct qstr *qstr, const char **name,
> >        void **value, size_t *len)
> > {
> >       return 0;
> > }
> >
> > In my opinion, the level of indirection is necessary to ensure that
> > kernel expectations are met.
>
> I investigated further. Instead of returning zero, I return one. This
> causes a crash even with the most recent kernel (lsm=bpf):
>
> [   27.685704] BUG: kernel NULL pointer dereference, address: 00000000000000e1
> [   27.686445] #PF: supervisor read access in kernel mode
> [   27.686964] #PF: error_code(0x0000) - not-present page
> [   27.687465] PGD 0 P4D 0
> [   27.687724] Oops: 0000 [#1] PREEMPT SMP NOPTI
> [   27.688155] CPU: 9 PID: 897 Comm: in:imjournal Not tainted 6.1.0-rc2 #255
> [   27.688807] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.13.0-1ubuntu1.1 04/01/2014
> [   27.689652] RIP: 0010:fsnotify+0x71a/0x780
> [   27.690056] Code: ff 48 85 db 74 54 48 83 bb 68 04 00 00 00 74 4a 41 8b 92 98 06 00 00 4d 85 ed
> 0f 85 a6 f9 ff ff e9 ad f9 ff ff 48 8b 44 24 08 <4c> 8b 90 e0 00 00 00 e9 00 fa ff ff 48 c7 c2 b8 12
> 78 82 be 81 01
> [   27.691809] RSP: 0018:ffffc90001307ca0 EFLAGS: 00010246
> [   27.692313] RAX: 0000000000000001 RBX: 0000000000000000 RCX: ffff88811d73b4a8
> [   27.692998] RDX: 0000000000000003 RSI: 0000000000000001 RDI: 0000000000000100
> [   27.693682] RBP: ffff888100441c08 R08: 0000000000000059 R09: 0000000000000000
> [   27.694371] R10: 0000000000000000 R11: ffff88846fc72d30 R12: 0000000000000100
> [   27.695073] R13: ffff88811a2a5200 R14: ffffc90001307dc0 R15: 0000000000000001
> [   27.695738] FS:  00007ff791000640(0000) GS:ffff88846fc40000(0000) knlGS:0000000000000000
> [   27.696137] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   27.696430] CR2: 00000000000000e1 CR3: 0000000112aa6000 CR4: 0000000000350ee0
> [   27.696782] Call Trace:
> [   27.696909]  <TASK>
> [   27.697026]  path_openat+0x484/0xa00
> [   27.697218]  ? rcu_read_lock_held_common+0xe/0x50
> [   27.697461]  do_filp_open+0x9f/0xf0
> [   27.697643]  ? rcu_read_lock_sched_held+0x13/0x70
> [   27.697888]  ? lock_release+0x1e1/0x2a0
> [   27.698085]  ? _raw_spin_unlock+0x29/0x50
> [   27.698291]  do_sys_openat2+0x226/0x300
> [   27.698491]  do_sys_open+0x34/0x60
> [   27.698667]  do_syscall_64+0x3b/0x90
> [   27.698861]  entry_SYSCALL_64_after_hwframe+0x63/0xcd
>
> Beeing positive, instead of negative, the return code is converted
> to a legitimate pointer instead of an error pointer, causing a crash
> in fsnotify().

Could you point to the code that does that?

I'm looking at security_inode_init_security() and it is indeed messy.
Per file system initxattrs callback that processes kmalloc-ed strings.
Yikes.

In the short term we should denylist inode_init_security hook to
disallow attaching bpf-lsm there. set/getxattr should be done
through kfuncs instead of such kmalloc-a-string hack.