[PATCH] LSM: general protection fault in legacy_parse_param

[PATCH] LSM: general protection fault in legacy_parse_param

[Casey Schaufler]

The usual LSM hook "bail on fail" scheme doesn't work for cases where
a security module may return an error code indicating that it does not
recognize an input.  In this particular case Smack sees a mount option
that it recognizes, and returns 0. A call to a BPF hook follows, which
returns -ENOPARAM, which confuses the caller because Smack has processed
its data.

Reported-by: syzbot+d1e3b1d92d25abf97943@syzkaller.appspotmail.com
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
---
 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)

Re: [PATCH] LSM: general protection fault in legacy_parse_param

[Christian Brauner]

On Mon, Oct 11, 2021 at 03:40:22PM -0700, Casey Schaufler wrote:
> The usual LSM hook "bail on fail" scheme doesn't work for cases where
> a security module may return an error code indicating that it does not
> recognize an input.  In this particular case Smack sees a mount option
> that it recognizes, and returns 0. A call to a BPF hook follows, which
> returns -ENOPARAM, which confuses the caller because Smack has processed
> its data.
> 
> Reported-by: syzbot+d1e3b1d92d25abf97943@syzkaller.appspotmail.com
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> ---

Thanks!
Note, I think that we still have the SELinux issue we discussed in the
other thread:

	rc = selinux_add_opt(opt, param->string, &fc->security);
	if (!rc) {
		param->string = NULL;
		rc = 1;
	}

SELinux returns 1 not the expected 0. Not sure if that got fixed or is
queued-up for -next. In any case, this here seems correct independent of
that:

Acked-by: Christian Brauner <christian.brauner@ubuntu.com>

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

Re: [PATCH] LSM: general protection fault in legacy_parse_param

[Casey Schaufler]

On 10/12/2021 3:32 AM, Christian Brauner wrote:
> On Mon, Oct 11, 2021 at 03:40:22PM -0700, Casey Schaufler wrote:
>> The usual LSM hook "bail on fail" scheme doesn't work for cases where
>> a security module may return an error code indicating that it does not
>> recognize an input.  In this particular case Smack sees a mount option
>> that it recognizes, and returns 0. A call to a BPF hook follows, which
>> returns -ENOPARAM, which confuses the caller because Smack has processed
>> its data.
>>
>> Reported-by: syzbot+d1e3b1d92d25abf97943@syzkaller.appspotmail.com
>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>> ---
> Thanks!
> Note, I think that we still have the SELinux issue we discussed in the
> other thread:
>
> 	rc = selinux_add_opt(opt, param->string, &fc->security);
> 	if (!rc) {
> 		param->string = NULL;
> 		rc = 1;
> 	}
>
> SELinux returns 1 not the expected 0. Not sure if that got fixed or is
> queued-up for -next. In any case, this here seems correct independent of
> that:

The aforementioned SELinux change depends on this patch. As the SELinux
code is today it blocks the problem seen with Smack, but introduces a
different issue. It prevents the BPF hook from being called.

So the question becomes whether the SELinux change should be included
here, or done separately. Without the security_fs_context_parse_param()
change the selinux_fs_context_parse_param() change results in messy
failures for SELinux mounts. 


>
> Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
>
>>  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)
>>
>>

Re: [PATCH] LSM: general protection fault in legacy_parse_param

[Paul Moore]

On Tue, Oct 12, 2021 at 10:27 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 10/12/2021 3:32 AM, Christian Brauner wrote:
> > On Mon, Oct 11, 2021 at 03:40:22PM -0700, Casey Schaufler wrote:
> >> The usual LSM hook "bail on fail" scheme doesn't work for cases where
> >> a security module may return an error code indicating that it does not
> >> recognize an input.  In this particular case Smack sees a mount option
> >> that it recognizes, and returns 0. A call to a BPF hook follows, which
> >> returns -ENOPARAM, which confuses the caller because Smack has processed
> >> its data.
> >>
> >> Reported-by: syzbot+d1e3b1d92d25abf97943@syzkaller.appspotmail.com
> >> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> >> ---
> > Thanks!
> > Note, I think that we still have the SELinux issue we discussed in the
> > other thread:
> >
> >       rc = selinux_add_opt(opt, param->string, &fc->security);
> >       if (!rc) {
> >               param->string = NULL;
> >               rc = 1;
> >       }
> >
> > SELinux returns 1 not the expected 0. Not sure if that got fixed or is
> > queued-up for -next. In any case, this here seems correct independent of
> > that:
>
> The aforementioned SELinux change depends on this patch. As the SELinux
> code is today it blocks the problem seen with Smack, but introduces a
> different issue. It prevents the BPF hook from being called.
>
> So the question becomes whether the SELinux change should be included
> here, or done separately. Without the security_fs_context_parse_param()
> change the selinux_fs_context_parse_param() change results in messy
> failures for SELinux mounts.

