diff mbox series

[v4,2/5] x86/iommu: make code addressing CVE-2011-1898 no VT-d specific

Message ID 20230124124142.38500-3-burzalodowa@gmail.com (mailing list archive)
State New, archived
Headers show
Series Make x86 IOMMU driver support configurable | expand

Commit Message

Xenia Ragiadakou Jan. 24, 2023, 12:41 p.m. UTC 
The variable untrusted_msi indicates whether the system is vulnerable to
CVE-2011-1898 due to the absence of interrupt remapping support.
Although AMD iommus with interrupt remapping disabled are also affected,
this case is not handled yet. Given that the issue is not VT-d specific,
and to accommodate future use of the flag for covering also the AMD iommu
case, move the definition of the flag out of the VT-d specific code to the
common x86 iommu code.

Also, since the current implementation assumes that only PV guests are prone
to this attack, take the opportunity to define untrusted_msi only when PV is
enabled.

No functional change intended.

Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---

Changes in v4:
  - in vtd code, guard with CONFIG_PV the use of untrusted_msi
  - mention in commit log that CVE-2011-1898 currently is not addressed for
    AMD iommus with disabled intremap
  - add Jan's Reviewed-by tag

 xen/drivers/passthrough/vtd/iommu.c | 5 ++---
 xen/drivers/passthrough/x86/iommu.c | 5 +++++
 2 files changed, 7 insertions(+), 3 deletions(-)

Comments

Tian, Kevin Feb. 1, 2023, 5:07 a.m. UTC | #1 
> From: Xenia Ragiadakou <burzalodowa@gmail.com>
> Sent: Tuesday, January 24, 2023 8:42 PM
> 
> The variable untrusted_msi indicates whether the system is vulnerable to
> CVE-2011-1898 due to the absence of interrupt remapping support.
> Although AMD iommus with interrupt remapping disabled are also affected,
> this case is not handled yet. Given that the issue is not VT-d specific,
> and to accommodate future use of the flag for covering also the AMD iommu
> case, move the definition of the flag out of the VT-d specific code to the
> common x86 iommu code.
> 
> Also, since the current implementation assumes that only PV guests are
> prone
> to this attack, take the opportunity to define untrusted_msi only when PV is
> enabled.
> 

I'm fine with this change given no functional change.

But I'm curious about the statement here that the current code only
applies to PV guest. I didn't see such statement in original mail [1]
and in concept a HVM guest with passthrough device can also do such
attack w/o interrupt remapping.

Any more context?

[1] http://old-list-archives.xenproject.org/archives/html/xen-devel/2011-05/msg00687.html
Xenia Ragiadakou Feb. 1, 2023, 7:31 a.m. UTC | #2 
On 2/1/23 07:07, Tian, Kevin wrote:
>> From: Xenia Ragiadakou <burzalodowa@gmail.com>
>> Sent: Tuesday, January 24, 2023 8:42 PM
>>
>> The variable untrusted_msi indicates whether the system is vulnerable to
>> CVE-2011-1898 due to the absence of interrupt remapping support.
>> Although AMD iommus with interrupt remapping disabled are also affected,
>> this case is not handled yet. Given that the issue is not VT-d specific,
>> and to accommodate future use of the flag for covering also the AMD iommu
>> case, move the definition of the flag out of the VT-d specific code to the
>> common x86 iommu code.
>>
>> Also, since the current implementation assumes that only PV guests are
>> prone
>> to this attack, take the opportunity to define untrusted_msi only when PV is
>> enabled.
>>
> 
> I'm fine with this change given no functional change.
> 
> But I'm curious about the statement here that the current code only
> applies to PV guest. I didn't see such statement in original mail [1]
> and in concept a HVM guest with passthrough device can also do such
> attack w/o interrupt remapping.
> 
> Any more context?

I agree. I phrased it that way because currently the mitigation 
addresses only maliciously injected PV traps.

