On Mon, Aug 30, 2021 at 10:41:29AM -0700, Casey Schaufler wrote: > 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? The documentation for fs_context_parse_param notes: * @fs_context_parse_param: * Userspace provided a parameter to configure a superblock. The LSM may * reject it with an error and may use it for itself, in which case it * should return 0; otherwise it should return -ENOPARAM to pass it on to * the filesystem. * @fc indicates the filesystem context. * @param The parameter So we should simply make selinux return 0 on top of your patch when it has consumed the option.
