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>
From:   Casey Schaufler <casey@schaufler-ca.com>
Message-ID: <61bf6b11-80f8-839e-4ae7-54c2c6021ed5@schaufler-ca.com>
Date:   Mon, 30 Aug 2021 07:25:15 -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: <20210830122348.jffs5dmq6z25qzw5@wittgenstein>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Content-Language: en-US
Content-Length: 3510
Precedence: bulk

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=3D160c5d7=
5300000
>> 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=3D150c5d7=
5300000
>> console output: https://syzkaller.appspot.com/x/log.txt?x=3D110c5d7530=
0000
>> kernel config:  https://syzkaller.appspot.com/x/.config?x=3D2fd902af77=
ff1e56
>> dashboard link: https://syzkaller.appspot.com/bug?extid=3Dd1e3b1d92d25=
abf97943
>> syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=3D126d084d=
300000
>> C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=3D16216eb130=
0000
>>
>> Reported-by: syzbot+d1e3b1d92d25abf97943@syzkaller.appspotmail.com
>> Fixes: 54261af473be ("security: Fix the default value of fs_context_pa=
rse_param hook")
>>
>> For information about bisection process see: https://goo.gl/tpsmEJ#bis=
ection
> 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,b=
pf"
>
> 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 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.

>
> 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 { pri=
ntf("returned: %d\n", retval); }'
> Attaching 1 probe...
> returned: 0
>
> root@f2-vm:~# bpftrace -e 'kretprobe:bpf_lsm_fs_context_parse_param { p=
rintf("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