Re: [patch V2 08/52] x86/fpu: Sanitize xstateregs_set()

Borislav Petkov
2021-06-15T17:40:58Z

On Mon, Jun 14, 2021 at 05:44:16PM +0200, Thomas Gleixner wrote:
> @@ -108,10 +110,10 @@ int xstateregs_set(struct task_struct *t
> const void *kbuf, const void __user *ubuf)
> {
> struct fpu *fpu = &target->thread.fpu;
> - struct xregs_state *xsave;
> + struct xregs_state *tmpbuf = NULL;
> int ret;
>
> - if (!boot_cpu_has(X86_FEATURE_XSAVE))
> + if (!static_cpu_has(X86_FEATURE_XSAVE))

cpu_feature_enabled() - we're going to use only that thing from now on
for simplicity.

> + if (!kbuf) {
> + tmpbuf = vmalloc(count);
> + if (!tmpbuf)
> + return -ENOMEM;
> +
> + if (copy_from_user(tmpbuf, ubuf, count)) {
> + ret = -EFAULT;
> + goto out;
> + }
> }
>
> - /*
> - * mxcsr reserved bits must be masked to zero for security reasons.
> - */
> - xsave->i387.mxcsr &= mxcsr_feature_mask;
> -
> - /*
> - * In case of failure, mark all states as init:
> - */
> - if (ret)
> - fpstate_init(&fpu->state);
> + fpu__prepare_write(fpu);

Yikes, why isn't this function called

fpu_invalidate_state(fpu)

?!

As in, what it does...

> @@ -1196,14 +1196,16 @@ int copy_kernel_to_xstate(struct xregs_s
> */
> xsave->header.xfeatures |= hdr.xfeatures;
>
> + /* mxcsr reserved bits must be masked to zero for historical reasons. */

Wasn't that comment supposed to get some love?

https://lkml.kernel.org/r/87k0n0w3p8.ffs@nanos.tec.linutronix.de

> + xsave->i387.mxcsr &= mxcsr_feature_mask;
> +

Thx.

--
Regards/Gruss,
Boris.

SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 36809, AG Nürnberg