FWIW, this patch looks good to me, so:

Acked-by: Paul Moore <paul@paul-moore.com>

... and with respect to the SELinux hook implementation returning 1 on
success, I don't have a good answer and looking through my inbox I see
David Howells hasn't responded either.  I see nothing in the original
commit explaining why, so I'm going to say let's just change it to
zero and be done with it; the good news is that if we do it now we've
got almost a full cycle in linux-next to see what falls apart.  As far
as the question of one vs two patches, it might be good to put both
changes into a single patch just so that folks who do backports don't
accidentally skip one and create a bad kernel build.  Casey, did you
want to respin this patch or would you prefer me to submit another
version?

> > Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
> >
> >>  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;
> >>  }

-- 
paul-moore.com

Re: [PATCH] LSM: general protection fault in legacy_parse_param

[Casey Schaufler]

On 1/25/2022 2:18 PM, Paul Moore wrote:
> On Tue, Oct 12, 2021 at 10:27 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 10/12/2021 3:32 AM, Christian Brauner wrote:
>>> On Mon, Oct 11, 2021 at 03:40:22PM -0700, Casey Schaufler wrote:
>>>> The usual LSM hook "bail on fail" scheme doesn't work for cases where
>>>> a security module may return an error code indicating that it does not
>>>> recognize an input.  In this particular case Smack sees a mount option
>>>> that it recognizes, and returns 0. A call to a BPF hook follows, which
>>>> returns -ENOPARAM, which confuses the caller because Smack has processed
>>>> its data.
>>>>
>>>> Reported-by: syzbot+d1e3b1d92d25abf97943@syzkaller.appspotmail.com
>>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>>>> ---
>>> Thanks!
>>> Note, I think that we still have the SELinux issue we discussed in the
>>> other thread:
>>>
>>>        rc = selinux_add_opt(opt, param->string, &fc->security);
>>>        if (!rc) {
>>>                param->string = NULL;
>>>                rc = 1;
>>>        }
>>>
>>> SELinux returns 1 not the expected 0. Not sure if that got fixed or is
>>> queued-up for -next. In any case, this here seems correct independent of
>>> that:
>> The aforementioned SELinux change depends on this patch. As the SELinux
>> code is today it blocks the problem seen with Smack, but introduces a
>> different issue. It prevents the BPF hook from being called.
>>
>> So the question becomes whether the SELinux change should be included
>> here, or done separately. Without the security_fs_context_parse_param()
>> change the selinux_fs_context_parse_param() change results in messy
>> failures for SELinux mounts.
> FWIW, this patch looks good to me, so:
>
> Acked-by: Paul Moore <paul@paul-moore.com>
>
> ... and with respect to the SELinux hook implementation returning 1 on
> success, I don't have a good answer and looking through my inbox I see
> David Howells hasn't responded either.  I see nothing in the original
> commit explaining why, so I'm going to say let's just change it to
> zero and be done with it; the good news is that if we do it now we've
> got almost a full cycle in linux-next to see what falls apart.  As far
> as the question of one vs two patches, it might be good to put both
> changes into a single patch just so that folks who do backports don't
> accidentally skip one and create a bad kernel build.  Casey, did you
> want to respin this patch or would you prefer me to submit another
> version?

I can create a single patch. I tried the combination on Fedora
and it worked just fine. I'll rebase and resend.

>
>>> Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
>>>
>>>>   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;
>>>>   }

Re: [PATCH] LSM: general protection fault in legacy_parse_param

[Paul Moore]

