On 8/30/2021 7:25 AM, Casey Schaufler wrote: > On 8/30/2021 5:23 AM, Christian Brauner wrote: >> On Fri, Aug 27, 2021 at 07:11:18PM -0700, syzbot wrote: >>> syzbot has bisected this issue to: >>> >>> commit 54261af473be4c5481f6196064445d2945f2bdab >>> Author: KP Singh <kpsingh@xxxxxxxxxx> >>> Date: Thu Apr 30 15:52:40 2020 +0000 >>> >>> security: Fix the default value of fs_context_parse_param hook >>> >>> bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=160c5d75300000 >>> start commit: 77dd11439b86 Merge tag 'drm-fixes-2021-08-27' of git://ano.. >>> git tree: upstream >>> final oops: https://syzkaller.appspot.com/x/report.txt?x=150c5d75300000 >>> console output: https://syzkaller.appspot.com/x/log.txt?x=110c5d75300000 >>> kernel config: https://syzkaller.appspot.com/x/.config?x=2fd902af77ff1e56 >>> dashboard link: https://syzkaller.appspot.com/bug?extid=d1e3b1d92d25abf97943 >>> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=126d084d300000 >>> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=16216eb1300000 >>> >>> Reported-by: syzbot+d1e3b1d92d25abf97943@xxxxxxxxxxxxxxxxxxxxxxxxx >>> Fixes: 54261af473be ("security: Fix the default value of fs_context_parse_param hook") >>> >>> For information about bisection process see: https://goo.gl/tpsmEJ#bisection >> So ok, this seems somewhat clear now. When smack and >> CONFIG_BPF_LSM=y >> is selected the bpf LSM will register NOP handlers including >> >> bpf_lsm_fs_context_fs_param() >> >> for the >> >> fs_context_fs_param >> >> LSM hook. The bpf LSM runs last, i.e. after smack according to: >> >> CONFIG_LSM="landlock,lockdown,yama,safesetid,integrity,tomoyo,smack,bpf" >> >> in the appended config. The smack hook runs and sets >> >> param->string = NULL >> >> then the bpf NOP handler runs returning -ENOPARM indicating to the vfs >> parameter parser that this is not a security module option so it should >> proceed processing the parameter subsequently causing the crash because >> param->string is not allowed to be NULL (Which the vfs parameter parser >> verifies early in fsconfig().). > The security_fs_context_parse_param() function is incorrectly > implemented using the call_int_hook() macro. It should return > zero if any of the modules return zero. It does not follow the > usual failure model of LSM hooks. It could be argued that the > code was fine before the addition of the BPF hook, but it was > going to fail as soon as any two security modules provided > mount options. > > Regardless, I will have a patch later today. Thank you for > tracking this down. Here's my proposed patch. I'll tidy it up with a proper commit message if it looks alright to y'all. I've tested with Smack and with and without BPF. security/security.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/security/security.c b/security/security.c index 09533cbb7221..3cf0faaf1c5b 100644 --- a/security/security.c +++ b/security/security.c @@ -885,7 +885,19 @@ int security_fs_context_dup(struct fs_context *fc, struct fs_context *src_fc) int security_fs_context_parse_param(struct fs_context *fc, struct fs_parameter *param) { - return call_int_hook(fs_context_parse_param, -ENOPARAM, fc, param); + struct security_hook_list *hp; + int trc; + int rc = -ENOPARAM; + + hlist_for_each_entry(hp, &security_hook_heads.fs_context_parse_param, + list) { + trc = hp->hook.fs_context_parse_param(fc, param); + if (trc == 0) + rc = 0; + else if (trc != -ENOPARAM) + return trc; + } + return rc; } int security_sb_alloc(struct super_block *sb) > >> If you take the appended syzkaller config and additionally select >> kprobes you can observe this by registering bpf kretprobes for: >> security_fs_context_parse_param() >> smack_fs_context_parse_param() >> bpf_lsm_fs_context_parse_param() >> in different terminal windows and then running the syzkaller provided >> reproducer: >> >> root@f2-vm:~# bpftrace -e 'kretprobe:smack_fs_context_parse_param { printf("returned: %d\n", retval); }' >> Attaching 1 probe... >> returned: 0 >> >> root@f2-vm:~# bpftrace -e 'kretprobe:bpf_lsm_fs_context_parse_param { printf("returned: %d\n", retval); }' >> Attaching 1 probe... >> returned: -519 >> >> root@f2-vm:~# bpftrace -e 'kretprobe:security_fs_context_parse_param { printf("returned: %d\n", retval); }' >> Attaching 1 probe... >> returned: -519 >> >> ^^^^^ >> This will ultimately tell the vfs to move on causing the crash because >> param->string is null at that point. >> >> Unless I missed something why that can't happen. >> >> Christian
