[v2] Make x86 IOMMU driver support configurable

[PATCH v2 5/8] x86/iommu: the code addressing CVE-2011-1898 is VT-d specific

Posted by Xenia Ragiadakou 3 years, 1 month ago

The variable untrusted_msi indicates whether the system is vulnerable to
CVE-2011-1898. This vulnerablity is VT-d specific.
Place the code that addresses the issue under CONFIG_INTEL_IOMMU.

No functional change intended.

Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
---

Changes in v2:
  - replace CONFIG_INTEL_VTD with CONFIG_INTEL_IOMMU

 xen/arch/x86/include/asm/iommu.h | 2 ++
 xen/arch/x86/pv/hypercall.c      | 2 ++
 xen/arch/x86/x86_64/entry.S      | 2 ++
 3 files changed, 6 insertions(+)

diff --git a/xen/arch/x86/include/asm/iommu.h b/xen/arch/x86/include/asm/iommu.h
index fc0afe35bf..fb5fe4e1bf 100644
--- a/xen/arch/x86/include/asm/iommu.h
+++ b/xen/arch/x86/include/asm/iommu.h
@@ -127,7 +127,9 @@ int iommu_identity_mapping(struct domain *d, p2m_access_t p2ma,
                            unsigned int flag);
 void iommu_identity_map_teardown(struct domain *d);
 
+#ifdef CONFIG_INTEL_IOMMU
 extern bool untrusted_msi;
+#endif
 
 int pi_update_irte(const struct pi_desc *pi_desc, const struct pirq *pirq,
                    const uint8_t gvec);
diff --git a/xen/arch/x86/pv/hypercall.c b/xen/arch/x86/pv/hypercall.c
index 2eedfbfae8..9d616a0fc5 100644
--- a/xen/arch/x86/pv/hypercall.c
+++ b/xen/arch/x86/pv/hypercall.c
@@ -193,8 +193,10 @@ void pv_ring1_init_hypercall_page(void *p)
 
 void do_entry_int82(struct cpu_user_regs *regs)
 {
+#ifdef CONFIG_INTEL_IOMMU
     if ( unlikely(untrusted_msi) )
         check_for_unexpected_msi((uint8_t)regs->entry_vector);
+#endif
 
     _pv_hypercall(regs, true /* compat */);
 }
diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
index ae01285181..8f2fb36770 100644
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -406,11 +406,13 @@ ENTRY(int80_direct_trap)
 .Lint80_cr3_okay:
         sti
 
+#ifdef CONFIG_INTEL_IOMMU
         cmpb  $0,untrusted_msi(%rip)
 UNLIKELY_START(ne, msi_check)
         movl  $0x80,%edi
         call  check_for_unexpected_msi
 UNLIKELY_END(msi_check)
+#endif
 
         movq  STACK_CPUINFO_FIELD(current_vcpu)(%rbx), %rbx
 
-- 
2.37.2

Re: [PATCH v2 5/8] x86/iommu: the code addressing CVE-2011-1898 is VT-d specific

Posted by Jan Beulich 3 years ago

On 04.01.2023 09:44, Xenia Ragiadakou wrote:
> The variable untrusted_msi indicates whether the system is vulnerable to
> CVE-2011-1898. This vulnerablity is VT-d specific.

As per the reply by Andrew to v1, this vulnerability is generic to intremap-
incapable or intremap-disabled configurations. You want to say so. In turn
I wonder whether instead of the changes you're making you wouldn't want to
move the definition of the variable to xen/drivers/passthrough/x86/iommu.c.
A useful further step might be to guard its definition (not necessarily
its declaration; see replies to earlier patches) by CONFIG_PV instead (of
course I understand that's largely orthogonal to your series here, yet it
would fit easily with moving the definition).

> --- a/xen/arch/x86/include/asm/iommu.h
> +++ b/xen/arch/x86/include/asm/iommu.h
> @@ -127,7 +127,9 @@ int iommu_identity_mapping(struct domain *d, p2m_access_t p2ma,
>                             unsigned int flag);
>  void iommu_identity_map_teardown(struct domain *d);
>  
> +#ifdef CONFIG_INTEL_IOMMU
>  extern bool untrusted_msi;
> +#endif

As per above / earlier comments I don't think this part is needed in any
event.

