Re: [PATCH 1/2] x86/fpu: move copyout_from_xsaves bounds check before the copy

[Ingo Molnar <mingo@kernel.org>]

* Ingo Molnar <mingo@kernel.org> wrote:

> So this code:
> 
> static inline int xstate_copyout(unsigned int pos, unsigned int count,
>                                  void *kbuf, void __user *ubuf,
>                                  const void *data, const int start_pos,
>                                  const int end_pos)
> {
>         if ((count == 0) || (pos < start_pos))
>                 return 0;
> 
>         if (end_pos < 0 || pos < end_pos) {
>                 unsigned int copy = (end_pos < 0 ? count : min(count, end_pos - pos));
> 
>                 if (kbuf) {
>                         memcpy(kbuf + pos, data, copy);
>                 } else {
>                         if (__copy_to_user(ubuf + pos, data, copy))
>                                 return -EFAULT;
>                 }
>         }
>         return 0;
> }
> 
> Is, after all the cleanups and fixes is in reality equivalent to:
> 
> static inline int
> __copy_xstate_to_kernel(void *kbuf, const void *data,
>                         unsigned int offset, unsigned int size)
> {
>         memcpy(kbuf + offset, data, size);
> 
>         return 0;
> }
> 
> !!!

Note that it's not entirely true - for the degenerate case of ptrace() requesting 
a very small and partial buffer that cannot even fit the headers, this check is 
still required - so we end up with something like:

static inline int
__copy_xstate_to_kernel(void *kbuf, const void *data,
                        unsigned int offset, unsigned int size, unsigned int size_total)
{ 
        if (offset < size_total) {
                unsigned int copy = min(size, size_total - offset);
                
                memcpy(kbuf + offset, data, copy);
        }       
        return 0;
}       

But it's still an inconsistent mess: we'll do a partial copy in headers but not 
for xstate components?

I believe the right solution is to allow partial copies only if they are at 
precise xstate (and legacy) component boundaries, and apply this to the header 
portion as well.

This allows user-space to request only the FPU bits for example - but doesn't 
force the kernel to handle really weird partial copy cases that very few people 
are testing ...

(Unless there's some ABI pattern from debugging applications that I missed?)

Thanks,

	Ingo