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 10:33:53 -0800
From:   Andy Lutomirski <luto@...nel.org>
To:     "Bae, Chang Seok" <chang.seok.bae@...el.com>
Cc:     Andy Lutomirski <luto@...nel.org>,
        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 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.  So
your patch really does break the syscall.  Please fix it.