Re: [Xen-devel] [PATCH v2 2/3] x86: explicitly disallow guest access to PPIN

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 08/11/2019 15:24, Jan Beulich wrote:
> To fulfill the "protected" in its name, don't let the real hardware
> values "shine through". Report a control register value expressing this.

Why not call it as it is?  They leak through due to bugs in MSR handling.

> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> v2: Use "cp" consistently. Re-base.
>
> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -135,6 +135,8 @@ int guest_rdmsr(struct vcpu *v, uint32_t
>      case MSR_TSX_FORCE_ABORT:
>      case MSR_AMD64_LWP_CFG:
>      case MSR_AMD64_LWP_CBADDR:
> +    case MSR_PPIN:
> +    case MSR_AMD_PPIN:
>          /* Not offered to guests. */
>          goto gp_fault;
>  
> @@ -237,6 +239,18 @@ int guest_rdmsr(struct vcpu *v, uint32_t
>                                     ARRAY_SIZE(msrs->dr_mask))];
>          break;
>  
> +    case MSR_PPIN_CTL:
> +        if ( cp->x86_vendor != X86_VENDOR_INTEL )
> +            goto gp_fault;
> +        *val = PPIN_LOCKOUT;
> +        break;
> +
> +    case MSR_AMD_PPIN_CTL:
> +        if ( !cp->extd.amd_ppin )
> +            goto gp_fault;
> +        *val = PPIN_LOCKOUT;
> +        break;
> +

The "not offered to guests" blocks should always be symmetric.  What
you've done here is half-virtualise something we have no intention to
ever virtualise for guests.

Both of these should be blanket #GP faults.  AMD should never be in the
position of seeing amd_ppin clear but PPIN_CTL returning LOCKOUT, and
while Intel does have model specific behaviour, whatever else might be
behind that MSR obviously shouldn't be leaking though either.

>          /*
>           * TODO: Implement when we have better topology representation.
>      case MSR_INTEL_CORE_THREAD_COUNT:
> @@ -273,10 +287,14 @@ int guest_wrmsr(struct vcpu *v, uint32_t
>      case MSR_INTEL_CORE_THREAD_COUNT:
>      case MSR_INTEL_PLATFORM_INFO:
>      case MSR_ARCH_CAPABILITIES:
> +    case MSR_PPIN:
> +    case MSR_AMD_PPIN:

... these should be in the lower block, as "not offered to guests" is
logically different from "we virtualise them, but they are read only".

~Andrew

>          /* Read-only */
>      case MSR_TSX_FORCE_ABORT:
>      case MSR_AMD64_LWP_CFG:
>      case MSR_AMD64_LWP_CBADDR:
> +    case MSR_PPIN_CTL:
> +    case MSR_AMD_PPIN_CTL:
>          /* Not offered to guests. */
>          goto gp_fault;
>  
>


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