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. > > > 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>