On Tue, Jan 25, 2022 at 6:30 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> On 1/25/2022 2:18 PM, Paul Moore wrote:
> > On Tue, Oct 12, 2021 at 10:27 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >> On 10/12/2021 3:32 AM, Christian Brauner wrote:
> >>> On Mon, Oct 11, 2021 at 03:40:22PM -0700, Casey Schaufler wrote:
> >>>> The usual LSM hook "bail on fail" scheme doesn't work for cases where
> >>>> a security module may return an error code indicating that it does not
> >>>> recognize an input.  In this particular case Smack sees a mount option
> >>>> that it recognizes, and returns 0. A call to a BPF hook follows, which
> >>>> returns -ENOPARAM, which confuses the caller because Smack has processed
> >>>> its data.
> >>>>
> >>>> Reported-by: syzbot+d1e3b1d92d25abf97943@syzkaller.appspotmail.com
> >>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> >>>> ---
> >>> Thanks!
> >>> Note, I think that we still have the SELinux issue we discussed in the
> >>> other thread:
> >>>
> >>>        rc = selinux_add_opt(opt, param->string, &fc->security);
> >>>        if (!rc) {
> >>>                param->string = NULL;
> >>>                rc = 1;
> >>>        }
> >>>
> >>> SELinux returns 1 not the expected 0. Not sure if that got fixed or is
> >>> queued-up for -next. In any case, this here seems correct independent of
> >>> that:
> >> The aforementioned SELinux change depends on this patch. As the SELinux
> >> code is today it blocks the problem seen with Smack, but introduces a
> >> different issue. It prevents the BPF hook from being called.
> >>
> >> So the question becomes whether the SELinux change should be included
> >> here, or done separately. Without the security_fs_context_parse_param()
> >> change the selinux_fs_context_parse_param() change results in messy
> >> failures for SELinux mounts.
> > FWIW, this patch looks good to me, so:
> >
> > Acked-by: Paul Moore <paul@paul-moore.com>
> >
> > ... and with respect to the SELinux hook implementation returning 1 on
> > success, I don't have a good answer and looking through my inbox I see
> > David Howells hasn't responded either.  I see nothing in the original
> > commit explaining why, so I'm going to say let's just change it to
> > zero and be done with it; the good news is that if we do it now we've
> > got almost a full cycle in linux-next to see what falls apart.  As far
> > as the question of one vs two patches, it might be good to put both
> > changes into a single patch just so that folks who do backports don't
> > accidentally skip one and create a bad kernel build.  Casey, did you
> > want to respin this patch or would you prefer me to submit another
> > version?
>
> I can create a single patch. I tried the combination on Fedora
> and it worked just fine. I'll rebase and resend.

Great, thank you.

> >>> Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
> >>>
> >>>>   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;
> >>>>   }

-- 
paul-moore.com

Re: [PATCH] LSM: general protection fault in legacy_parse_param

[Christian Brauner]

On Tue, Jan 25, 2022 at 05:18:02PM -0500, Paul Moore wrote:
> On Tue, Oct 12, 2021 at 10:27 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > On 10/12/2021 3:32 AM, Christian Brauner wrote:
> > > On Mon, Oct 11, 2021 at 03:40:22PM -0700, Casey Schaufler wrote:
> > >> The usual LSM hook "bail on fail" scheme doesn't work for cases where
> > >> a security module may return an error code indicating that it does not
> > >> recognize an input.  In this particular case Smack sees a mount option
> > >> that it recognizes, and returns 0. A call to a BPF hook follows, which
> > >> returns -ENOPARAM, which confuses the caller because Smack has processed
> > >> its data.
> > >>
> > >> Reported-by: syzbot+d1e3b1d92d25abf97943@syzkaller.appspotmail.com
> > >> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> > >> ---
> > > Thanks!
> > > Note, I think that we still have the SELinux issue we discussed in the
> > > other thread:
> > >
> > >       rc = selinux_add_opt(opt, param->string, &fc->security);
> > >       if (!rc) {
> > >               param->string = NULL;
> > >               rc = 1;
> > >       }
> > >
> > > SELinux returns 1 not the expected 0. Not sure if that got fixed or is
> > > queued-up for -next. In any case, this here seems correct independent of
> > > that:
> >
> > The aforementioned SELinux change depends on this patch. As the SELinux
> > code is today it blocks the problem seen with Smack, but introduces a
> > different issue. It prevents the BPF hook from being called.
> >
> > So the question becomes whether the SELinux change should be included
> > here, or done separately. Without the security_fs_context_parse_param()
> > change the selinux_fs_context_parse_param() change results in messy
> > failures for SELinux mounts.
> 
> FWIW, this patch looks good to me, so:
> 
> Acked-by: Paul Moore <paul@paul-moore.com>
> 
> ... and with respect to the SELinux hook implementation returning 1 on
> success, I don't have a good answer and looking through my inbox I see
> David Howells hasn't responded either.  I see nothing in the original
> commit explaining why, so I'm going to say let's just change it to
> zero and be done with it; the good news is that if we do it now we've