> --- a/xen/arch/x86/pv/hypercall.c
> +++ b/xen/arch/x86/pv/hypercall.c
> @@ -193,8 +193,10 @@ void pv_ring1_init_hypercall_page(void *p)
>  
>  void do_entry_int82(struct cpu_user_regs *regs)
>  {
> +#ifdef CONFIG_INTEL_IOMMU
>      if ( unlikely(untrusted_msi) )
>          check_for_unexpected_msi((uint8_t)regs->entry_vector);
> +#endif
>  
>      _pv_hypercall(regs, true /* compat */);
>  }
> diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
> index ae01285181..8f2fb36770 100644
> --- a/xen/arch/x86/x86_64/entry.S
> +++ b/xen/arch/x86/x86_64/entry.S
> @@ -406,11 +406,13 @@ ENTRY(int80_direct_trap)
>  .Lint80_cr3_okay:
>          sti
>  
> +#ifdef CONFIG_INTEL_IOMMU
>          cmpb  $0,untrusted_msi(%rip)
>  UNLIKELY_START(ne, msi_check)
>          movl  $0x80,%edi
>          call  check_for_unexpected_msi
>  UNLIKELY_END(msi_check)
> +#endif
>  
>          movq  STACK_CPUINFO_FIELD(current_vcpu)(%rbx), %rbx
>

Re: [PATCH v2 5/8] x86/iommu: the code addressing CVE-2011-1898 is VT-d specific

Posted by Xenia Ragiadakou 3 years ago

On 1/12/23 14:01, Jan Beulich wrote:
> On 04.01.2023 09:44, Xenia Ragiadakou wrote:
>> The variable untrusted_msi indicates whether the system is vulnerable to
>> CVE-2011-1898. This vulnerablity is VT-d specific.
> 
> As per the reply by Andrew to v1, this vulnerability is generic to intremap-
> incapable or intremap-disabled configurations. You want to say so. In turn
> I wonder whether instead of the changes you're making you wouldn't want to
> move the definition of the variable to xen/drivers/passthrough/x86/iommu.c.
> A useful further step might be to guard its definition (not necessarily
> its declaration; see replies to earlier patches) by CONFIG_PV instead (of
> course I understand that's largely orthogonal to your series here, yet it
> would fit easily with moving the definition).

Sure I can do that.

> 
>> --- a/xen/arch/x86/include/asm/iommu.h
>> +++ b/xen/arch/x86/include/asm/iommu.h
>> @@ -127,7 +127,9 @@ int iommu_identity_mapping(struct domain *d, p2m_access_t p2ma,
>>                              unsigned int flag);
>>   void iommu_identity_map_teardown(struct domain *d);
>>   
>> +#ifdef CONFIG_INTEL_IOMMU
>>   extern bool untrusted_msi;
>> +#endif
> 
> As per above / earlier comments I don't think this part is needed in any
> event.
> 
>> --- a/xen/arch/x86/pv/hypercall.c
>> +++ b/xen/arch/x86/pv/hypercall.c
>> @@ -193,8 +193,10 @@ void pv_ring1_init_hypercall_page(void *p)
>>   
>>   void do_entry_int82(struct cpu_user_regs *regs)
>>   {
>> +#ifdef CONFIG_INTEL_IOMMU
>>       if ( unlikely(untrusted_msi) )
>>           check_for_unexpected_msi((uint8_t)regs->entry_vector);
>> +#endif
>>   
>>       _pv_hypercall(regs, true /* compat */);
>>   }
>> diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
>> index ae01285181..8f2fb36770 100644
>> --- a/xen/arch/x86/x86_64/entry.S
>> +++ b/xen/arch/x86/x86_64/entry.S
>> @@ -406,11 +406,13 @@ ENTRY(int80_direct_trap)
>>   .Lint80_cr3_okay:
>>           sti
>>   
>> +#ifdef CONFIG_INTEL_IOMMU
>>           cmpb  $0,untrusted_msi(%rip)
>>   UNLIKELY_START(ne, msi_check)
>>           movl  $0x80,%edi
>>           call  check_for_unexpected_msi
>>   UNLIKELY_END(msi_check)
>> +#endif
>>   
>>           movq  STACK_CPUINFO_FIELD(current_vcpu)(%rbx), %rbx
>>   
> 

-- 
Xenia