> 
> [1] http://old-list-archives.xenproject.org/archives/html/xen-devel/2011-05/msg00687.html
Jan Beulich Feb. 1, 2023, 9:30 a.m. UTC | #3 
On 01.02.2023 06:07, Tian, Kevin wrote:
>> From: Xenia Ragiadakou <burzalodowa@gmail.com>
>> Sent: Tuesday, January 24, 2023 8:42 PM
>>
>> The variable untrusted_msi indicates whether the system is vulnerable to
>> CVE-2011-1898 due to the absence of interrupt remapping support.
>> Although AMD iommus with interrupt remapping disabled are also affected,
>> this case is not handled yet. Given that the issue is not VT-d specific,
>> and to accommodate future use of the flag for covering also the AMD iommu
>> case, move the definition of the flag out of the VT-d specific code to the
>> common x86 iommu code.
>>
>> Also, since the current implementation assumes that only PV guests are
>> prone
>> to this attack, take the opportunity to define untrusted_msi only when PV is
>> enabled.
>>
> 
> I'm fine with this change given no functional change.
> 
> But I'm curious about the statement here that the current code only
> applies to PV guest. I didn't see such statement in original mail [1]
> and in concept a HVM guest with passthrough device can also do such
> attack w/o interrupt remapping.
> 
> Any more context?

Isn't this simply because we don't allow HVM to have devices assigned
without intremap? (I'm not sure, but even for PV allowing this may
have been limited to the xend tool stack.)

Jan
Tian, Kevin Feb. 2, 2023, 3:47 a.m. UTC | #4 
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Wednesday, February 1, 2023 5:30 PM
> 
> On 01.02.2023 06:07, Tian, Kevin wrote:
> >> From: Xenia Ragiadakou <burzalodowa@gmail.com>
> >> Sent: Tuesday, January 24, 2023 8:42 PM
> >>
> >> The variable untrusted_msi indicates whether the system is vulnerable to
> >> CVE-2011-1898 due to the absence of interrupt remapping support.
> >> Although AMD iommus with interrupt remapping disabled are also
> affected,
> >> this case is not handled yet. Given that the issue is not VT-d specific,
> >> and to accommodate future use of the flag for covering also the AMD
> iommu
> >> case, move the definition of the flag out of the VT-d specific code to the
> >> common x86 iommu code.
> >>
> >> Also, since the current implementation assumes that only PV guests are
> >> prone
> >> to this attack, take the opportunity to define untrusted_msi only when PV
> is
> >> enabled.
> >>
> >
> > I'm fine with this change given no functional change.
> >
> > But I'm curious about the statement here that the current code only
> > applies to PV guest. I didn't see such statement in original mail [1]
> > and in concept a HVM guest with passthrough device can also do such
> > attack w/o interrupt remapping.
> >
> > Any more context?
> 
> Isn't this simply because we don't allow HVM to have devices assigned
> without intremap? (I'm not sure, but even for PV allowing this may
> have been limited to the xend tool stack.)
> 

OK, this is what I'm seeking.

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
diff mbox series

Patch

diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index 62e143125d..e97b1fe8cd 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -54,9 +54,6 @@ 
                                  ? dom_iommu(d)->arch.vtd.pgd_maddr \
                                  : (pdev)->arch.vtd.pgd_maddr)
 
-/* Possible unfiltered LAPIC/MSI messages from untrusted sources? */
-bool __read_mostly untrusted_msi;
-
 bool __read_mostly iommu_igfx = true;
 bool __read_mostly iommu_qinval = true;
 #ifndef iommu_snoop
@@ -2770,6 +2767,7 @@  static int cf_check reassign_device_ownership(
         if ( !has_arch_pdevs(target) )
             vmx_pi_hooks_assign(target);
 
+#ifdef CONFIG_PV
         /*
          * Devices assigned to untrusted domains (here assumed to be any domU)
          * can attempt to send arbitrary LAPIC/MSI messages. We are unprotected
@@ -2778,6 +2776,7 @@  static int cf_check reassign_device_ownership(
         if ( !iommu_intremap && !is_hardware_domain(target) &&
              !is_system_domain(target) )
             untrusted_msi = true;
+#endif
 
         ret = domain_context_mapping(target, devfn, pdev);
 
diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
index f671b0f2bb..c5021ea023 100644
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -36,6 +36,11 @@  bool __initdata iommu_superpages = true;
 
 enum iommu_intremap __read_mostly iommu_intremap = iommu_intremap_full;
 
+#ifdef CONFIG_PV
+/* Possible unfiltered LAPIC/MSI messages from untrusted sources? */
+bool __read_mostly untrusted_msi;
+#endif
+
 #ifndef iommu_intpost
 /*
  * In the current implementation of VT-d posted interrupts, in some extreme