linux-kernel - Re: [patch 09/41] x86/kvm: Avoid looking up PKRU in XSAVE buffer

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:   Mon, 14 Jun 2021 12:26:20 +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 09/41] x86/kvm: Avoid looking up PKRU in XSAVE buffer

On Fri, Jun 11, 2021 at 06:15:32PM +0200, Thomas Gleixner wrote:
> -		if (src) {
> -			u32 size, offset, ecx, edx;
> -			cpuid_count(XSTATE_CPUID, xfeature_nr,
> -				    &size, &offset, &ecx, &edx);
> -			if (xfeature_nr == XFEATURE_PKRU)
> -				memcpy(dest + offset, &vcpu->arch.pkru,
> -				       sizeof(vcpu->arch.pkru));
> -			else
> -				memcpy(dest + offset, src, size);
> +		cpuid_count(XSTATE_CPUID, xfeature_nr,
> +			    &size, &offset, &ecx, &edx);
>  
> +		if (xfeature_nr == XFEATURE_PKRU) {
> +			memcpy(dest + offset, &vcpu->arch.pkru,
> +			       sizeof(vcpu->arch.pkru));
> +		} else {
> +			src = get_xsave_addr(xsave, xfeature_nr);
> +			if (src)
> +				memcpy(dest + offset, src, size);
>  		}
>  
>  		valid -= xfeature_mask;

How about pulling up that PKRU case even further (pasting the whole
changed loop instead of an unreadable diff) and keeping the CPUID access
and the xsave address handling separate?

        valid = xstate_bv & ~XFEATURE_MASK_FPSSE;
        while (valid) {
                u32 size, offset, ecx, edx;
                u64 xfeature_mask = valid & -valid;
                int xfeature_nr = fls64(xfeature_mask) - 1;
                void *src;

                if (xfeature_nr == XFEATURE_PKRU) {
                        memcpy(dest + offset, &vcpu->arch.pkru, sizeof(vcpu->arch.pkru));
                        continue;
                }

                cpuid_count(XSTATE_CPUID, xfeature_nr, &size, &offset, &ecx, &edx);

                src = get_xsave_addr(xsave, xfeature_nr);
                if (src)
                        memcpy(dest + offset, src, size);

                valid -= xfeature_mask;
        }

Btw, I'm wondering if that CPUID read in a loop can be replaced with
adding accessors for xstate_{offsets,sizes,..} etc and providing them to
kvm...


> @@ -4632,18 +4633,20 @@ static void load_xsave(struct kvm_vcpu *
>  	 */
>  	valid = xstate_bv & ~XFEATURE_MASK_FPSSE;
>  	while (valid) {
> +		u32 size, offset, ecx, edx;
>  		u64 xfeature_mask = valid & -valid;
>  		int xfeature_nr = fls64(xfeature_mask) - 1;
> -		void *dest = get_xsave_addr(xsave, xfeature_nr);
>  
> -		if (dest) {
> -			u32 size, offset, ecx, edx;
> -			cpuid_count(XSTATE_CPUID, xfeature_nr,
> -				    &size, &offset, &ecx, &edx);
> -			if (xfeature_nr == XFEATURE_PKRU)
> -				memcpy(&vcpu->arch.pkru, src + offset,
> -				       sizeof(vcpu->arch.pkru));
> -			else
> +		cpuid_count(XSTATE_CPUID, xfeature_nr,
> +			    &size, &offset, &ecx, &edx);
> +
> +		if (xfeature_nr == XFEATURE_PKRU) {
> +			memcpy(&vcpu->arch.pkru, src + offset,
> +			       sizeof(vcpu->arch.pkru));
> +		} else {
> +			void *dest = get_xsave_addr(xsave, xfeature_nr);
> +


^ Superfluous newline.

> +			if (dest)
>  				memcpy(dest, src + offset, size);
>  		}
>  
> 

-- 
Regards/Gruss,
    Boris.

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