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
