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(-)
From: Christian Brauner <hidden> Date: 2021-10-12 10:32:50
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 <redacted>
On Mon, Oct 11, 2021 at 03:40:22PM -0700, Casey Schaufler wrote:
quoted
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.
From: Paul Moore <paul@paul-moore.com> Date: 2022-01-25 22:18:16
On Tue, Oct 12, 2021 at 10:27 AM Casey Schaufler [off-list ref] wrote:
On 10/12/2021 3:32 AM, Christian Brauner wrote:
quoted
On Mon, Oct 11, 2021 at 03:40:22PM -0700, Casey Schaufler wrote:
quoted
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?
On Tue, Oct 12, 2021 at 10:27 AM Casey Schaufler [off-list ref] wrote:
quoted
On 10/12/2021 3:32 AM, Christian Brauner wrote:
quoted
On Mon, Oct 11, 2021 at 03:40:22PM -0700, Casey Schaufler wrote:
quoted
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.
From: Paul Moore <paul@paul-moore.com> Date: 2022-01-25 23:36:30
On Tue, Jan 25, 2022 at 6:30 PM Casey Schaufler [off-list ref] wrote:
On 1/25/2022 2:18 PM, Paul Moore wrote:
quoted
On Tue, Oct 12, 2021 at 10:27 AM Casey Schaufler [off-list ref] wrote:
quoted
On 10/12/2021 3:32 AM, Christian Brauner wrote:
quoted
On Mon, Oct 11, 2021 at 03:40:22PM -0700, Casey Schaufler wrote:
quoted
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.
From: Christian Brauner <brauner@kernel.org> Date: 2022-01-26 07:24:51
On Tue, Jan 25, 2022 at 05:18:02PM -0500, Paul Moore wrote:
On Tue, Oct 12, 2021 at 10:27 AM Casey Schaufler [off-list ref] wrote:
quoted
On 10/12/2021 3:32 AM, Christian Brauner wrote:
quoted
On Mon, Oct 11, 2021 at 03:40:22PM -0700, Casey Schaufler wrote:
quoted
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
From: Paul Moore <paul@paul-moore.com> Date: 2022-01-26 22:37:56
On Wed, Jan 26, 2022 at 2:24 AM Christian Brauner [off-list ref] wrote:
On Tue, Jan 25, 2022 at 05:18:02PM -0500, Paul Moore wrote:
quoted
On Tue, Oct 12, 2021 at 10:27 AM Casey Schaufler [off-list ref] wrote:
quoted
On 10/12/2021 3:32 AM, Christian Brauner wrote:
quoted
On Mon, Oct 11, 2021 at 03:40:22PM -0700, Casey Schaufler wrote:
quoted
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.
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(-)
From: James Morris <jmorris@namei.org> Date: 2022-01-27 17:56:41
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>
From: Paul Moore <paul@paul-moore.com> Date: 2022-01-28 01:44:52
On Thu, Jan 27, 2022 at 12:46 PM James Morris [off-list ref] wrote:
On Thu, 27 Jan 2022, Casey Schaufler wrote:
quoted
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 <redacted>
Looks good to me too, thanks Casey. Since James' already ACK'd it, I
went ahead and pulled this into selinux/next.
On Thu, Jan 27, 2022 at 12:46 PM James Morris [off-list ref] wrote:
quoted
On Thu, 27 Jan 2022, Casey Schaufler wrote:
quoted
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 <redacted>
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.
From: Christian Brauner <brauner@kernel.org> Date: 2022-01-28 08:59:23
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>