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

[Andy Lutomirski]

On Tue, Dec 1, 2020 at 8:00 PM Bae, Chang Seok <chang.seok.bae@xxxxxxxxx> wrote:
>
>
> > On Nov 25, 2020, at 03:33, Andy Lutomirski <luto@xxxxxxxxxx> wrote:
> >
> > On Tue, Nov 24, 2020 at 10:22 AM Bae, Chang Seok
> > <chang.seok.bae@xxxxxxxxx> wrote:
> >>
> >>
> >>> On Nov 19, 2020, at 21:07, Andy Lutomirski <luto@xxxxxxxxxx> wrote:
> >>>
> >>> On Thu, Nov 19, 2020 at 3:37 PM Chang S. Bae <chang.seok.bae@xxxxxxxxx> 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.

Oh, I get it.  The user is expected to pass in data in the uncompacted format.

Have you confirmed that real ptrace users (gdb?  does anything
actually use this code?) can handle the huge buffers that will be
needed?

--Andy