[PATCH] target/i386: do not crash if microvm guest uses SGX CPUID leaves

[PATCH] target/i386: do not crash if microvm guest uses SGX CPUID leaves

[Paolo Bonzini]

sgx_epc_get_section assumes a PC platform is in use:

bool sgx_epc_get_section(int section_nr, uint64_t *addr, uint64_t *size)
{
    PCMachineState *pcms = PC_MACHINE(qdev_get_machine());

However, sgx_epc_get_section is called by CPUID regardless of whether
SGX state has been initialized or which platform is in use.  Check
whether the machine has the right QOM class and if not behave as if
there are no EPC sections.

Fixes: 1dec2e1f19f ("i386: Update SGX CPUID info according to hardware/KVM/user input", 2021-09-30)
Cc: qemu-stable@nongnu.org
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2142
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/i386/sgx.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/i386/sgx.c b/hw/i386/sgx.c
index de76397bcfb..25b2055d653 100644
--- a/hw/i386/sgx.c
+++ b/hw/i386/sgx.c
@@ -266,10 +266,12 @@ void hmp_info_sgx(Monitor *mon, const QDict *qdict)
 
 bool sgx_epc_get_section(int section_nr, uint64_t *addr, uint64_t *size)
 {
-    PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
+    PCMachineState *pcms =
+        (PCMachineState *)object_dynamic_cast(qdev_get_machine(),
+                                              TYPE_PC_MACHINE);
     SGXEPCDevice *epc;
 
-    if (pcms->sgx_epc.size == 0 || pcms->sgx_epc.nr_sections <= section_nr) {
+    if (!pcms || pcms->sgx_epc.size == 0 || pcms->sgx_epc.nr_sections <= section_nr) {
         return true;
     }
 
-- 
2.45.2

Re: [PATCH] target/i386: do not crash if microvm guest uses SGX CPUID leaves

[Zhao Liu]

Hi Paolo,

On Tue, Jul 16, 2024 at 06:55:30PM +0200, Paolo Bonzini wrote:
> Date: Tue, 16 Jul 2024 18:55:30 +0200
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH] target/i386: do not crash if microvm guest uses SGX CPUID
>  leaves
> X-Mailer: git-send-email 2.45.2
> 
> sgx_epc_get_section assumes a PC platform is in use:
> 
> bool sgx_epc_get_section(int section_nr, uint64_t *addr, uint64_t *size)
> {
>     PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
> 
> However, sgx_epc_get_section is called by CPUID regardless of whether
> SGX state has been initialized or which platform is in use.  Check
> whether the machine has the right QOM class and if not behave as if
> there are no EPC sections.
> 
> Fixes: 1dec2e1f19f ("i386: Update SGX CPUID info according to hardware/KVM/user input", 2021-09-30)
> Cc: qemu-stable@nongnu.org
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2142
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/i386/sgx.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/i386/sgx.c b/hw/i386/sgx.c
> index de76397bcfb..25b2055d653 100644
> --- a/hw/i386/sgx.c
> +++ b/hw/i386/sgx.c
> @@ -266,10 +266,12 @@ void hmp_info_sgx(Monitor *mon, const QDict *qdict)
>  
>  bool sgx_epc_get_section(int section_nr, uint64_t *addr, uint64_t *size)
>  {
> -    PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
> +    PCMachineState *pcms =
> +        (PCMachineState *)object_dynamic_cast(qdev_get_machine(),
> +                                              TYPE_PC_MACHINE);
>      SGXEPCDevice *epc;
>  
> -    if (pcms->sgx_epc.size == 0 || pcms->sgx_epc.nr_sections <= section_nr) {
> +    if (!pcms || pcms->sgx_epc.size == 0 || pcms->sgx_epc.nr_sections <= section_nr) {
>          return true;
>      }
>  

The above change is necessary...

...but it only avoids encoding sub-leafs CPUID.0x12.{0x2..N}, while
subleafs CPUID.0x12.{0x0,0x1} still have valid SGX information.

According to the error message in qmp_query_sgx(), sgx is only supported
on PC machines. So how about simply taking it a step further and masking
out the entire 0x12 leaf for microvm as well?

for example:

diff --git a/hw/i386/sgx-stub.c b/hw/i386/sgx-stub.c
index 16b1dfd90bb5..38ff75e9f377 100644
--- a/hw/i386/sgx-stub.c
+++ b/hw/i386/sgx-stub.c
@@ -32,6 +32,11 @@ void pc_machine_init_sgx_epc(PCMachineState *pcms)
     memset(&pcms->sgx_epc, 0, sizeof(SGXEPCState));
 }

+bool check_sgx_support(void)
+{
+    return false;
+}
+
 bool sgx_epc_get_section(int section_nr, uint64_t *addr, uint64_t *size)
 {
     return true;
diff --git a/hw/i386/sgx.c b/hw/i386/sgx.c
index de76397bcfb3..dcf178b1e755 100644
--- a/hw/i386/sgx.c
+++ b/hw/i386/sgx.c
@@ -264,6 +264,14 @@ void hmp_info_sgx(Monitor *mon, const QDict *qdict)
                    size);
 }

+bool check_sgx_support(void)
+{
+    if(!object_dynamic_cast(qdev_get_machine(), TYPE_X86_MACHINE)) {
+        return false;
+    }
+    return true;
+}
+
 bool sgx_epc_get_section(int section_nr, uint64_t *addr, uint64_t *size)
 {
     PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
diff --git a/include/hw/i386/sgx-epc.h b/include/hw/i386/sgx-epc.h
index 3e00efd870c9..41d55da47999 100644
--- a/include/hw/i386/sgx-epc.h
+++ b/include/hw/i386/sgx-epc.h
@@ -58,6 +58,7 @@ typedef struct SGXEPCState {
     int nr_sections;
 } SGXEPCState;

+bool check_sgx_support(void);
 bool sgx_epc_get_section(int section_nr, uint64_t *addr, uint64_t *size);
 void sgx_epc_build_srat(GArray *table_data);

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index c05765eeafc8..f0b464f7ea79 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -6702,7 +6702,8 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
     case 0x12:
 #ifndef CONFIG_USER_ONLY
         if (!kvm_enabled() ||
-            !(env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_SGX)) {
+            !(env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_SGX) ||
+            !check_sgx_support()) {
             *eax = *ebx = *ecx = *edx = 0;
             break;
         }