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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.12.2019 20:47, Andrew Cooper wrote:
> 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.

Well, the wrmsr side of things simply was the wrong way round
(see below) - with it flipped they then are 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.

In the interest of getting this ack-ed I might switch to the
blanket-#GP as you suggest, but I'm having trouble seeing why
giving back sane (and safe) values is wrong. This isn't meant
to indicate we might virtualize more of this. But why incur an
unnecessary #GP(0) in the guest when we can indicate the same
in a more "friendly" manner?

>> @@ -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".

Hmm, yes, I got these and ...

>>          /* 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;

... these the wrong way round.

Jan

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