It was originally supposed to return 1 but then this got changed but - a
classic - the documentation wasn't.

> got almost a full cycle in linux-next to see what falls apart.  As far

Sweet!

Re: [PATCH] LSM: general protection fault in legacy_parse_param

[Paul Moore]

On Wed, Jan 26, 2022 at 2:24 AM Christian Brauner <brauner@kernel.org> wrote:
>
> On Tue, Jan 25, 2022 at 05:18:02PM -0500, Paul Moore wrote:
> > On Tue, Oct 12, 2021 at 10:27 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > > On 10/12/2021 3:32 AM, Christian Brauner wrote:
> > > > On Mon, Oct 11, 2021 at 03:40:22PM -0700, Casey Schaufler wrote:
> > > >> The usual LSM hook "bail on fail" scheme doesn't work for cases where
> > > >> a security module may return an error code indicating that it does not
> > > >> recognize an input.  In this particular case Smack sees a mount option
> > > >> that it recognizes, and returns 0. A call to a BPF hook follows, which
> > > >> returns -ENOPARAM, which confuses the caller because Smack has processed
> > > >> its data.
> > > >>
> > > >> Reported-by: syzbot+d1e3b1d92d25abf97943@syzkaller.appspotmail.com
> > > >> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> > > >> ---
> > > > Thanks!
> > > > Note, I think that we still have the SELinux issue we discussed in the
> > > > other thread:
> > > >
> > > >       rc = selinux_add_opt(opt, param->string, &fc->security);
> > > >       if (!rc) {
> > > >               param->string = NULL;
> > > >               rc = 1;
> > > >       }
> > > >
> > > > SELinux returns 1 not the expected 0. Not sure if that got fixed or is
> > > > queued-up for -next. In any case, this here seems correct independent of
> > > > that:
> > >
> > > The aforementioned SELinux change depends on this patch. As the SELinux
> > > code is today it blocks the problem seen with Smack, but introduces a
> > > different issue. It prevents the BPF hook from being called.
> > >
> > > So the question becomes whether the SELinux change should be included
> > > here, or done separately. Without the security_fs_context_parse_param()
> > > change the selinux_fs_context_parse_param() change results in messy
> > > failures for SELinux mounts.
> >
> > FWIW, this patch looks good to me, so:
> >
> > Acked-by: Paul Moore <paul@paul-moore.com>
> >
> > ... and with respect to the SELinux hook implementation returning 1 on
> > success, I don't have a good answer and looking through my inbox I see
> > David Howells hasn't responded either.  I see nothing in the original
> > commit explaining why, so I'm going to say let's just change it to
> > zero and be done with it; the good news is that if we do it now we've
>
>
> It was originally supposed to return 1 but then this got changed but - a
> classic - the documentation wasn't.

I'm shocked! :)

Thanks Christian.

-- 
paul-moore.com

[PATCH v2] LSM: general protection fault in legacy_parse_param

[Casey Schaufler]

The usual LSM hook "bail on fail" scheme doesn't work for cases where
a security module may return an error code indicating that it does not
recognize an input.  In this particular case Smack sees a mount option
that it recognizes, and returns 0. A call to a BPF hook follows, which
returns -ENOPARAM, which confuses the caller because Smack has processed
its data.

The SELinux hook incorrectly returns 1 on success. There was a time
when this was correct, however the current expectation is that it
return 0 on success. This is repaired.

Reported-by: syzbot+d1e3b1d92d25abf97943@syzkaller.appspotmail.com
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
---
  security/security.c      | 17 +++++++++++++++--
  security/selinux/hooks.c |  5 ++---
  2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/security/security.c b/security/security.c
