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:   Tue, 24 Nov 2020 18:22:31 +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 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.

Thanks,
Chang

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