Re: [PATCH 03/10] x86/fpu: Use validate_xstate_header() to validate the xstate_header in sanitize_restored_xstate()

[Ingo Molnar <mingo@kernel.org>]

* 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