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

[Zhao Liu <zhao1.liu@intel.com>]

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;
         }