[PATCH for-4.14] x86/msr: Disallow access to Processor Trace MSRs

[PATCH for-4.14] x86/msr: Disallow access to Processor Trace MSRs

[Andrew Cooper]

We do not expose the feature to guests, so should disallow access to the
respective MSRs.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Paul Durrant <paul@xen.org>
CC: Michał Leszczyński <michal.leszczynski@cert.pl>

Paul: For 4.14.  This needs backporting to older trees as well.

Michał: CC'ing, just to keep you in the loop.  Xen has some dubious default
MSR semantics which we're still in the middle of untangling in a backwards
compatible way.  Patches like this will eventually not be necessary, but they
are for now.
---
 xen/arch/x86/msr.c              | 12 ++++++++++++
 xen/include/asm-x86/msr-index.h |  8 ++++++++
 2 files changed, 20 insertions(+)

diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index 0bfb5839b2..05afe601a8 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -168,6 +168,12 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val)
     case MSR_TSX_FORCE_ABORT:
     case MSR_TSX_CTRL:
     case MSR_MCU_OPT_CTRL:
+    case MSR_RTIT_OUTPUT_BASE:
+    case MSR_RTIT_OUTPUT_MASK:
+    case MSR_RTIT_CTL:
+    case MSR_RTIT_STATUS:
+    case MSR_RTIT_CR3_MATCH:
+    case MSR_RTIT_ADDR_A(0) ... MSR_RTIT_ADDR_B(3):
     case MSR_U_CET:
     case MSR_S_CET:
     case MSR_PL0_SSP ... MSR_INTERRUPT_SSP_TABLE:
@@ -329,6 +335,12 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
     case MSR_TSX_FORCE_ABORT:
     case MSR_TSX_CTRL:
     case MSR_MCU_OPT_CTRL:
+    case MSR_RTIT_OUTPUT_BASE:
+    case MSR_RTIT_OUTPUT_MASK:
+    case MSR_RTIT_CTL:
+    case MSR_RTIT_STATUS:
+    case MSR_RTIT_CR3_MATCH:
+    case MSR_RTIT_ADDR_A(0) ... MSR_RTIT_ADDR_B(3):
     case MSR_U_CET:
     case MSR_S_CET:
     case MSR_PL0_SSP ... MSR_INTERRUPT_SSP_TABLE:
diff --git a/xen/include/asm-x86/msr-index.h b/xen/include/asm-x86/msr-index.h
index b328a47ed8..0fe98af923 100644
--- a/xen/include/asm-x86/msr-index.h
+++ b/xen/include/asm-x86/msr-index.h
@@ -69,6 +69,14 @@
 #define MSR_MCU_OPT_CTRL                    0x00000123
 #define  MCU_OPT_CTRL_RNGDS_MITG_DIS        (_AC(1, ULL) <<  0)
 
+#define MSR_RTIT_OUTPUT_BASE                0x00000560
+#define MSR_RTIT_OUTPUT_MASK                0x00000561
+#define MSR_RTIT_CTL                        0x00000570
+#define MSR_RTIT_STATUS                     0x00000571
+#define MSR_RTIT_CR3_MATCH                  0x00000572
+#define MSR_RTIT_ADDR_A(n)                 (0x00000580 + (n) * 2)
+#define MSR_RTIT_ADDR_B(n)                 (0x00000581 + (n) * 2)
+
 #define MSR_U_CET                           0x000006a0
 #define MSR_S_CET                           0x000006a2
 #define  CET_SHSTK_EN                       (_AC(1, ULL) <<  0)
-- 
2.11.0

Re: [PATCH for-4.14] x86/msr: Disallow access to Processor Trace MSRs

[Michał Leszczyński]

----- 19 cze 2020 o 13:58, Andrew Cooper andrew.cooper3@citrix.com napisał(a):

> We do not expose the feature to guests, so should disallow access to the
> respective MSRs.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wl@xen.org>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Paul Durrant <paul@xen.org>
> CC: Michał Leszczyński <michal.leszczynski@cert.pl>
> 
> Paul: For 4.14.  This needs backporting to older trees as well.
> 
> Michał: CC'ing, just to keep you in the loop.  Xen has some dubious default
> MSR semantics which we're still in the middle of untangling in a backwards
> compatible way.  Patches like this will eventually not be necessary, but they
> are for now.


As for external IPT monitoring, it would be best if the VM would think
that IPT is simply not supported at all by the underlying hypervisor.


Best regards,
Michał Leszczyński
CERT Polska

Re: [PATCH for-4.14] x86/msr: Disallow access to Processor Trace MSRs