index 3d4eb474f35b..e649c8691be2 100644
--- a/security/security.c
+++ b/security/security.c
@@ -884,9 +884,22 @@ int security_fs_context_dup(struct fs_context *fc, struct fs_context *src_fc)
  	return call_int_hook(fs_context_dup, 0, fc, src_fc);
  }
  
-int security_fs_context_parse_param(struct fs_context *fc, struct fs_parameter *param)
+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)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 5b6895e4fc29..371f67a37f9a 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2860,10 +2860,9 @@ static int selinux_fs_context_parse_param(struct fs_context *fc,
  		return opt;
  
  	rc = selinux_add_opt(opt, param->string, &fc->security);
-	if (!rc) {
+	if (!rc)
  		param->string = NULL;
-		rc = 1;
-	}
+
  	return rc;
  }
  

Re: [PATCH v2] LSM: general protection fault in legacy_parse_param

[James Morris]

On Thu, 27 Jan 2022, Casey Schaufler wrote:

> The usual LSM hook "bail on fail" scheme doesn't work for cases where
> a security module may return an error code indicating that it does not
> recognize an input.  In this particular case Smack sees a mount option
> that it recognizes, and returns 0. A call to a BPF hook follows, which
> returns -ENOPARAM, which confuses the caller because Smack has processed
> its data.
> 
> The SELinux hook incorrectly returns 1 on success. There was a time
> when this was correct, however the current expectation is that it
> return 0 on success. This is repaired.
> 
> Reported-by: syzbot+d1e3b1d92d25abf97943@syzkaller.appspotmail.com
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>


Acked-by: James Morris <jamorris@linux.microsoft.com>

> ---
>  security/security.c      | 17 +++++++++++++++--
>  security/selinux/hooks.c |  5 ++---
>  2 files changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/security/security.c b/security/security.c
> index 3d4eb474f35b..e649c8691be2 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -884,9 +884,22 @@ int security_fs_context_dup(struct fs_context *fc, struct
> fs_context *src_fc)
>  	return call_int_hook(fs_context_dup, 0, fc, src_fc);
>  }
>  
> -int security_fs_context_parse_param(struct fs_context *fc, struct
> fs_parameter *param)
> +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)
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 5b6895e4fc29..371f67a37f9a 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2860,10 +2860,9 @@ static int selinux_fs_context_parse_param(struct
> fs_context *fc,
>  		return opt;
>  
>  	rc = selinux_add_opt(opt, param->string, &fc->security);
> -	if (!rc) {
> +	if (!rc)
>  		param->string = NULL;
> -		rc = 1;
> -	}
> +
>  	return rc;
>  }
>  
> 
> 

-- 
James Morris
<jmorris@namei.org>

Re: [PATCH v2] LSM: general protection fault in legacy_parse_param

[Paul Moore]

On Thu, Jan 27, 2022 at 12:46 PM James Morris <jmorris@namei.org> wrote:
> On Thu, 27 Jan 2022, Casey Schaufler wrote:
>
> > The usual LSM hook "bail on fail" scheme doesn't work for cases where
> > a security module may return an error code indicating that it does not
> > recognize an input.  In this particular case Smack sees a mount option
> > that it recognizes, and returns 0. A call to a BPF hook follows, which
> > returns -ENOPARAM, which confuses the caller because Smack has processed
> > its data.
> >
> > The SELinux hook incorrectly returns 1 on success. There was a time
> > when this was correct, however the current expectation is that it
> > return 0 on success. This is repaired.
> >
> > Reported-by: syzbot+d1e3b1d92d25abf97943@syzkaller.appspotmail.com
> > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>
>
> Acked-by: James Morris <jamorris@linux.microsoft.com>

Looks good to me too, thanks Casey.  Since James' already ACK'd it, I
went ahead and pulled this into selinux/next.

