linux-kernel - Re: [patch V2 08/52] x86/fpu: Sanitize xstateregs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 15 Jun 2021 19:40:45 +0200
From:   Borislav Petkov <bp@...e.de>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Andy Lutomirski <luto@...nel.org>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Fenghua Yu <fenghua.yu@...el.com>,
        Tony Luck <tony.luck@...el.com>,
        Yu-cheng Yu <yu-cheng.yu@...el.com>,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        Peter Zijlstra <peterz@...radead.org>,
        Kan Liang <kan.liang@...ux.intel.com>
Subject: Re: [patch V2 08/52] x86/fpu: Sanitize xstateregs_set()

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