linux-kernel - Re: [PATCH v2 15/22] x86/fpu/xstate: Support ptracer-induced xstate area expansion

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 2 Dec 2020 04:00:15 +0000
From:   "Bae, Chang Seok" <chang.seok.bae@...el.com>
To:     Andy Lutomirski <luto@...nel.org>
CC:     Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...nel.org>, Borislav Petkov <bp@...e.de>,
        X86 ML <x86@...nel.org>, "Brown, Len" <len.brown@...el.com>,
        "Hansen, Dave" <dave.hansen@...el.com>,
        "Liu, Jing2" <jing2.liu@...el.com>,
        "Shankar, Ravi V" <ravi.v.shankar@...el.com>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 15/22] x86/fpu/xstate: Support ptracer-induced xstate
 area expansion


> On Nov 25, 2020, at 03:33, Andy Lutomirski <luto@...nel.org> wrote:
> 
> On Tue, Nov 24, 2020 at 10:22 AM Bae, Chang Seok
> <chang.seok.bae@...el.com> wrote:
>> 
>> 
>>> On Nov 19, 2020, at 21:07, Andy Lutomirski <luto@...nel.org> wrote:
>>> 
>>> On Thu, Nov 19, 2020 at 3:37 PM Chang S. Bae <chang.seok.bae@...el.com> wrote:
>>>> 
>>>> 
>>>> diff --git a/arch/x86/kernel/fpu/regset.c b/arch/x86/kernel/fpu/regset.c
>>>> index 8d863240b9c6..6b9d0c0a266d 100644
>>>> --- a/arch/x86/kernel/fpu/regset.c
>>>> +++ b/arch/x86/kernel/fpu/regset.c
>>>> @@ -125,6 +125,35 @@ int xstateregs_set(struct task_struct *target, const struct user_regset *regset,
>>>> 
>>>>       xsave = __xsave(fpu);
>>>> 
>>>> +       /*
>>>> +        * When a ptracer attempts to write any state in task->fpu but not allocated,
>>>> +        * it dynamically expands the xstate area of fpu->state_ptr.
>>>> +        */
>>>> +       if (count > get_xstate_size(fpu->state_mask)) {
>>>> +               unsigned int offset, size;
>>>> +               struct xstate_header hdr;
>>>> +               u64 mask;
>>>> +
>>>> +               offset = offsetof(struct xregs_state, header);
>>>> +               size = sizeof(hdr);
>>>> +
>>>> +               /* Retrieve XSTATE_BV */
>>>> +               if (kbuf) {
>>>> +                       memcpy(&hdr, kbuf + offset, size);
>>>> +               } else {
>>>> +                       ret = __copy_from_user(&hdr, ubuf + offset, size);
>>>> +                       if (ret)
>>>> +                               return ret;
>>>> +               }
>>>> +
>>>> +               mask = hdr.xfeatures & xfeatures_mask_user_dynamic;
>>>> +               if (!mask) {
>>>> +                       ret = alloc_xstate_area(fpu, mask, NULL);
>>>> +                       if (ret)
>>>> +                               return ret;
>>>> +               }
>>>> +       }
>>>> +
>>> 
>>> This whole function is garbage.  The count parameter is entirely
>>> ignored except that the beginning of the function compares it to the
>>> constant known size.  Now that it's dynamic, you need to actually
>>> validate the count.  Right now, you will happily overrun the buffer if
>>> the mask in the buffer isn't consistent with count.
>> 
>> In practice, copy_{kernel|user}_to_xstate() is the copy function. It actually
>> relies on the mask [1], rather than the count. If the state bit not set in the
>> mask, the state is not copied.
>> 
>> This path may be better to be clean up for readability. We can try to cleanup
>> in a separate series.
>> 
>> Also, I think the series needs to enable XFD only with XSAVES -- the compacted
>> format used in the kernel.
> 
> I disagree.  Before your patch, if you passed in a fixed-size buffer
> with arbitrary data, the worst that could happen was corruption of the
> target process.  With your patch, if you pass in a fixed-size buffer
> with too many mask bits set, the syscall will overrun the buffer.

True, user space provides a fixed-size buffer in an uncompacted format
-- the size should be enough to cover states set in XCR0 [1]. 

Here, the code only cares states set in XCR0; the mask bits not set in XCR0 do
not trigger the expansion. I don’t get the buffer overrun.

Thanks,
Chang

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/kernel/fpu/regset.c#n120