linux-kernel - Re: [PATCH 1/2] x86/fpu: move copyout_from

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170126094733.GA22486@gmail.com>
Date:   Thu, 26 Jan 2017 10:47:33 +0100
From:   Ingo Molnar <mingo@...nel.org>
To:     riel@...hat.com
Cc:     linux-kernel@...r.kernel.org, luto@...nel.org,
        yu-cheng.yu@...el.com, dave.hansen@...ux.intel.com, bp@...e.de
Subject: Re: [PATCH 1/2] x86/fpu: move copyout_from_xsaves bounds check
 before the copy


* Ingo Molnar <mingo@...nel.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