[PATCH] x86/svm: Intercept and terminate RDPRU with #UD

Andrew Cooper posted 1 patch 2 weeks, 3 days ago
xen/arch/x86/hvm/svm/svm.c         | 5 ++---
xen/arch/x86/hvm/svm/vmcb.c        | 3 ++-
xen/include/asm-x86/hvm/svm/vmcb.h | 4 +++-
3 files changed, 7 insertions(+), 5 deletions(-)

[PATCH] x86/svm: Intercept and terminate RDPRU with #UD

Posted by Andrew Cooper 2 weeks, 3 days ago
The RDPRU instruction isn't supported at all (and it is unclear how this can
ever be offered safely to guests).  However, a guest which ignores CPUID and
blindly executes RDPRU will find that it functions.

Use the intercept and terminate with #UD.  While at it, fold SKINIT into the
same "unconditionally disabled" path.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>

I could have sworn that I'd posted this before, but I can't locate any
evidence of it.  I've got a separate patch adding the CPUID infrastructure for
rdpru, but that is better left until we've got more libx86 levelling logic in
place.
---
 xen/arch/x86/hvm/svm/svm.c         | 5 ++---
 xen/arch/x86/hvm/svm/vmcb.c        | 3 ++-
 xen/include/asm-x86/hvm/svm/vmcb.h | 4 +++-
 3 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 8dc92c8b9f96..18c4831f98ad 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2945,6 +2945,8 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
 
     case VMEXIT_MONITOR:
     case VMEXIT_MWAIT:
+    case VMEXIT_SKINIT:
+    case VMEXIT_RDPRU:
         hvm_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC);
         break;
 
@@ -2963,9 +2965,6 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
     case VMEXIT_CLGI:
         svm_vmexit_do_clgi(regs, v);
         break;
-    case VMEXIT_SKINIT:
-        hvm_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC);
-        break;
 
     case VMEXIT_XSETBV:
         if ( vmcb_get_cpl(vmcb) )
diff --git a/xen/arch/x86/hvm/svm/vmcb.c b/xen/arch/x86/hvm/svm/vmcb.c
index 55da9302e5d7..565e997155f2 100644
--- a/xen/arch/x86/hvm/svm/vmcb.c
+++ b/xen/arch/x86/hvm/svm/vmcb.c
@@ -70,7 +70,8 @@ static int construct_vmcb(struct vcpu *v)
         GENERAL2_INTERCEPT_STGI        | GENERAL2_INTERCEPT_CLGI        |
         GENERAL2_INTERCEPT_SKINIT      | GENERAL2_INTERCEPT_MWAIT       |
         GENERAL2_INTERCEPT_WBINVD      | GENERAL2_INTERCEPT_MONITOR     |
-        GENERAL2_INTERCEPT_XSETBV      | GENERAL2_INTERCEPT_ICEBP;
+        GENERAL2_INTERCEPT_XSETBV      | GENERAL2_INTERCEPT_ICEBP       |
+        GENERAL2_INTERCEPT_RDPRU;
 
     /* Intercept all debug-register writes. */
     vmcb->_dr_intercepts = ~0u;
diff --git a/xen/include/asm-x86/hvm/svm/vmcb.h b/xen/include/asm-x86/hvm/svm/vmcb.h
index 4fa2ddfb2ff2..ed7cebea7174 100644
--- a/xen/include/asm-x86/hvm/svm/vmcb.h
+++ b/xen/include/asm-x86/hvm/svm/vmcb.h
@@ -74,7 +74,8 @@ enum GenericIntercept2bits
     GENERAL2_INTERCEPT_MONITOR = 1 << 10,
     GENERAL2_INTERCEPT_MWAIT   = 1 << 11,
     GENERAL2_INTERCEPT_MWAIT_CONDITIONAL = 1 << 12,
-    GENERAL2_INTERCEPT_XSETBV  = 1 << 13
+    GENERAL2_INTERCEPT_XSETBV  = 1 << 13,
+    GENERAL2_INTERCEPT_RDPRU   = 1 << 14,
 };
 
 
@@ -300,6 +301,7 @@ enum VMEXIT_EXITCODE
     VMEXIT_MWAIT            = 139, /* 0x8b */
     VMEXIT_MWAIT_CONDITIONAL= 140, /* 0x8c */
     VMEXIT_XSETBV           = 141, /* 0x8d */
+    VMEXIT_RDPRU            = 142, /* 0x8e */
     VMEXIT_NPF              = 1024, /* 0x400, nested paging fault */
     VMEXIT_INVALID          =  -1
 };
-- 
2.11.0


Re: [PATCH] x86/svm: Intercept and terminate RDPRU with #UD

Posted by Jan Beulich 2 weeks, 2 days ago
On 08.09.2021 18:19, Andrew Cooper wrote:
> The RDPRU instruction isn't supported at all (and it is unclear how this can
> ever be offered safely to guests).

