Re: [syzbot] general protection fault in legacy_parse_param

Re: [syzbot] general protection fault in legacy_parse_param

[syzbot]

syzbot has bisected this issue to:

commit 54261af473be4c5481f6196064445d2945f2bdab
Author: KP Singh <kpsingh@google.com>
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@syzkaller.appspotmail.com
Fixes: 54261af473be ("security: Fix the default value of fs_context_parse_param hook")

For information about bisection process see: https://goo.gl/tpsmEJ#bisection

Re: [syzbot] general protection fault in legacy_parse_param

[Christian Brauner]

On Fri, Aug 27, 2021 at 07:11:18PM -0700, syzbot wrote:
> syzbot has bisected this issue to:
> 
> commit 54261af473be4c5481f6196064445d2945f2bdab
> Author: KP Singh <kpsingh@google.com>
> 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@syzkaller.appspotmail.com
> 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().).

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

Re: [syzbot] general protection fault in legacy_parse_param

[Casey Schaufler]

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@google.com>
>> 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@syzkaller.appspotmail.com
>> 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.

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

Re: [syzbot] general protection fault in legacy_parse_param

[Casey Schaufler]

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@google.com>
>>> 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@syzkaller.appspotmail.com
>>> 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

Re: [syzbot] general protection fault in legacy_parse_param

[Christian Brauner]

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@google.com>
> >>> 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@syzkaller.appspotmail.com
> >>> 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>

Re: [syzbot] general protection fault in legacy_parse_param

[Casey Schaufler]

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@google.com>
>>>>> 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@syzkaller.appspotmail.com
>>>>> 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>

Re: [syzbot] general protection fault in legacy_parse_param

[Christian Brauner]

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@google.com>
> >>>>> 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@syzkaller.appspotmail.com
> >>>>> 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.