Messages in this thread | | | From | Andy Lutomirski <> | Date | Tue, 24 Nov 2020 10:33:53 -0800 | 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@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.
| |