Re: [syzbot] general protection fault in legacy_parse_param

[Casey Schaufler]

On 8/27/2021 8:30 AM, Christian Brauner wrote:
> On Tue, Jul 06, 2021 at 08:50:44AM -0400, Paul Moore wrote:
>> On Mon, Jul 5, 2021 at 1:52 AM Dmitry Vyukov <dvyukov@xxxxxxxxxx> wrote:
>>> On Sun, Jul 4, 2021 at 4:14 PM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
>>>> On Sat, Jul 3, 2021 at 6:16 PM Casey Schaufler <casey@xxxxxxxxxxxxxxxx> wrote:
>>>>> On 7/2/2021 10:51 PM, Dmitry Vyukov wrote:
>>>>>> On Sat, Jul 3, 2021 at 7:41 AM syzbot
>>>>>> <syzbot+d1e3b1d92d25abf97943@xxxxxxxxxxxxxxxxxxxxxxxxx> wrote:
>>>>>>> Hello,
>>>>>>>
>>>>>>> syzbot found the following issue on:
>>>>>>>
>>>>>>> HEAD commit:    62fb9874 Linux 5.13
>>>>>>> git tree:       upstream
>>>>>>> console output: https://syzkaller.appspot.com/x/log.txt?x=12ffa118300000
>>>>>>> kernel config:  https://syzkaller.appspot.com/x/.config?x=19404adbea015a58
>>>>>>> dashboard link: https://syzkaller.appspot.com/bug?extid=d1e3b1d92d25abf97943
>>>>>>> compiler:       Debian clang version 11.0.1-2
>>>>>>>
>>>>>>> Unfortunately, I don't have any reproducer for this issue yet.
>>>>>>>
>>>>>>> IMPORTANT: if you fix the issue, please add the following tag to the commit:
>>>>>>> Reported-by: syzbot+d1e3b1d92d25abf97943@xxxxxxxxxxxxxxxxxxxxxxxxx
>>>>>> +Casey for what looks like a smackfs issue
>>>>> This is from the new mount infrastructure introduced by
>>>>> David Howells in November 2018. It makes sense that there
>>>>> may be a problem in SELinux as well, as the code was introduced
>>>>> by the same developer at the same time for the same purpose.
>>>>>
>>>>>> The crash was triggered by this test case:
>>>>>>
>>>>>> 21:55:33 executing program 1:
>>>>>> r0 = fsopen(&(0x7f0000000040)='ext3\x00', 0x1)
>>>>>> fsconfig$FSCONFIG_SET_STRING(r0, 0x1, &(0x7f00000002c0)='smackfsroot',
>>>>>> &(0x7f0000000300)='default_permissions', 0x0)
>>>>>>
>>>>>> And I think the issue is in smack_fs_context_parse_param():
>>>>>> https://elixir.bootlin.com/linux/latest/source/security/smack/smack_lsm.c#L691
>>>>>>
>>>>>> But it seems that selinux_fs_context_parse_param() contains the same issue:
>>>>>> https://elixir.bootlin.com/linux/latest/source/security/selinux/hooks.c#L2919
>>>>>> +So selinux maintainers as well.
>>>>>>
>>>>>>> general protection fault, probably for non-canonical address 0xdffffc0000000000: 0000 [#1] PREEMPT SMP KASAN
>>>>>>> KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007]
>>>>>>> CPU: 0 PID: 20300 Comm: syz-executor.1 Not tainted 5.13.0-syzkaller #0
>>>>>>> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
>>>>>>> RIP: 0010:memchr+0x2f/0x70 lib/string.c:1054
>>>>>>> Code: 41 54 53 48 89 d3 41 89 f7 45 31 f6 49 bc 00 00 00 00 00 fc ff df 0f 1f 44 00 00 48 85 db 74 3b 48 89 fd 48 89 f8 48 c1 e8 03 <42> 0f b6 04 20 84 c0 75 0f 48 ff cb 48 8d 7d 01 44 38 7d 00 75 db
>>>>>>> RSP: 0018:ffffc90001dafd00 EFLAGS: 00010246
>>>>>>> RAX: 0000000000000000 RBX: 0000000000000013 RCX: dffffc0000000000
>>>>>>> RDX: 0000000000000013 RSI: 000000000000002c RDI: 0000000000000000
>>>>>>> RBP: 0000000000000000 R08: ffffffff81e171bf R09: ffffffff81e16f95
>>>>>>> R10: 0000000000000002 R11: ffff88807e96b880 R12: dffffc0000000000
>>>>>>> R13: ffff888020894000 R14: 0000000000000000 R15: 000000000000002c
>>>>>>> FS:  00007fe01ae27700(0000) GS:ffff8880b9a00000(0000) knlGS:0000000000000000
>>>>>>> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>>>>>> CR2: 00000000005645a8 CR3: 0000000018afc000 CR4: 00000000001506f0
>>>>>>> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>>>>>>> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>>>>>>> Call Trace:
>>>>>>>  legacy_parse_param+0x461/0x7e0 fs/fs_context.c:537
>>>>>>>  vfs_parse_fs_param+0x1e5/0x460 fs/fs_context.c:117
>>>> It's Sunday morning and perhaps my mind is not yet in a "hey, let's
>>>> look at VFS kernel code!" mindset, but I'm not convinced the problem
>>>> is the 'param->string = NULL' assignment in the LSM hooks.  In both
>>>> the case of SELinux and Smack that code ends up returning either a 0
>>>> (Smack) or a 1 (SELinux) - that's a little odd in it's own way, but I
>>>> don't believe it is relevant here - either way these return values are
>>>> not equal to -ENOPARAM so we should end up returning early from
>>>> vfs_parse_fs_param before it calls down into legacy_parse_param():
>>>>
>>>> Taken from https://elixir.bootlin.com/linux/latest/source/fs/fs_context.c#L109 :
>>>>
>>>>   ret = security_fs_context_parse_param(fc, param);
>>>>   if (ret != -ENOPARAM)
>>>>     /* Param belongs to the LSM or is disallowed by the LSM; so
>>>>      * don't pass to the FS.
>>>>      */
>>>>     return ret;
>>>>
>>>>   if (fc->ops->parse_param) {
>>>>     ret = fc->ops->parse_param(fc, param);
>>>>     if (ret != -ENOPARAM)
>>>>       return ret;
>>>>   }
>>> Hi Paul,
>>>
>>> You are right.
>>> I almost connected the dots, but not exactly.
>>> Now that I read more code around, setting "param->string = NULL" in
>>> smack_fs_context_parse_param() looks correct to me (the fs copies and
>>> takes ownership of the string).
>>>
>>> I don't see how the crash happened...
>> FWIW, I poked around a bit too and couldn't see anything obvious
>> either, but I can't pretend to know as much about the VFS layer as the
>> VFS folks.  Hopefully they might have better luck.
> I'm not sure that's right.
> If the smack hook runs first, it will set
>
> param->string = NULL
>
> now the selinux hook runs. But the selinux param hook doesn't end up in
> selinux_add_opt() instead it will fail before
> opt = fs_parse(fc, selinux_fs_parameters, param, &result);
> which will return -ENOPARAM since it's not a selinux option subsequently
> causing the crash.
>
> Does that sound plausible?

No. You can't (currently) have both Smack and SELinux enabled at
the same time. If you're invoking both the Smack hook and the SELinux
hook you're doing somthing way wrong.

>
> Christian