| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <3354839e-5e7a-08c7-277a-9bbebfbfc0bc@schaufler-ca.com>
Date: Mon, 30 Aug 2021 10:41:29 -0700
From: Casey Schaufler <casey@...aufler-ca.com>
To: Christian Brauner <christian.brauner@...ntu.com>,
David Howells <dhowells@...hat.com>
Cc: syzbot <syzbot+d1e3b1d92d25abf97943@...kaller.appspotmail.com>,
andriin@...com, ast@...nel.org, bpf@...r.kernel.org,
daniel@...earbox.net, dvyukov@...gle.com, jmorris@...ei.org,
kafai@...com, kpsingh@...gle.com, linux-fsdevel@...r.kernel.org,
linux-kernel@...r.kernel.org,
linux-security-module@...r.kernel.org, netdev@...r.kernel.org,
paul@...l-moore.com, selinux@...r.kernel.org,
songliubraving@...com, stephen.smalley.work@...il.com,
syzkaller-bugs@...glegroups.com, tonymarislogistics@...dex.com,
viro@...iv.linux.org.uk, yhs@...com
Subject: Re: [syzbot] general protection fault in legacy_parse_param
On 8/30/2021 9:57 AM, Christian Brauner wrote:
> On Mon, Aug 30, 2021 at 09:40:57AM -0700, Casey Schaufler wrote:
>> On 8/30/2021 7:25 AM, Casey Schaufler wrote:
>>> On 8/30/2021 5:23 AM, Christian Brauner wrote:
>>>> On Fri, Aug 27, 2021 at 07:11:18PM -0700, syzbot wrote:
>>>>> syzbot has bisected this issue to:
>>>>>
>>>>> commit 54261af473be4c5481f6196064445d2945f2bdab
>>>>> Author: KP Singh <kpsingh@...gle.com>
>>>>> Date: Thu Apr 30 15:52:40 2020 +0000
>>>>>
>>>>> security: Fix the default value of fs_context_parse_param hook
>>>>>
>>>>> bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=160c5d75300000
>>>>> start commit: 77dd11439b86 Merge tag 'drm-fixes-2021-08-27' of git://ano..
>>>>> git tree: upstream
>>>>> final oops: https://syzkaller.appspot.com/x/report.txt?x=150c5d75300000
>>>>> console output: https://syzkaller.appspot.com/x/log.txt?x=110c5d75300000
>>>>> kernel config: https://syzkaller.appspot.com/x/.config?x=2fd902af77ff1e56
>>>>> dashboard link: https://syzkaller.appspot.com/bug?extid=d1e3b1d92d25abf97943
>>>>> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=126d084d300000
>>>>> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=16216eb1300000
>>>>>
>>>>> Reported-by: syzbot+d1e3b1d92d25abf97943@...kaller.appspotmail.com
>>>>> Fixes: 54261af473be ("security: Fix the default value of fs_context_parse_param hook")
>>>>>
>>>>> For information about bisection process see: https://goo.gl/tpsmEJ#bisection
>>>> So ok, this seems somewhat clear now. When smack and
>>>> CONFIG_BPF_LSM=y
>>>> is selected the bpf LSM will register NOP handlers including
>>>>
>>>> bpf_lsm_fs_context_fs_param()
>>>>
>>>> for the
>>>>
>>>> fs_context_fs_param
>>>>
>>>> LSM hook. The bpf LSM runs last, i.e. after smack according to:
>>>>
>>>> CONFIG_LSM="landlock,lockdown,yama,safesetid,integrity,tomoyo,smack,bpf"
>>>>
>>>> in the appended config. The smack hook runs and sets
>>>>
>>>> param->string = NULL
>>>>
>>>> then the bpf NOP handler runs returning -ENOPARM indicating to the vfs
>>>> parameter parser that this is not a security module option so it should
>>>> proceed processing the parameter subsequently causing the crash because
>>>> param->string is not allowed to be NULL (Which the vfs parameter parser
>>>> verifies early in fsconfig().).
>>> The security_fs_context_parse_param() function is incorrectly
>>> implemented using the call_int_hook() macro. It should return
>>> zero if any of the modules return zero. It does not follow the
>>> usual failure model of LSM hooks. It could be argued that the
>>> code was fine before the addition of the BPF hook, but it was
>>> going to fail as soon as any two security modules provided
>>> mount options.
>>>
>>> Regardless, I will have a patch later today. Thank you for
>>> tracking this down.
>> Here's my proposed patch. I'll tidy it up with a proper
>> commit message if it looks alright to y'all. I've tested
>> with Smack and with and without BPF.
> Looks good to me.
> On question, in contrast to smack, selinux returns 1 instead of 0 on
> success. So selinux would cause an early return preventing other hooks
> from running. Just making sure that this is intentional.
>
> Iirc, this would mean that selinux causes fsconfig() to return a
> positive value to userspace which I think is a bug; likely in selinux.
> So I think selinux should either return 0 or the security hook itself
> needs to overwrite a positive value with a sensible errno that can be
> seen by userspace.
I think that I agree. The SELinux and Smack versions of the
hook are almost identical except for setting rc to 1 in the
SELinux case. And returning 1 makes no sense if you follow
the callers back. David Howells wrote both the SELinux and
Smack versions. David - why are they different? which is correct?
>
>>
>> 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)
> <snip>
Powered by blists - more mailing lists