[PATCH v2] x86: ignore guest microcode loading attempts

[PATCH v2] x86: ignore guest microcode loading attempts

[Jan Beulich]

The respective MSRs are write-only, and hence attempts by guests to
write to these are - as of 1f1d183d49 ("x86/HVM: don't give the wrong
impression of WRMSR succeeding") no longer ignored. Restore original
behavior for the two affected MSRs.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Add comments.
---
While what is being logged for the current osstest failures on the 4.7
and 4.9 branches doesn't fully prove this to be the problem, RCX holding
0x79 and there being a recorded hypervisor level #GP recovery
immediately before the guest triple fault is sufficient indication imo.
What I'm unsure about is whether we want to ignore such writes also for
PV guests. If not, at least the WRMSR change would need to move into
hvm/hvm.c.

--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -147,6 +147,8 @@ int guest_rdmsr(const struct vcpu *v, ui
 
     switch ( msr )
     {
+    case MSR_AMD_PATCHLOADER:
+    case MSR_IA32_UCODE_WRITE:
     case MSR_PRED_CMD:
         /* Write-only */
         goto gp_fault;
@@ -200,6 +202,26 @@ int guest_wrmsr(struct vcpu *v, uint32_t
         /* Read-only */
         goto gp_fault;
 
+    case MSR_AMD_PATCHLOADER:
+        /*
+         * See note on MSR_IA32_UCODE_WRITE below, which may or may not apply
+         * to AMD CPUs as well (at least the architectural/CPUID part does).
+         */
+        if ( d->arch.cpuid->x86_vendor != X86_VENDOR_AMD )
+            goto gp_fault;
+        break;
+
+    case MSR_IA32_UCODE_WRITE:
+        /*
+         * Some versions of Windows at least on certain hardware try to load
+         * microcode before setting up an IDT. Therefore we must not inject #GP
+         * for such attempts. Also the MSR is architectural and not qualified
+         * by any CPUID bit.
+         */
+        if ( d->arch.cpuid->x86_vendor != X86_VENDOR_INTEL )
+            goto gp_fault;
+        break;
+
     case MSR_SPEC_CTRL:
         if ( !cp->feat.ibrsb )
             goto gp_fault; /* MSR available? */




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2] x86: ignore guest microcode loading attempts

[Andrew Cooper]

On 15/03/18 11:09, Jan Beulich wrote:
> The respective MSRs are write-only, and hence attempts by guests to
> write to these are - as of 1f1d183d49 ("x86/HVM: don't give the wrong
> impression of WRMSR succeeding") no longer ignored. Restore original
> behavior for the two affected MSRs.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v2: Add comments.
> ---
> While what is being logged for the current osstest failures on the 4.7
> and 4.9 branches doesn't fully prove this to be the problem, RCX holding
> 0x79 and there being a recorded hypervisor level #GP recovery
> immediately before the guest triple fault is sufficient indication imo.
> What I'm unsure about is whether we want to ignore such writes also for
> PV guests. If not, at least the WRMSR change would need to move into
> hvm/hvm.c.

Sorry - I've raced with v2 of your patch.  The PV side currently always
yields #GP, because we have a read_safe() check on the wrmsr side.  With
that behaviour retained (i.e. with !is_pv_domain() checks in the hunks
below), Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

I specifically want to avoid any further additions to the legacy MSR
paths.  They should dwindle to nothing as we move functionality over.

>
> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -147,6 +147,8 @@ int guest_rdmsr(const struct vcpu *v, ui
>  
>      switch ( msr )
>      {
> +    case MSR_AMD_PATCHLOADER:
> +    case MSR_IA32_UCODE_WRITE:
>      case MSR_PRED_CMD:
>          /* Write-only */
>          goto gp_fault;
> @@ -200,6 +202,26 @@ int guest_wrmsr(struct vcpu *v, uint32_t
>          /* Read-only */
>          goto gp_fault;
>  
> +    case MSR_AMD_PATCHLOADER:
> +        /*
> +         * See note on MSR_IA32_UCODE_WRITE below, which may or may not apply
> +         * to AMD CPUs as well (at least the architectural/CPUID part does).
> +         */
> +        if ( d->arch.cpuid->x86_vendor != X86_VENDOR_AMD )
> +            goto gp_fault;
> +        break;
> +
> +    case MSR_IA32_UCODE_WRITE:
> +        /*
> +         * Some versions of Windows at least on certain hardware try to load
> +         * microcode before setting up an IDT. Therefore we must not inject #GP
> +         * for such attempts. Also the MSR is architectural and not qualified
> +         * by any CPUID bit.
> +         */
> +        if ( d->arch.cpuid->x86_vendor != X86_VENDOR_INTEL )
> +            goto gp_fault;
> +        break;
> +
>      case MSR_SPEC_CTRL:
>          if ( !cp->feat.ibrsb )
>              goto gp_fault; /* MSR available? */
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2] x86: ignore guest microcode loading attempts

[Jan Beulich]

>>> On 15.03.18 at 12:14, <andrew.cooper3@citrix.com> wrote:
> On 15/03/18 11:09, Jan Beulich wrote:
>> The respective MSRs are write-only, and hence attempts by guests to
>> write to these are - as of 1f1d183d49 ("x86/HVM: don't give the wrong
>> impression of WRMSR succeeding") no longer ignored. Restore original
>> behavior for the two affected MSRs.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> v2: Add comments.
>> ---
>> While what is being logged for the current osstest failures on the 4.7
>> and 4.9 branches doesn't fully prove this to be the problem, RCX holding
>> 0x79 and there being a recorded hypervisor level #GP recovery
>> immediately before the guest triple fault is sufficient indication imo.
>> What I'm unsure about is whether we want to ignore such writes also for
>> PV guests. If not, at least the WRMSR change would need to move into
>> hvm/hvm.c.
> 
> Sorry - I've raced with v2 of your patch.  The PV side currently always
> yields #GP, because we have a read_safe() check on the wrmsr side.  With
> that behaviour retained (i.e. with !is_pv_domain() checks in the hunks
> below), Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Just to be sure we're on the same page (as the ! above is not what
I've meant to do), this is the hunk in question:

@@ -200,6 +202,28 @@ int guest_wrmsr(struct vcpu *v, uint32_t
         /* Read-only */
         goto gp_fault;
 
+    case MSR_AMD_PATCHLOADER:
+        /*
+         * See note on MSR_IA32_UCODE_WRITE below, which may or may not apply
+         * to AMD CPUs as well (at least the architectural/CPUID part does).
+         */
+        if ( is_pv_domain(d) ||
+             d->arch.cpuid->x86_vendor != X86_VENDOR_AMD )
+            goto gp_fault;
+        break;
+
+    case MSR_IA32_UCODE_WRITE:
+        /*
+         * Some versions of Windows at least on certain hardware try to load
+         * microcode before setting up an IDT. Therefore we must not inject #GP
+         * for such attempts. Also the MSR is architectural and not qualified
+         * by any CPUID bit.
+         */
+        if ( is_pv_domain(d) ||
+             d->arch.cpuid->x86_vendor != X86_VENDOR_INTEL )
+            goto gp_fault;
+        break;
+
     case MSR_SPEC_CTRL:
         if ( !cp->feat.ibrsb )
             goto gp_fault; /* MSR available? */



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2] x86: ignore guest microcode loading attempts

[Andrew Cooper]

On 15/03/18 11:36, Jan Beulich wrote:
>>>> On 15.03.18 at 12:14, <andrew.cooper3@citrix.com> wrote:
>> On 15/03/18 11:09, Jan Beulich wrote:
>>> The respective MSRs are write-only, and hence attempts by guests to
>>> write to these are - as of 1f1d183d49 ("x86/HVM: don't give the wrong
>>> impression of WRMSR succeeding") no longer ignored. Restore original
>>> behavior for the two affected MSRs.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> ---
>>> v2: Add comments.
>>> ---
>>> While what is being logged for the current osstest failures on the 4.7
>>> and 4.9 branches doesn't fully prove this to be the problem, RCX holding
>>> 0x79 and there being a recorded hypervisor level #GP recovery
>>> immediately before the guest triple fault is sufficient indication imo.
>>> What I'm unsure about is whether we want to ignore such writes also for
>>> PV guests. If not, at least the WRMSR change would need to move into
>>> hvm/hvm.c.
>> Sorry - I've raced with v2 of your patch.  The PV side currently always
>> yields #GP, because we have a read_safe() check on the wrmsr side.  With
>> that behaviour retained (i.e. with !is_pv_domain() checks in the hunks
>> below), Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Just to be sure we're on the same page (as the ! above is not what
> I've meant to do), this is the hunk in question:

LGTM.

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

>
> @@ -200,6 +202,28 @@ int guest_wrmsr(struct vcpu *v, uint32_t
>          /* Read-only */
>          goto gp_fault;
>  
> +    case MSR_AMD_PATCHLOADER:
> +        /*
> +         * See note on MSR_IA32_UCODE_WRITE below, which may or may not apply
> +         * to AMD CPUs as well (at least the architectural/CPUID part does).
> +         */
> +        if ( is_pv_domain(d) ||
> +             d->arch.cpuid->x86_vendor != X86_VENDOR_AMD )
> +            goto gp_fault;
> +        break;
> +
> +    case MSR_IA32_UCODE_WRITE:
> +        /*
> +         * Some versions of Windows at least on certain hardware try to load
> +         * microcode before setting up an IDT. Therefore we must not inject #GP
> +         * for such attempts. Also the MSR is architectural and not qualified
> +         * by any CPUID bit.
> +         */
> +        if ( is_pv_domain(d) ||
> +             d->arch.cpuid->x86_vendor != X86_VENDOR_INTEL )
> +            goto gp_fault;
> +        break;
> +
>      case MSR_SPEC_CTRL:
>          if ( !cp->feat.ibrsb )
>              goto gp_fault; /* MSR available? */
>
>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel