On 8/30/2021 9:57 AM, Christian Brauner wrote: > On Mon, Aug 30, 2021 at 09:40:57AM -0700, Casey Schaufler wrote: >> 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. > Looks good to me. > On question, in contrast to smack, selinux returns 1 instead of 0 on > success. So selinux would cause an early return preventing other hooks > from running. Just making sure that this is intentional. > > Iirc, this would mean that selinux causes fsconfig() to return a > positive value to userspace which I think is a bug; likely in selinux. > So I think selinux should either return 0 or the security hook itself > needs to overwrite a positive value with a sensible errno that can be > seen by userspace. I think that I agree. The SELinux and Smack versions of the hook are almost identical except for setting rc to 1 in the SELinux case. And returning 1 makes no sense if you follow the callers back. David Howells wrote both the SELinux and Smack versions. David - why are they different? which is correct? > >> >> 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) > <snip>
