| Message ID | 018a9bb4-accb-c19a-5b0a-fde22f4bc822@schaufler-ca.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | LSM: general protection fault in legacy_parse_param | expand |
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) > >
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) >> >>
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; > >> }
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; >>>> }
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; > >>>> }
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!
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.
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)
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(-)