Messages in this thread | | | From | "Bae, Chang Seok" <> | Subject | Re: [PATCH v2 15/22] x86/fpu/xstate: Support ptracer-induced xstate area expansion | Date | Wed, 2 Dec 2020 04:00:15 +0000 |
| |
> On Nov 25, 2020, at 03:33, Andy Lutomirski <luto@kernel.org> wrote: > > 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.
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
| |