| Message ID | a19e0338-5240-4a6d-aecf-145539aecbce@schaufler-ca.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | [v2] LSM: general protection fault in legacy_parse_param | expand |
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; > } > > >
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; > > }
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; >>> }
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>
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; }
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(-)