[Jan Beulich]

On 19.06.2020 14:10, Michał Leszczyński wrote:
> ----- 19 cze 2020 o 13:58, Andrew Cooper andrew.cooper3@citrix.com napisał(a):
> 
>> We do not expose the feature to guests, so should disallow access to the
>> respective MSRs.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Wei Liu <wl@xen.org>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Paul Durrant <paul@xen.org>
>> CC: Michał Leszczyński <michal.leszczynski@cert.pl>
>>
>> Paul: For 4.14.  This needs backporting to older trees as well.
>>
>> Michał: CC'ing, just to keep you in the loop.  Xen has some dubious default
>> MSR semantics which we're still in the middle of untangling in a backwards
>> compatible way.  Patches like this will eventually not be necessary, but they
>> are for now.
> 
> 
> As for external IPT monitoring, it would be best if the VM would think
> that IPT is simply not supported at all by the underlying hypervisor.

This is already the case, isn't it? Yet not reporting a feature may
not keep a guest from trying to access the respective MSRs.

Jan

Re: [PATCH for-4.14] x86/msr: Disallow access to Processor Trace MSRs

[Michał Leszczyński]

----- 19 cze 2020 o 14:49, Jan Beulich jbeulich@suse.com napisał(a):

> On 19.06.2020 14:10, Michał Leszczyński wrote:
>> ----- 19 cze 2020 o 13:58, Andrew Cooper andrew.cooper3@citrix.com napisał(a):
>> 
>>> We do not expose the feature to guests, so should disallow access to the
>>> respective MSRs.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> ---
>>> CC: Jan Beulich <JBeulich@suse.com>
>>> CC: Wei Liu <wl@xen.org>
>>> CC: Roger Pau Monné <roger.pau@citrix.com>
>>> CC: Paul Durrant <paul@xen.org>
>>> CC: Michał Leszczyński <michal.leszczynski@cert.pl>
>>>
>>> Paul: For 4.14.  This needs backporting to older trees as well.
>>>
>>> Michał: CC'ing, just to keep you in the loop.  Xen has some dubious default
>>> MSR semantics which we're still in the middle of untangling in a backwards
>>> compatible way.  Patches like this will eventually not be necessary, but they
>>> are for now.
>> 
>> 
>> As for external IPT monitoring, it would be best if the VM would think
>> that IPT is simply not supported at all by the underlying hypervisor.
> 
> This is already the case, isn't it? Yet not reporting a feature may
> not keep a guest from trying to access the respective MSRs.
> 
> Jan


Okay, understood :)

ml

Re: [PATCH for-4.14] x86/msr: Disallow access to Processor Trace MSRs

[Andrew Cooper]

On 19/06/2020 13:57, Michał Leszczyński wrote:
> ----- 19 cze 2020 o 14:49, Jan Beulich jbeulich@suse.com napisał(a):
>
>> On 19.06.2020 14:10, Michał Leszczyński wrote:
>>> ----- 19 cze 2020 o 13:58, Andrew Cooper andrew.cooper3@citrix.com napisał(a):
>>>
>>>> We do not expose the feature to guests, so should disallow access to the
>>>> respective MSRs.
>>>>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> ---
>>>> CC: Jan Beulich <JBeulich@suse.com>
>>>> CC: Wei Liu <wl@xen.org>
>>>> CC: Roger Pau Monné <roger.pau@citrix.com>
>>>> CC: Paul Durrant <paul@xen.org>
>>>> CC: Michał Leszczyński <michal.leszczynski@cert.pl>
>>>>
>>>> Paul: For 4.14.  This needs backporting to older trees as well.
>>>>
>>>> Michał: CC'ing, just to keep you in the loop.  Xen has some dubious default
>>>> MSR semantics which we're still in the middle of untangling in a backwards
>>>> compatible way.  Patches like this will eventually not be necessary, but they
>>>> are for now.
>>>
>>> As for external IPT monitoring, it would be best if the VM would think
>>> that IPT is simply not supported at all by the underlying hypervisor.
>> This is already the case, isn't it? Yet not reporting a feature may
>> not keep a guest from trying to access the respective MSRs.
>>
>> Jan
>
> Okay, understood :)

Hiding bits in CPUID doesn't magically make the feature disappear out of
the pipeline.

Some things we can effectively disable (using suitable intercepts to
audit changes to control registers), but for most instruction groups,
its trivial to discover if the pipeline supports them, via fault analysis.

