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