Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1567079 > unrolled thread
| Started by | riel@redhat.com |
|---|---|
| First post | 2017-01-26 03:10 +0100 |
| Last post | 2017-01-26 10:50 +0100 |
| Articles | 3 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
[PATCH 1/2] x86/fpu: move copyout_from_xsaves bounds check before the copy riel@redhat.com - 2017-01-26 03:10 +0100
Re: [PATCH 1/2] x86/fpu: move copyout_from_xsaves bounds check before the copy Ingo Molnar <mingo@kernel.org> - 2017-01-26 10:50 +0100
Re: [PATCH 1/2] x86/fpu: move copyout_from_xsaves bounds check before the copy Ingo Molnar <mingo@kernel.org> - 2017-01-26 10:50 +0100
| From | riel@redhat.com |
|---|---|
| Date | 2017-01-26 03:10 +0100 |
| Subject | [PATCH 1/2] x86/fpu: move copyout_from_xsaves bounds check before the copy |
| Message-ID | <t3HFD-1sp-7@gated-at.bofh.it> |
From: Rik van Riel <riel@redhat.com> Userspace may have programs, especially debuggers, that do not know how large the full XSAVE area space is. They pass in a size argument, and expect to not get more data than that. Unfortunately, the current copyout_from_xsaves does the bounds check after the copy out to userspace. This could theoretically result in the kernel scribbling over userspace memory outside of the buffer, before bailing out of the copy. In practice, this is not likely to be an issue, since debuggers are likely to specify the size they know about, and that size is likely to exactly match the XSAVE fields they know about. However, we could be a little more careful and do the bounds check before committing ourselves with a copy to userspace. Signed-off-by: Rik van Riel <riel@redhat.com> --- arch/x86/kernel/fpu/xstate.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c index c24ac1efb12d..c1508d56ecfb 100644 --- a/arch/x86/kernel/fpu/xstate.c +++ b/arch/x86/kernel/fpu/xstate.c @@ -992,13 +992,13 @@ int copyout_from_xsaves(unsigned int pos, unsigned int count, void *kbuf, offset = xstate_offsets[i]; size = xstate_sizes[i]; + if (offset + size > count) + break; + ret = xstate_copyout(offset, size, kbuf, ubuf, src, 0, count); if (ret) return ret; - - if (offset + size >= count) - break; } } -- 2.9.3
[toc] | [next] | [standalone]
| From | Ingo Molnar <mingo@kernel.org> |
|---|---|
| Date | 2017-01-26 10:50 +0100 |
| Subject | Re: [PATCH 1/2] x86/fpu: move copyout_from_xsaves bounds check before the copy |
| Message-ID | <t3OQO-5FW-33@gated-at.bofh.it> |
| In reply to | #1567079 |
* riel@redhat.com <riel@redhat.com> wrote:
> From: Rik van Riel <riel@redhat.com>
>
> Userspace may have programs, especially debuggers, that do not know
> how large the full XSAVE area space is. They pass in a size argument,
> and expect to not get more data than that.
>
> Unfortunately, the current copyout_from_xsaves does the bounds check
> after the copy out to userspace. This could theoretically result
> in the kernel scribbling over userspace memory outside of the buffer,
> before bailing out of the copy.
>
> In practice, this is not likely to be an issue, since debuggers are
> likely to specify the size they know about, and that size is likely
> to exactly match the XSAVE fields they know about.
>
> However, we could be a little more careful and do the bounds check
> before committing ourselves with a copy to userspace.
>
> Signed-off-by: Rik van Riel <riel@redhat.com>
> ---
> arch/x86/kernel/fpu/xstate.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> index c24ac1efb12d..c1508d56ecfb 100644
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -992,13 +992,13 @@ int copyout_from_xsaves(unsigned int pos, unsigned int count, void *kbuf,
> offset = xstate_offsets[i];
> size = xstate_sizes[i];
>
> + if (offset + size > count)
> + break;
> +
> ret = xstate_copyout(offset, size, kbuf, ubuf, src, 0, count);
>
> if (ret)
> return ret;
> -
> - if (offset + size >= count)
> - break;
That's not a robust way to do a bounds check either - what if 'offset' is so large
that it overflows and offset + size falls within the 'sane' 0..count range?
Also, what about partial copies?
Plus, to add insult to injury, xstate_copyout() is a totally unnecessary
obfuscation to begin with:
- 'start_pos' is always 0
- 'end_pos' is always 'count'
- both are const for no good reason: they are not pointers
- both are signed for no good reason: they are derived from unsigned types and I
don't see how negative values can ever be valid here.
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;
}
!!!
So the real fix is to get rid of xstate_copyout() altogether and just do the
memcpy directly - the regset obfuscation actively hid a real bug...
Thanks,
Ingo
[toc] | [prev] | [next] | [standalone]
| From | Ingo Molnar <mingo@kernel.org> |
|---|---|
| Date | 2017-01-26 10:50 +0100 |
| Subject | Re: [PATCH 1/2] x86/fpu: move copyout_from_xsaves bounds check before the copy |
| Message-ID | <t3OQO-5FW-31@gated-at.bofh.it> |
| In reply to | #1567243 |
* 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
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web