linux-kernel - Re: [CFT PATCH v2 2/2] KVM: x86: support XSAVES usage in the host

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]

Message-Id: <802AF3F4-2631-4979-A8CC-46B406546062@gmail.com>
Date:	Wed, 3 Dec 2014 16:23:13 +0200
From:	Nadav Amit <nadav.amit@...il.com>
To:	Paolo Bonzini <pbonzini@...hat.com>
Cc:	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	kvm list <kvm@...r.kernel.org>,
	Wanpeng Li <wanpeng.li@...ux.intel.com>,
	Nadav Amit <namit@...technion.ac.il>, hpa@...ux.intel.com,
	Fenghua Yu <fenghua.yu@...el.com>
Subject: Re: [CFT PATCH v2 2/2] KVM: x86: support XSAVES usage in the host

Paolo Bonzini <pbonzini@...hat.com> wrote:

> Userspace is expecting non-compacted format for KVM_GET_XSAVE, but
> struct xsave_struct might be using the compacted format.  Convert
> in order to preserve userspace ABI.
> 
> Likewise, userspace is passing non-compacted format for KVM_SET_XSAVE
> but the kernel will pass it to XRSTORS, and we need to convert back.
> 
> Fixes: f31a9f7c71691569359fa7fb8b0acaa44bce0324
> Cc: Fenghua Yu <fenghua.yu@...el.com>
> Cc: H. Peter Anvin <hpa@...ux.intel.com>
> Cc: Nadav Amit <namit@...technion.ac.il>
> Signed-off-by: Paolo Bonzini <pbonzini@...hat.com>
> ---
> arch/x86/kvm/x86.c | 87 +++++++++++++++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 80 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 08b5657e57ed..373b0ab9a32e 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -3132,15 +3132,89 @@ static int kvm_vcpu_ioctl_x86_set_debugregs(struct kvm_vcpu *vcpu,
> 	return 0;
> }
> 
> +#define XSTATE_COMPACTION_ENABLED (1ULL << 63)
> +
> +static void fill_xsave(u8 *dest, struct kvm_vcpu *vcpu)
> +{
> +	struct xsave_struct *xsave = &vcpu->arch.guest_fpu.state->xsave;
> +	u64 xstate_bv = vcpu->arch.guest_supported_xcr0 | XSTATE_FPSSE;
> +	u64 valid;
> +
> +	/*
> +	 * Copy legacy XSAVE area, to avoid complications with CPUID
> +	 * leaves 0 and 1 in the loop below.
> +	 */
> +	memcpy(dest, xsave, XSAVE_HDR_OFFSET);
> +
> +	/* Set XSTATE_BV */
> +	*(u64 *)(dest + XSAVE_HDR_OFFSET) = xstate_bv;

I have a problem with this line. I ran some experiments and it has a
side-effect of causing XINUSE (an internal register which saves which state
components are not in the initial state) to be all set. As a results,
after load_xsave runs, when the guest runs xsave instruction, initialised
xsave state components are marked as not-initialised in the guest’s
xstate_bv.

This causes both transparency issues (the VM does not behave as bare-metal
machine). In addition it may cause performance overheads, since from this
point on, xsave and xrstor instructions would save and load state which is
in fact in the initial state.

I think it is better just to replace the last line with:

*(u64 *)(dest + XSAVE_HDR_OFFSET) = xsave->xsave_hdr.xstate_bv

Thanks,
Nadav

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/