Messages in this thread | | | Date | Tue, 25 Jan 2022 15:30:48 -0800 | Subject | Re: [PATCH] LSM: general protection fault in legacy_parse_param | From | 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; >>>> }
| |