Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1738753
| From | Ingo Molnar <mingo@kernel.org> |
|---|---|
| Newsgroups | linux.kernel |
| Subject | Re: [PATCH 03/10] x86/fpu: Use validate_xstate_header() to validate the xstate_header in sanitize_restored_xstate() |
| Date | 2017-09-25 08:10 +0200 |
| Message-ID | <utuKC-2qC-5@gated-at.bofh.it> (permalink) |
| References | <utcNH-7Gh-3@gated-at.bofh.it> <utcXo-7Yz-9@gated-at.bofh.it> <utkid-3NE-13@gated-at.bofh.it> <utkrU-46H-5@gated-at.bofh.it> <utlnZ-4Gy-33@gated-at.bofh.it> |
| Organization | linux.* mail to news gateway |
* Eric Biggers <ebiggers3@gmail.com> wrote:
> The following diff against tip/master fixes the bug. Note: we *could* check
> 'use_xsave()' instead of 'state_size > offsetof(struct xregs_state, header)',
> but that might be confusing in the case where we couldn't find the xstate
> information in the memory layout and only copy the fxregs_state, since then we'd
> actually be validating the xsave_header which was already there, which shouldn't
> ever fail.
>
> diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
> index afe54247cf27..fb639e70048f 100644
> --- a/arch/x86/kernel/fpu/signal.c
> +++ b/arch/x86/kernel/fpu/signal.c
> @@ -331,7 +331,8 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
> err = copy_user_to_xstate(&fpu->state.xsave, buf_fx);
> } else {
> err = __copy_from_user(&fpu->state.xsave, buf_fx, state_size);
> - if (!err)
> +
> + if (!err && state_size > offsetof(struct xregs_state, header))
> err = validate_xstate_header(&fpu->state.xsave.header);
> }
Yeah, I agree that checking 'state_size' is cleaner although note that technically
this check isn't enough, because if 'state_size' is pointing somewhere inside the
header (i.e. does not fully include it), the code still attempts a bad memcpy().
But that cannot happen, due to how state_size is set up:
int state_size = fpu_kernel_xstate_size;
...
state_size = sizeof(struct fxregs_state);
...
} else {
...
state_size = fx_sw_user.xstate_size;
...
and because fx_sw_user.xstate_size has to be at least:
int min_xstate_size = sizeof(struct fxregs_state) +
sizeof(struct xstate_header);
i.e. the 'state_size' variable has a discrete set of possible values, none of
which values point inside the header. Something to keep in mind ...
Note that there's some room for improvement within both the signal and the regset
copying of FPU state. We have this pattern:
if (using_compacted_format()) {
err = copy_user_to_xstate(&fpu->state.xsave, buf_fx);
} else {
err = __copy_from_user(&fpu->state.xsave, buf_fx, state_size);
if (!err)
err = validate_xstate_header(&fpu->state.xsave.header);
}
... and copy_user_to_xstate() does:
if (__copy_from_user(&hdr, ubuf + offset, size))
return -EFAULT;
if (validate_xstate_header(&hdr))
return -EINVAL;
I.e. what we probably want is a helper function that just copies the darn thing
and validates everything.
Note how regset.c duplicates a similar pattern:
if (using_compacted_format()) {
if (kbuf)
ret = copy_kernel_to_xstate(xsave, kbuf);
else
ret = copy_user_to_xstate(xsave, ubuf);
} else {
ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, xsave, 0, -1);
if (!ret)
ret = validate_xstate_header(&xsave->header);
}
I.e. what we should probably do is to push the using_compacted_format() check into
copy_user_to_xstate(). That makes copy_user_to_xstate() a high level method that
can deal with all formats and which does all verification.
But that's a separate cleanup.
Thanks,
Ingo
Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread
[PATCH 00/10] x86/fpu: Split up "x86/fpu: Tighten validation of user-supplied xstate_header" Ingo Molnar <mingo@kernel.org> - 2017-09-24 13:00 +0200
[PATCH 02/10] x86/fpu: Use validate_xstate_header() to validate the xstate_header in xstateregs_set() Ingo Molnar <mingo@kernel.org> - 2017-09-24 13:00 +0200
[tip:x86/fpu] x86/fpu: Use validate_xstate_header() to validate the xstate_header in xstateregs_set() tip-bot for Eric Biggers <tipbot@zytor.com> - 2017-09-26 10:50 +0200
[PATCH 09/10] x86/fpu: Use validate_xstate_header() to validate the xstate_header in copy_user_to_xstate() Ingo Molnar <mingo@kernel.org> - 2017-09-24 13:00 +0200
[tip:x86/fpu] x86/fpu: Use validate_xstate_header() to validate the xstate_header in copy_user_to_xstate() tip-bot for Eric Biggers <tipbot@zytor.com> - 2017-09-26 10:50 +0200
[PATCH 10/10] x86/fpu: Use using_compacted_format() instead of open coded X86_FEATURE_XSAVES Ingo Molnar <mingo@kernel.org> - 2017-09-24 13:10 +0200
[tip:x86/fpu] x86/fpu: Use using_compacted_format() instead of open coded X86_FEATURE_XSAVES tip-bot for Eric Biggers <tipbot@zytor.com> - 2017-09-26 10:50 +0200
[PATCH 03/10] x86/fpu: Use validate_xstate_header() to validate the xstate_header in sanitize_restored_xstate() Ingo Molnar <mingo@kernel.org> - 2017-09-24 13:10 +0200
Re: [PATCH 03/10] x86/fpu: Use validate_xstate_header() to validate the xstate_header in sanitize_restored_xstate() Eric Biggers <ebiggers3@gmail.com> - 2017-09-24 21:00 +0200
Re: [PATCH 03/10] x86/fpu: Use validate_xstate_header() to validate the xstate_header in sanitize_restored_xstate() Ingo Molnar <mingo@kernel.org> - 2017-09-24 21:10 +0200
Re: [PATCH 03/10] x86/fpu: Use validate_xstate_header() to validate the xstate_header in sanitize_restored_xstate() Eric Biggers <ebiggers3@gmail.com> - 2017-09-24 22:10 +0200
Re: [PATCH 03/10] x86/fpu: Use validate_xstate_header() to validate the xstate_header in sanitize_restored_xstate() Ingo Molnar <mingo@kernel.org> - 2017-09-25 08:10 +0200
Re: [PATCH 03/10] x86/fpu: Use validate_xstate_header() to validate the xstate_header in sanitize_restored_xstate() Ingo Molnar <mingo@kernel.org> - 2017-09-25 08:20 +0200
Re: [PATCH 03/10] x86/fpu: Use validate_xstate_header() to validate the xstate_header in sanitize_restored_xstate() Eric Biggers <ebiggers3@gmail.com> - 2017-09-25 09:30 +0200
Re: [PATCH 03/10] x86/fpu: Use validate_xstate_header() to validate the xstate_header in sanitize_restored_xstate() Ingo Molnar <mingo@kernel.org> - 2017-09-25 09:40 +0200
[tip:x86/fpu] x86/fpu: Use validate_xstate_header() to validate the xstate_header in __fpu__restore_sig() tip-bot for Eric Biggers <tipbot@zytor.com> - 2017-09-26 10:50 +0200
[PATCH 07/10] x86/fpu: Copy the full header in copy_user_to_xstate() Ingo Molnar <mingo@kernel.org> - 2017-09-24 13:10 +0200
[tip:x86/fpu] x86/fpu: Copy the full header in copy_user_to_xstate() tip-bot for Eric Biggers <tipbot@zytor.com> - 2017-09-26 10:50 +0200
[PATCH 01/10] x86/fpu: Introduce validate_xstate_header() Ingo Molnar <mingo@kernel.org> - 2017-09-24 13:10 +0200
[tip:x86/fpu] x86/fpu: Introduce validate_xstate_header() tip-bot for Eric Biggers <tipbot@zytor.com> - 2017-09-26 10:50 +0200
[PATCH 08/10] x86/fpu: Eliminate the 'xfeatures' local variable in copy_user_to_xstate() Ingo Molnar <mingo@kernel.org> - 2017-09-24 13:10 +0200
[tip:x86/fpu] x86/fpu: Eliminate the 'xfeatures' local variable in copy_user_to_xstate() tip-bot for Eric Biggers <tipbot@zytor.com> - 2017-09-26 10:50 +0200
[PATCH 04/10] x86/fpu: Copy the full state_header in copy_kernel_to_xstate() Ingo Molnar <mingo@kernel.org> - 2017-09-24 13:10 +0200
[tip:x86/fpu] x86/fpu: Copy the full state_header in copy_kernel_to_xstate() tip-bot for Eric Biggers <tipbot@zytor.com> - 2017-09-26 10:50 +0200
[PATCH 06/10] x86/fpu: Use validate_xstate_header() to validate the xstate_header in copy_kernel_to_xstate() Ingo Molnar <mingo@kernel.org> - 2017-09-24 13:10 +0200
[tip:x86/fpu] x86/fpu: Use validate_xstate_header() to validate the xstate_header in copy_kernel_to_xstate() tip-bot for Eric Biggers <tipbot@zytor.com> - 2017-09-26 10:50 +0200
Re: [PATCH 00/10] x86/fpu: Split up "x86/fpu: Tighten validation of user-supplied xstate_header" Linus Torvalds <torvalds@linux-foundation.org> - 2017-09-24 20:10 +0200
Re: [PATCH 00/10] x86/fpu: Split up "x86/fpu: Tighten validation of user-supplied xstate_header" Ingo Molnar <mingo@kernel.org> - 2017-09-24 21:10 +0200
csiph-web