An implicit hint to me to consider "x86emul: support RDPRU" rejected? That's
still in my queue waiting for ...

>  However, a guest which ignores CPUID and
> blindly executes RDPRU will find that it functions.
> 
> Use the intercept and terminate with #UD.  While at it, fold SKINIT into the
> same "unconditionally disabled" path.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
> 
> I could have sworn that I'd posted this before, but I can't locate any
> evidence of it.  I've got a separate patch adding the CPUID infrastructure for
> rdpru, but that is better left until we've got more libx86 levelling logic in
> place.

... this. Which - if exposure to guests makes no sense - would seem pretty
pointless then as well?

> --- a/xen/arch/x86/hvm/svm/vmcb.c
> +++ b/xen/arch/x86/hvm/svm/vmcb.c
> @@ -70,7 +70,8 @@ static int construct_vmcb(struct vcpu *v)
>          GENERAL2_INTERCEPT_STGI        | GENERAL2_INTERCEPT_CLGI        |
>          GENERAL2_INTERCEPT_SKINIT      | GENERAL2_INTERCEPT_MWAIT       |
>          GENERAL2_INTERCEPT_WBINVD      | GENERAL2_INTERCEPT_MONITOR     |
> -        GENERAL2_INTERCEPT_XSETBV      | GENERAL2_INTERCEPT_ICEBP;
> +        GENERAL2_INTERCEPT_XSETBV      | GENERAL2_INTERCEPT_ICEBP       |
> +        GENERAL2_INTERCEPT_RDPRU;

Some of the other intercepts here suggest it is okay to enable ones
in the absence of support in the underlying hardware, but I thought
I'd double check. I couldn't find any statement either way in the PM.
Assuming this is fine
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan


Re: [PATCH] x86/svm: Intercept and terminate RDPRU with #UD

Posted by Andrew Cooper 2 weeks, 2 days ago
On 09/09/2021 10:57, Jan Beulich wrote:
> On 08.09.2021 18:19, Andrew Cooper wrote:
>> The RDPRU instruction isn't supported at all (and it is unclear how this can
>> ever be offered safely to guests).
> An implicit hint to me to consider "x86emul: support RDPRU" rejected? That's
> still in my queue waiting for ...
>
>>  However, a guest which ignores CPUID and
>> blindly executes RDPRU will find that it functions.
>>
>> Use the intercept and terminate with #UD.  While at it, fold SKINIT into the
>> same "unconditionally disabled" path.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Wei Liu <wl@xen.org>
>>
>> I could have sworn that I'd posted this before, but I can't locate any
>> evidence of it.  I've got a separate patch adding the CPUID infrastructure for
>> rdpru, but that is better left until we've got more libx86 levelling logic in
>> place.
> ... this. Which - if exposure to guests makes no sense - would seem pretty
> pointless then as well?

Well - given how many people want aperf/mperf, I suspect we might end up
having it as an opt-in only thing, so no - this isn't a rejection of the
x86emul work.

We can *almost* provide a safe way for VM's to use this infrastructure
if we had something like Linux's restartable sequences infrastructure. 
In that case, only an SMI would mess things up, from the guest kernel's
perspective.

On the other hand, it's probably easier to lobby Intel and AMD for an
interface here which isn't fundamentally broken in the face of NMI/SMI/etc.

>
>> --- a/xen/arch/x86/hvm/svm/vmcb.c
>> +++ b/xen/arch/x86/hvm/svm/vmcb.c
>> @@ -70,7 +70,8 @@ static int construct_vmcb(struct vcpu *v)
>>          GENERAL2_INTERCEPT_STGI        | GENERAL2_INTERCEPT_CLGI        |
>>          GENERAL2_INTERCEPT_SKINIT      | GENERAL2_INTERCEPT_MWAIT       |
>>          GENERAL2_INTERCEPT_WBINVD      | GENERAL2_INTERCEPT_MONITOR     |
>> -        GENERAL2_INTERCEPT_XSETBV      | GENERAL2_INTERCEPT_ICEBP;
>> +        GENERAL2_INTERCEPT_XSETBV      | GENERAL2_INTERCEPT_ICEBP       |
>> +        GENERAL2_INTERCEPT_RDPRU;
> Some of the other intercepts here suggest it is okay to enable ones
> in the absence of support in the underlying hardware, but I thought
> I'd double check. I couldn't find any statement either way in the PM.
> Assuming this is fine

They're just bits in memory.  Older CPUs will ignore them, and indeed -
pre-RDPRU hardware is fine running with this intercept bit set.

Honestly, I've got half a mind to default the intercepts to ~0 rather
than 0.  For running older Xen on new hardware, it will lead to fewer
unexpected surprises.

> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.

~Andrew


Re: [PATCH] x86/svm: Intercept and terminate RDPRU with #UD