Despite the software manuals being clear on the matter, not all code
checks CPUID properly before poking features.  If anything in your VM
does do this, then it is likely to crash on migrate, so wherever
possible, block access at all levels, not just in CPUID.

(Windows DRM/Anti-cheat systems seem to have a particular knack for
finding features we haven't disabled properly, and architectural corner
cases we don't cope with correctly.)

~Andrew

Re: [PATCH for-4.14] x86/msr: Disallow access to Processor Trace MSRs

[Jan Beulich]

On 19.06.2020 13:58, Andrew Cooper wrote:
> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -168,6 +168,12 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val)
>      case MSR_TSX_FORCE_ABORT:
>      case MSR_TSX_CTRL:
>      case MSR_MCU_OPT_CTRL:
> +    case MSR_RTIT_OUTPUT_BASE:
> +    case MSR_RTIT_OUTPUT_MASK:
> +    case MSR_RTIT_CTL:
> +    case MSR_RTIT_STATUS:
> +    case MSR_RTIT_CR3_MATCH:
> +    case MSR_RTIT_ADDR_A(0) ... MSR_RTIT_ADDR_B(3):

The respective CPUID field is 3 bits wide, so wouldn't it be better
to cover the full possible range (0...6 afaict)?

Jan

Re: [PATCH for-4.14] x86/msr: Disallow access to Processor Trace MSRs

[Andrew Cooper]

On 19/06/2020 13:48, Jan Beulich wrote:
> On 19.06.2020 13:58, Andrew Cooper wrote:
>> --- a/xen/arch/x86/msr.c
>> +++ b/xen/arch/x86/msr.c
>> @@ -168,6 +168,12 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val)
>>      case MSR_TSX_FORCE_ABORT:
>>      case MSR_TSX_CTRL:
>>      case MSR_MCU_OPT_CTRL:
>> +    case MSR_RTIT_OUTPUT_BASE:
>> +    case MSR_RTIT_OUTPUT_MASK:
>> +    case MSR_RTIT_CTL:
>> +    case MSR_RTIT_STATUS:
>> +    case MSR_RTIT_CR3_MATCH:
>> +    case MSR_RTIT_ADDR_A(0) ... MSR_RTIT_ADDR_B(3):
> The respective CPUID field is 3 bits wide, so wouldn't it be better
> to cover the full possible range (0...6 afaict)?

Last time I tried, you objected to me covering MSR ranges which weren't
defined.

If you want to extend the range like that, it ought to be
MSR_RTIT_OUTPUT_BASE ... MSR_RTIT_ADDR_B(7) to cover the entire area
which seems to be exclusively for PT.

~Andrew

Re: [PATCH for-4.14] x86/msr: Disallow access to Processor Trace MSRs

[Jan Beulich]

On 19.06.2020 15:28, Andrew Cooper wrote:
> On 19/06/2020 13:48, Jan Beulich wrote:
>> On 19.06.2020 13:58, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/msr.c
>>> +++ b/xen/arch/x86/msr.c
>>> @@ -168,6 +168,12 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val)
>>>      case MSR_TSX_FORCE_ABORT:
>>>      case MSR_TSX_CTRL:
>>>      case MSR_MCU_OPT_CTRL:
>>> +    case MSR_RTIT_OUTPUT_BASE:
>>> +    case MSR_RTIT_OUTPUT_MASK:
>>> +    case MSR_RTIT_CTL:
>>> +    case MSR_RTIT_STATUS:
>>> +    case MSR_RTIT_CR3_MATCH:
>>> +    case MSR_RTIT_ADDR_A(0) ... MSR_RTIT_ADDR_B(3):
>> The respective CPUID field is 3 bits wide, so wouldn't it be better
>> to cover the full possible range (0...6 afaict)?
> 
> Last time I tried, you objected to me covering MSR ranges which weren't
> defined.

Wasn't that for a range where some number had been re-used from
earlier models (with entirely different purpose)?

> If you want to extend the range like that, it ought to be
> MSR_RTIT_OUTPUT_BASE ... MSR_RTIT_ADDR_B(7) to cover the entire area
> which seems to be exclusively for PT.

