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: [email protected]
Signed-off-by: Casey Schaufler <[email protected]>
---
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)
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: [email protected]
> Signed-off-by: Casey Schaufler <[email protected]>
> ---
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 <[email protected]>
> 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)
>
>
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: [email protected]
>> Signed-off-by: Casey Schaufler <[email protected]>
>> ---
> 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 <[email protected]>
>
>> 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)
>>
>>
On Tue, Oct 12, 2021 at 10:27 AM Casey Schaufler <[email protected]> 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: [email protected]
> >> Signed-off-by: Casey Schaufler <[email protected]>
> >> ---
> > 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 <[email protected]>
... 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 <[email protected]>
> >
> >> 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
On 1/25/2022 2:18 PM, Paul Moore wrote:
> On Tue, Oct 12, 2021 at 10:27 AM Casey Schaufler <[email protected]> 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: [email protected]
>>>> Signed-off-by: Casey Schaufler <[email protected]>
>>>> ---
>>> 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 <[email protected]>
>
> ... 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 <[email protected]>
>>>
>>>> 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;
>>>> }
On Tue, Jan 25, 2022 at 6:30 PM Casey Schaufler <[email protected]> wrote:
>
> On 1/25/2022 2:18 PM, Paul Moore wrote:
> > On Tue, Oct 12, 2021 at 10:27 AM Casey Schaufler <[email protected]> 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: [email protected]
> >>>> Signed-off-by: Casey Schaufler <[email protected]>
> >>>> ---
> >>> 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 <[email protected]>
> >
> > ... 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 <[email protected]>
> >>>
> >>>> 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
On Tue, Jan 25, 2022 at 05:18:02PM -0500, Paul Moore wrote:
> On Tue, Oct 12, 2021 at 10:27 AM Casey Schaufler <[email protected]> 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: [email protected]
> > >> Signed-off-by: Casey Schaufler <[email protected]>
> > >> ---
> > > 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 <[email protected]>
>
> ... 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!
On Wed, Jan 26, 2022 at 2:24 AM Christian Brauner <[email protected]> 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 <[email protected]> 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: [email protected]
> > > >> Signed-off-by: Casey Schaufler <[email protected]>
> > > >> ---
> > > > 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 <[email protected]>
> >
> > ... 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
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: [email protected]
Signed-off-by: Casey Schaufler <[email protected]>
---
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;
}
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: [email protected]
> Signed-off-by: Casey Schaufler <[email protected]>
Acked-by: James Morris <[email protected]>
> ---
> 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
<[email protected]>
On Thu, Jan 27, 2022 at 12:46 PM James Morris <[email protected]> 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: [email protected]
> > Signed-off-by: Casey Schaufler <[email protected]>
>
>
> Acked-by: James Morris <[email protected]>
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
On 1/27/2022 5:44 PM, Paul Moore wrote:
> On Thu, Jan 27, 2022 at 12:46 PM James Morris <[email protected]> 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: [email protected]
>>> Signed-off-by: Casey Schaufler <[email protected]>
>>
>> Acked-by: James Morris <[email protected]>
> 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;
>>> }
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: [email protected]
> Signed-off-by: Casey Schaufler <[email protected]>
> ---
Looks good,
Acked-by: Christian Brauner <[email protected]>