Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [syzbot] general protection fault in legacy_parse_param
To:     Christian Brauner <christian.brauner@ubuntu.com>,
        syzbot <syzbot+d1e3b1d92d25abf97943@syzkaller.appspotmail.com>
Cc:     andriin@fb.com, ast@kernel.org, bpf@vger.kernel.org,
        daniel@iogearbox.net, dhowells@redhat.com, dvyukov@google.com,
        jmorris@namei.org, kafai@fb.com, kpsingh@google.com,
        linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-security-module@vger.kernel.org, netdev@vger.kernel.org,
        paul@paul-moore.com, selinux@vger.kernel.org,
        songliubraving@fb.com, stephen.smalley.work@gmail.com,
        syzkaller-bugs@googlegroups.com, tonymarislogistics@yandex.com,
        viro@zeniv.linux.org.uk, yhs@fb.com,
        Casey Schaufler <casey@schaufler-ca.com>
References: <0000000000004e5ec705c6318557@google.com>
 <0000000000008d2a0005ca951d94@google.com>
 <20210830122348.jffs5dmq6z25qzw5@wittgenstein>
 <61bf6b11-80f8-839e-4ae7-54c2c6021ed5@schaufler-ca.com>
From:   Casey Schaufler <casey@schaufler-ca.com>
Message-ID: <89d0e012-4caf-4cda-3c4e-803a2c6ebc2b@schaufler-ca.com>
Date:   Mon, 30 Aug 2021 09:40:57 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.13.0
MIME-Version: 1.0
In-Reply-To: <61bf6b11-80f8-839e-4ae7-54c2c6021ed5@schaufler-ca.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Content-Language: en-US
Content-Length: 4679
Precedence: bulk

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@google.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=3D160c5d=
75300000
>>> 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=3D150c5d=
75300000
>>> console output: https://syzkaller.appspot.com/x/log.txt?x=3D110c5d753=
00000
>>> kernel config:  https://syzkaller.appspot.com/x/.config?x=3D2fd902af7=
7ff1e56
>>> dashboard link: https://syzkaller.appspot.com/bug?extid=3Dd1e3b1d92d2=
5abf97943
>>> syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=3D126d084=
d300000
>>> C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=3D16216eb13=
00000
>>>
>>> Reported-by: syzbot+d1e3b1d92d25abf97943@syzkaller.appspotmail.com
>>> Fixes: 54261af473be ("security: Fix the default value of fs_context_p=
arse_param hook")
>>>
>>> For information about bisection process see: https://goo.gl/tpsmEJ#bi=
section
>> So ok, this seems somewhat clear now. When smack and=20
>> CONFIG_BPF_LSM=3Dy
>> 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=3D"landlock,lockdown,yama,safesetid,integrity,tomoyo,smack,=
bpf"
>>
>> in the appended config. The smack hook runs and sets
>>
>> param->string =3D 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 shoul=
d
>> proceed processing the parameter subsequently causing the crash becaus=
e
>> param->string is not allowed to be NULL (Which the vfs parameter parse=
r
>> 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.


 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, s=
truct fs_context *src_fc)
=20
 int security_fs_context_parse_param(struct fs_context *fc, struct fs_par=
ameter *param)
 {
-	return call_int_hook(fs_context_parse_param, -ENOPARAM, fc, param);
+	struct security_hook_list *hp;
+	int trc;
+	int rc =3D -ENOPARAM;
+
+	hlist_for_each_entry(hp, &security_hook_heads.fs_context_parse_param,
+			     list) {
+		trc =3D hp->hook.fs_context_parse_param(fc, param);
+		if (trc =3D=3D 0)
+			rc =3D 0;
+		else if (trc !=3D -ENOPARAM)
+			return trc;
+	}
+	return rc;
 }
=20
 int security_sb_alloc(struct super_block *sb)

>
>> If you take the appended syzkaller config and additionally select
>> kprobes you can observe this by registering bpf kretprobes for:
>> security_fs_context_parse_param()
>> smack_fs_context_parse_param()
>> bpf_lsm_fs_context_parse_param()
>> in different terminal windows and then running the syzkaller provided
>> reproducer:
>>
>> root@f2-vm:~# bpftrace -e 'kretprobe:smack_fs_context_parse_param { pr=
intf("returned: %d\n", retval); }'
>> Attaching 1 probe...
>> returned: 0
>>
>> root@f2-vm:~# bpftrace -e 'kretprobe:bpf_lsm_fs_context_parse_param { =
printf("returned: %d\n", retval); }'
>> Attaching 1 probe...
>> returned: -519
>>
>> root@f2-vm:~# bpftrace -e 'kretprobe:security_fs_context_parse_param {=
 printf("returned: %d\n", retval); }'
>> Attaching 1 probe...
>> returned: -519
>>
>> ^^^^^
>> This will ultimately tell the vfs to move on causing the crash because=

>> param->string is null at that point.
>>
>> Unless I missed something why that can't happen.
>>
>> Christian