I'd be okay with that, just that I'm not sure about (7): While
SDM Vol 2 oddly enough doesn't seem to list anything for leaf 7
subleaf 1 (or I'm sufficiently blind today), Vol 4 clearly says
for n=0...3 "If CPUID.(EAX=07H,ECX=1):EAX[2:0] > <n>", and the
field obviously can't be > 7.

Jan

Re: [PATCH for-4.14] x86/msr: Disallow access to Processor Trace MSRs

[Andrew Cooper]

On 19/06/2020 14:39, Jan Beulich wrote:
> On 19.06.2020 15:28, Andrew Cooper wrote:
>> On 19/06/2020 13:48, Jan Beulich wrote:
>>> On 19.06.2020 13:58, Andrew Cooper wrote:
>>>> --- a/xen/arch/x86/msr.c
>>>> +++ b/xen/arch/x86/msr.c
>>>> @@ -168,6 +168,12 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val)
>>>>      case MSR_TSX_FORCE_ABORT:
>>>>      case MSR_TSX_CTRL:
>>>>      case MSR_MCU_OPT_CTRL:
>>>> +    case MSR_RTIT_OUTPUT_BASE:
>>>> +    case MSR_RTIT_OUTPUT_MASK:
>>>> +    case MSR_RTIT_CTL:
>>>> +    case MSR_RTIT_STATUS:
>>>> +    case MSR_RTIT_CR3_MATCH:
>>>> +    case MSR_RTIT_ADDR_A(0) ... MSR_RTIT_ADDR_B(3):
>>> The respective CPUID field is 3 bits wide, so wouldn't it be better
>>> to cover the full possible range (0...6 afaict)?
>> Last time I tried, you objected to me covering MSR ranges which weren't
>> defined.
> Wasn't that for a range where some number had been re-used from
> earlier models (with entirely different purpose)?

I don't recall, but the answer isn't relevant to whether the MSRs at
those indices ought to be available to guests.

>> If you want to extend the range like that, it ought to be
>> MSR_RTIT_OUTPUT_BASE ... MSR_RTIT_ADDR_B(7) to cover the entire area
>> which seems to be exclusively for PT.
> I'd be okay with that, just that I'm not sure about (7): While
> SDM Vol 2 oddly enough doesn't seem to list anything for leaf 7
> subleaf 1 (or I'm sufficiently blind today), Vol 4 clearly says
> for n=0...3 "If CPUID.(EAX=07H,ECX=1):EAX[2:0] > <n>", and the
> field obviously can't be > 7.

7 gives the top of the bank of MSRs.  It isn't related to CPUID data.

~Andrew

Re: [PATCH for-4.14] x86/msr: Disallow access to Processor Trace MSRs

[Jan Beulich]

On 22.06.2020 19:16, Andrew Cooper wrote:
> On 19/06/2020 14:39, Jan Beulich wrote:
>> On 19.06.2020 15:28, Andrew Cooper wrote:
>>> On 19/06/2020 13:48, Jan Beulich wrote:
>>>> On 19.06.2020 13:58, Andrew Cooper wrote:
>>>>> --- a/xen/arch/x86/msr.c
>>>>> +++ b/xen/arch/x86/msr.c
>>>>> @@ -168,6 +168,12 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val)
>>>>>      case MSR_TSX_FORCE_ABORT:
>>>>>      case MSR_TSX_CTRL:
>>>>>      case MSR_MCU_OPT_CTRL:
>>>>> +    case MSR_RTIT_OUTPUT_BASE:
>>>>> +    case MSR_RTIT_OUTPUT_MASK:
>>>>> +    case MSR_RTIT_CTL:
>>>>> +    case MSR_RTIT_STATUS:
>>>>> +    case MSR_RTIT_CR3_MATCH:
>>>>> +    case MSR_RTIT_ADDR_A(0) ... MSR_RTIT_ADDR_B(3):
>>>> The respective CPUID field is 3 bits wide, so wouldn't it be better
>>>> to cover the full possible range (0...6 afaict)?
>>> Last time I tried, you objected to me covering MSR ranges which weren't
>>> defined.
>> Wasn't that for a range where some number had been re-used from
>> earlier models (with entirely different purpose)?
> 
> I don't recall, but the answer isn't relevant to whether the MSRs at
> those indices ought to be available to guests.
> 
>>> If you want to extend the range like that, it ought to be
>>> MSR_RTIT_OUTPUT_BASE ... MSR_RTIT_ADDR_B(7) to cover the entire area
>>> which seems to be exclusively for PT.
>> I'd be okay with that, just that I'm not sure about (7): While
>> SDM Vol 2 oddly enough doesn't seem to list anything for leaf 7
>> subleaf 1 (or I'm sufficiently blind today), Vol 4 clearly says
>> for n=0...3 "If CPUID.(EAX=07H,ECX=1):EAX[2:0] > <n>", and the
>> field obviously can't be > 7.
> 
> 7 gives the top of the bank of MSRs.  It isn't related to CPUID data.

Well, okay - let's go that route then?

Jan