> On Nov 25, 2020, at 03:33, Andy Lutomirski <l...@kernel.org> wrote:
>
> On Tue, Nov 24, 2020 at 10:22 AM Bae, Chang Seok
> <chang.seok....@intel.com> wrote:
>>
>>
>>> On Nov 19, 2020, at 21:07, Andy Lutomirski <l...@kernel.org> wrote:
>>>
>>> On Thu, Nov 19, 2020 at 3:37 PM Chang S. Bae <chang.seok....@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
