* Re: [PATCH] LSM: general protection fault in legacy_parse_param
2022-01-25 22:18 ` Paul Moore
@ 2022-01-25 23:30 ` Casey Schaufler
2022-01-25 23:36 ` Paul Moore
2022-01-26 7:24 ` Christian Brauner
2022-01-27 16:51 ` [PATCH v2] " Casey Schaufler
2 siblings, 1 reply; 13+ messages in thread
From: Casey Schaufler @ 2022-01-25 23:30 UTC (permalink / raw)
To: Paul Moore
Cc: Christian Brauner, Christian Brauner, James Morris,
Linux Security Module list, LKML, syzbot, David Howells,
linux-fsdevel, selinux, 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;
>>>> }
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] LSM: general protection fault in legacy_parse_param
2022-01-25 23:30 ` Casey Schaufler
@ 2022-01-25 23:36 ` Paul Moore
0 siblings, 0 replies; 13+ messages in thread
From: Paul Moore @ 2022-01-25 23:36 UTC (permalink / raw)
To: Casey Schaufler
Cc: Christian Brauner, Christian Brauner, James Morris,
Linux Security Module list, LKML, syzbot, David Howells,
linux-fsdevel, selinux
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] LSM: general protection fault in legacy_parse_param
2022-01-25 22:18 ` Paul Moore
2022-01-25 23:30 ` Casey Schaufler
@ 2022-01-26 7:24 ` Christian Brauner
2022-01-26 22:37 ` Paul Moore
2022-01-27 16:51 ` [PATCH v2] " Casey Schaufler
2 siblings, 1 reply; 13+ messages in thread
From: Christian Brauner @ 2022-01-26 7:24 UTC (permalink / raw)
To: Paul Moore
Cc: Casey Schaufler, Christian Brauner, Christian Brauner,
James Morris, Linux Security Module list, LKML, syzbot,
David Howells, linux-fsdevel, selinux
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!
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] LSM: general protection fault in legacy_parse_param
2022-01-26 7:24 ` Christian Brauner
@ 2022-01-26 22:37 ` Paul Moore
0 siblings, 0 replies; 13+ messages in thread
From: Paul Moore @ 2022-01-26 22:37 UTC (permalink / raw)
To: Christian Brauner
Cc: Casey Schaufler, Christian Brauner, Christian Brauner,
James Morris, Linux Security Module list, LKML, syzbot,
David Howells, linux-fsdevel, selinux
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2] LSM: general protection fault in legacy_parse_param
2022-01-25 22:18 ` Paul Moore
2022-01-25 23:30 ` Casey Schaufler
2022-01-26 7:24 ` Christian Brauner
@ 2022-01-27 16:51 ` Casey Schaufler
2022-01-27 17:33 ` James Morris
2022-01-28 8:59 ` Christian Brauner
2 siblings, 2 replies; 13+ messages in thread
From: Casey Schaufler @ 2022-01-27 16:51 UTC (permalink / raw)
To: Paul Moore
Cc: Christian Brauner, Christian Brauner, James Morris,
Linux Security Module list, LKML, syzbot, David Howells,
linux-fsdevel, selinux, 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;
}
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v2] LSM: general protection fault in legacy_parse_param
2022-01-27 16:51 ` [PATCH v2] " Casey Schaufler
@ 2022-01-27 17:33 ` James Morris
2022-01-28 1:44 ` Paul Moore
2022-01-28 8:59 ` Christian Brauner
1 sibling, 1 reply; 13+ messages in thread
From: James Morris @ 2022-01-27 17:33 UTC (permalink / raw)
To: Casey Schaufler
Cc: Paul Moore, Christian Brauner, Christian Brauner,
Linux Security Module list, LKML, syzbot, David Howells,
linux-fsdevel, selinux
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>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v2] LSM: general protection fault in legacy_parse_param
2022-01-27 17:33 ` James Morris
@ 2022-01-28 1:44 ` Paul Moore
2022-01-28 2:33 ` Casey Schaufler
0 siblings, 1 reply; 13+ messages in thread
From: Paul Moore @ 2022-01-28 1:44 UTC (permalink / raw)
To: James Morris
Cc: Casey Schaufler, Christian Brauner, Christian Brauner,
Linux Security Module list, LKML, syzbot, David Howells,
linux-fsdevel, selinux
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
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v2] LSM: general protection fault in legacy_parse_param
2022-01-28 1:44 ` Paul Moore
@ 2022-01-28 2:33 ` Casey Schaufler
0 siblings, 0 replies; 13+ messages in thread
From: Casey Schaufler @ 2022-01-28 2:33 UTC (permalink / raw)
To: Paul Moore, James Morris
Cc: Christian Brauner, Christian Brauner, Linux Security Module list,
LKML, syzbot, David Howells, linux-fsdevel, selinux,
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;
>>> }
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] LSM: general protection fault in legacy_parse_param
2022-01-27 16:51 ` [PATCH v2] " Casey Schaufler
2022-01-27 17:33 ` James Morris
@ 2022-01-28 8:59 ` Christian Brauner
1 sibling, 0 replies; 13+ messages in thread
From: Christian Brauner @ 2022-01-28 8:59 UTC (permalink / raw)
To: Casey Schaufler
Cc: Paul Moore, Christian Brauner, Christian Brauner, James Morris,
Linux Security Module list, LKML, syzbot, David Howells,
linux-fsdevel, selinux
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>
^ permalink raw reply [flat|nested] 13+ messages in thread