* Eric Biggers <[email protected]> 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