Posted by Jan Beulich 2 weeks, 2 days ago
On 09.09.2021 13:34, Andrew Cooper wrote:
> On 09/09/2021 10:57, Jan Beulich wrote:
>> On 08.09.2021 18:19, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/hvm/svm/vmcb.c
>>> +++ b/xen/arch/x86/hvm/svm/vmcb.c
>>> @@ -70,7 +70,8 @@ static int construct_vmcb(struct vcpu *v)
>>>          GENERAL2_INTERCEPT_STGI        | GENERAL2_INTERCEPT_CLGI        |
>>>          GENERAL2_INTERCEPT_SKINIT      | GENERAL2_INTERCEPT_MWAIT       |
>>>          GENERAL2_INTERCEPT_WBINVD      | GENERAL2_INTERCEPT_MONITOR     |
>>> -        GENERAL2_INTERCEPT_XSETBV      | GENERAL2_INTERCEPT_ICEBP;
>>> +        GENERAL2_INTERCEPT_XSETBV      | GENERAL2_INTERCEPT_ICEBP       |
>>> +        GENERAL2_INTERCEPT_RDPRU;
>> Some of the other intercepts here suggest it is okay to enable ones
>> in the absence of support in the underlying hardware, but I thought
>> I'd double check. I couldn't find any statement either way in the PM.
>> Assuming this is fine
> 
> They're just bits in memory.  Older CPUs will ignore them, and indeed -
> pre-RDPRU hardware is fine running with this intercept bit set.
> 
> Honestly, I've got half a mind to default the intercepts to ~0 rather
> than 0.  For running older Xen on new hardware, it will lead to fewer
> unexpected surprises.

I, too, was considering this, but then we have

    default:
    unexpected_exit_type:
        gprintk(XENLOG_ERR, "Unexpected vmexit: reason %#"PRIx64", "
                "exitinfo1 %#"PRIx64", exitinfo2 %#"PRIx64"\n",
                exit_reason, vmcb->exitinfo1, vmcb->exitinfo2);
    crash_or_fault:
        svm_crash_or_fault(v);
        break;
    }

at the bottom of the switch() statement handling the exit codes.
Getting crashed (or crashing because of an unexpected fault) is
surely a surprise as well (to a guest).

Jan


Re: [PATCH] x86/svm: Intercept and terminate RDPRU with #UD

Posted by Andrew Cooper 2 weeks, 2 days ago
On 09/09/2021 12:47, Jan Beulich wrote:
> On 09.09.2021 13:34, Andrew Cooper wrote:
>> On 09/09/2021 10:57, Jan Beulich wrote:
>>> On 08.09.2021 18:19, Andrew Cooper wrote:
>>>> --- a/xen/arch/x86/hvm/svm/vmcb.c
>>>> +++ b/xen/arch/x86/hvm/svm/vmcb.c
>>>> @@ -70,7 +70,8 @@ static int construct_vmcb(struct vcpu *v)
>>>>          GENERAL2_INTERCEPT_STGI        | GENERAL2_INTERCEPT_CLGI        |
>>>>          GENERAL2_INTERCEPT_SKINIT      | GENERAL2_INTERCEPT_MWAIT       |
>>>>          GENERAL2_INTERCEPT_WBINVD      | GENERAL2_INTERCEPT_MONITOR     |
>>>> -        GENERAL2_INTERCEPT_XSETBV      | GENERAL2_INTERCEPT_ICEBP;
>>>> +        GENERAL2_INTERCEPT_XSETBV      | GENERAL2_INTERCEPT_ICEBP       |
>>>> +        GENERAL2_INTERCEPT_RDPRU;
>>> Some of the other intercepts here suggest it is okay to enable ones
>>> in the absence of support in the underlying hardware, but I thought
>>> I'd double check. I couldn't find any statement either way in the PM.
>>> Assuming this is fine
>> They're just bits in memory.  Older CPUs will ignore them, and indeed -
>> pre-RDPRU hardware is fine running with this intercept bit set.
>>
>> Honestly, I've got half a mind to default the intercepts to ~0 rather
>> than 0.  For running older Xen on new hardware, it will lead to fewer
>> unexpected surprises.
> I, too, was considering this, but then we have
>
>     default:
>     unexpected_exit_type:
>         gprintk(XENLOG_ERR, "Unexpected vmexit: reason %#"PRIx64", "
>                 "exitinfo1 %#"PRIx64", exitinfo2 %#"PRIx64"\n",
>                 exit_reason, vmcb->exitinfo1, vmcb->exitinfo2);
>     crash_or_fault:
>         svm_crash_or_fault(v);
>         break;
>     }
>
> at the bottom of the switch() statement handling the exit codes.
> Getting crashed (or crashing because of an unexpected fault) is
> surely a surprise as well (to a guest).

It is less bad than the alternative, which was failing to intercept e.g.
XSETBV.

Something's going to crash or malfunction either way.  Intercepting
everything makes it far more obvious, and limits the damage to only the
offending guest.

And yes - this is once again why we really need a credible support
statement for CPUs.

~Andrew