Re: [Xen-devel] [PATCH 6/6] x86/msr: Blacklist various MSRs which guests definitely shouldn't be using

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Feb 26, 2018 at 05:35:19PM +0000, Andrew Cooper wrote:
> The main purpose is to blacklist the Intel Resource Director Technology MSRs.
> We do not yet virtualise support for guests, but Linux has been observed to
> probe for these MSRs without checking CPUID first.
> 
> The architecturally inaccessable ranges don't need to fall back into the
> legacy ranges, because they are not going to eventually evaluate as
> accessible.
> 
> The Silicon Debug interface will probably never be virtualised for guests, but
> doesn't want to leak through from real hardware.  SGX isn't yet virtualised,
> but likely will be in the future.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Sergey Dyasli <sergey.dyasli@xxxxxxxxxx>
> ---
>  xen/arch/x86/msr.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
> index 3cb4158..0237637 100644
> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -158,6 +158,10 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, 
> uint64_t *val)
>          *val = vp->spec_ctrl.raw;
>          break;
>  
> +    case 0x8c ... 0x8f: /* SGX Launch Enclave Public Key Hash. */
> +        /* Not implemented yet. */
> +        goto gp_fault;
> +
>      case MSR_INTEL_PLATFORM_INFO:
>          if ( !dp->plaform_info.available )
>              goto gp_fault;
> @@ -183,6 +187,15 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, 
> uint64_t *val)
>          ret = guest_rdmsr_x2apic(v, msr, val);
>          goto out;
>  
> +    case 0xc80:
> +        /* Silicon Debug Inferface not advertised to guests. */
> +        goto gp_fault;
> +
> +    case 0xc81 ... 0xc8f: /* Misc RDT MSRs. */
> +    case 0xc90 ... 0xd8f: /* CAT Mask registers. */
> +        /* Not implemented yet. */
> +        goto gp_fault;

You have sorted them by index, I would maybe group them together in
order to use a single goto.

I would however prefer to have defines for those values.

> +
>      case 0x40000000 ... 0x400001ff:
>          if ( is_viridian_domain(d) )
>          {
> @@ -196,6 +209,15 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, 
> uint64_t *val)
>          goto out;
>  
>      default:
> +        /*
> +         * Blacklist the architecturally inaccessable MSRs. No point 
> wandering
> +         * the legacy handlers.
> +         */
> +        if ( msr > 0x1fff &&
> +             (msr < 0xc0000000 || msr > 0xc0001fff) &&
> +             (msr < 0xc0010000 || msr > 0xc0011fff) )

Maybe this could be a macro in order to avoid duplication with the
chunk in wrmsr? (MSR_INACCESSIBLE or some such).

Thanks, Roger.

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