> > ---
> >  security/security.c      | 17 +++++++++++++++--
> >  security/selinux/hooks.c |  5 ++---
> >  2 files changed, 17 insertions(+), 5 deletions(-)
> >
> > diff --git a/security/security.c b/security/security.c
> > index 3d4eb474f35b..e649c8691be2 100644
> > --- a/security/security.c
> > +++ b/security/security.c
> > @@ -884,9 +884,22 @@ int security_fs_context_dup(struct fs_context *fc, struct
> > fs_context *src_fc)
> >       return call_int_hook(fs_context_dup, 0, fc, src_fc);
> >  }
> >
> > -int security_fs_context_parse_param(struct fs_context *fc, struct
> > fs_parameter *param)
> > +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)
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 5b6895e4fc29..371f67a37f9a 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -2860,10 +2860,9 @@ static int selinux_fs_context_parse_param(struct
> > fs_context *fc,
> >               return opt;
> >
> >       rc = selinux_add_opt(opt, param->string, &fc->security);
> > -     if (!rc) {
> > +     if (!rc)
> >               param->string = NULL;
> > -             rc = 1;
> > -     }
> > +
> >       return rc;
> >  }

-- 
paul-moore.com

Re: [PATCH v2] LSM: general protection fault in legacy_parse_param

[Casey Schaufler]

On 1/27/2022 5:44 PM, Paul Moore wrote:
> On Thu, Jan 27, 2022 at 12:46 PM James Morris <jmorris@namei.org> wrote:
>> On Thu, 27 Jan 2022, Casey Schaufler wrote:
>>
>>> The usual LSM hook "bail on fail" scheme doesn't work for cases where
>>> a security module may return an error code indicating that it does not
>>> recognize an input.  In this particular case Smack sees a mount option
>>> that it recognizes, and returns 0. A call to a BPF hook follows, which
>>> returns -ENOPARAM, which confuses the caller because Smack has processed
>>> its data.
>>>
>>> The SELinux hook incorrectly returns 1 on success. There was a time
>>> when this was correct, however the current expectation is that it
>>> return 0 on success. This is repaired.
>>>
>>> Reported-by: syzbot+d1e3b1d92d25abf97943@syzkaller.appspotmail.com
>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>>
>> Acked-by: James Morris <jamorris@linux.microsoft.com>
> Looks good to me too, thanks Casey.  Since James' already ACK'd it, I
> went ahead and pulled this into selinux/next.

Works for me. It was either James' tree or the SELinux tree.
Going through the security tree may have made more sense because
the original problem was reported against Smack, but that tree
isn't very active.

>
>>> ---
>>>   security/security.c      | 17 +++++++++++++++--
>>>   security/selinux/hooks.c |  5 ++---
>>>   2 files changed, 17 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/security/security.c b/security/security.c
>>> index 3d4eb474f35b..e649c8691be2 100644
>>> --- a/security/security.c
>>> +++ b/security/security.c
>>> @@ -884,9 +884,22 @@ int security_fs_context_dup(struct fs_context *fc, struct
>>> fs_context *src_fc)
>>>        return call_int_hook(fs_context_dup, 0, fc, src_fc);
>>>   }
>>>
>>> -int security_fs_context_parse_param(struct fs_context *fc, struct
>>> fs_parameter *param)
>>> +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)
>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>>> index 5b6895e4fc29..371f67a37f9a 100644
>>> --- a/security/selinux/hooks.c
>>> +++ b/security/selinux/hooks.c
>>> @@ -2860,10 +2860,9 @@ static int selinux_fs_context_parse_param(struct
>>> fs_context *fc,
>>>                return opt;
>>>
>>>        rc = selinux_add_opt(opt, param->string, &fc->security);
>>> -     if (!rc) {
>>> +     if (!rc)
>>>                param->string = NULL;
>>> -             rc = 1;
>>> -     }
>>> +
>>>        return rc;
>>>   }

Re: [PATCH v2] LSM: general protection fault in legacy_parse_param

[Christian Brauner]

On Thu, Jan 27, 2022 at 08:51:44AM -0800, Casey Schaufler wrote:
> The usual LSM hook "bail on fail" scheme doesn't work for cases where
> a security module may return an error code indicating that it does not
> recognize an input.  In this particular case Smack sees a mount option
> that it recognizes, and returns 0. A call to a BPF hook follows, which
> returns -ENOPARAM, which confuses the caller because Smack has processed
> its data.
> 
> The SELinux hook incorrectly returns 1 on success. There was a time
> when this was correct, however the current expectation is that it
> return 0 on success. This is repaired.
> 
> Reported-by: syzbot+d1e3b1d92d25abf97943@syzkaller.appspotmail.com
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> ---

Looks good,
Acked-by: Christian Brauner <brauner@kernel.org>