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

[Roberto Sassu]

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@xxxxxxxxxxxxxxx> wrote:
> > > From: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
> > > 
> > > 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().

Roberto