lkml.org 
[lkml]   [2020]   [Nov]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [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@intel.com> wrote:
>
>
> > On Nov 19, 2020, at 21:07, Andy Lutomirski <luto@kernel.org> wrote:
> >
> > On Thu, Nov 19, 2020 at 3:37 PM Chang S. Bae <chang.seok.bae@intel.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.

\
 
 \ /
  Last update: 2020-11-24 19:36    [W:0.089 / U:0.020 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site