The current usage of msi_desc->msg in vmx_pi_update_irte() will make the
field contain a translated MSI message, instead of the expected
untranslated one.  This breaks dump_msi(), that use the data in
msi_desc->msg to print the interrupt details.

Fix this by introducing a dummy local msi_msg, and use it with
iommu_update_ire_from_msi().  vmx_pi_update_irte() relies on the MSI
message not changing, so there's no need to propagate the resulting msi_msg
to the hardware, and the contents can be ignored.

Fixes: a5e25908d18d ('VT-d: introduce new fields in msi_desc to track binding with guest interrupt')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v3:
 - New in this version.
---
 xen/arch/x86/hvm/vmx/vmx.c     | 9 ++++++++-
 xen/arch/x86/include/asm/msi.h | 2 +-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 0241303b4bf4..c407513af624 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -396,6 +396,13 @@ static int cf_check vmx_pi_update_irte(const struct vcpu *v,
     const struct pi_desc *pi_desc = v ? &v->arch.hvm.vmx.pi_desc : NULL;
     struct irq_desc *desc;
     struct msi_desc *msi_desc;
+    /*
+     * vmx_pi_update_irte() relies on the IRTE already being setup, and just
+     * updates the guest vector, but not the other IRTE fields.  As such the
+     * contents of msg are not consumed by iommu_update_ire_from_msi().  Even
+     * if not consumed, zero the contents to avoid possible stack leaks.
+     */
+    struct msi_msg msg = {};
     int rc;
 
     desc = pirq_spin_lock_irq_desc(pirq, NULL);
@@ -415,7 +422,7 @@ static int cf_check vmx_pi_update_irte(const struct vcpu *v,
 
     ASSERT_PDEV_LIST_IS_READ_LOCKED(msi_desc->dev->domain);
 
-    return iommu_update_ire_from_msi(msi_desc, &msi_desc->msg);
+    return iommu_update_ire_from_msi(msi_desc, &msg);
 
  unlock_out:
     spin_unlock_irq(&desc->lock);
diff --git a/xen/arch/x86/include/asm/msi.h b/xen/arch/x86/include/asm/msi.h
index 378b85ee947b..975d0f26b35d 100644
--- a/xen/arch/x86/include/asm/msi.h
+++ b/xen/arch/x86/include/asm/msi.h
@@ -124,7 +124,7 @@ struct msi_desc {
     int irq;
     int remap_index;         /* index in interrupt remapping table */
 
-    struct msi_msg msg;      /* Last set MSI message */
+    struct msi_msg msg;      /* Last set MSI message (untranslated) */
 };
 
 /*
-- 
2.48.1

On 11.03.2025 13:06, Roger Pau Monne wrote:
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -396,6 +396,13 @@ static int cf_check vmx_pi_update_irte(const struct vcpu *v,
>      const struct pi_desc *pi_desc = v ? &v->arch.hvm.vmx.pi_desc : NULL;
>      struct irq_desc *desc;
>      struct msi_desc *msi_desc;
> +    /*
> +     * vmx_pi_update_irte() relies on the IRTE already being setup, and just
> +     * updates the guest vector, but not the other IRTE fields.  As such the
> +     * contents of msg are not consumed by iommu_update_ire_from_msi().  Even
> +     * if not consumed, zero the contents to avoid possible stack leaks.
> +     */
> +    struct msi_msg msg = {};

What the comment says is true only when pi_desc != NULL. As can be seen in
context above, it can very well be NULL here, though (which isn't to say
that I'm convinced the NULL case is handled correctly here). I'd view it as
more safe anyway if you set msg from msi_desc->msg.

Jan

On Tue, Mar 11, 2025 at 02:10:04PM +0100, Jan Beulich wrote:
> On 11.03.2025 13:06, Roger Pau Monne wrote:
> > --- a/xen/arch/x86/hvm/vmx/vmx.c
> > +++ b/xen/arch/x86/hvm/vmx/vmx.c
> > @@ -396,6 +396,13 @@ static int cf_check vmx_pi_update_irte(const struct vcpu *v,
> >      const struct pi_desc *pi_desc = v ? &v->arch.hvm.vmx.pi_desc : NULL;
> >      struct irq_desc *desc;
> >      struct msi_desc *msi_desc;
> > +    /*
> > +     * vmx_pi_update_irte() relies on the IRTE already being setup, and just
> > +     * updates the guest vector, but not the other IRTE fields.  As such the
> > +     * contents of msg are not consumed by iommu_update_ire_from_msi().  Even
> > +     * if not consumed, zero the contents to avoid possible stack leaks.
> > +     */
> > +    struct msi_msg msg = {};
> 
> What the comment says is true only when pi_desc != NULL. As can be seen in
> context above, it can very well be NULL here, though (which isn't to say
> that I'm convinced the NULL case is handled correctly here). I'd view it as
> more safe anyway if you set msg from msi_desc->msg.

Indeed that's likely better.  I'm also unsure the teardown is correct
(or needed), but I didn't want to deal with that right now.

Thanks, Roger.

The current usage of msi_desc->msg in vmx_pi_update_irte() will make the
field contain a translated MSI message, instead of the expected
untranslated one.  This breaks dump_msi(), that use the data in
msi_desc->msg to print the interrupt details.

Fix this by introducing a dummy local msi_msg, and use it with
iommu_update_ire_from_msi().  vmx_pi_update_irte() relies on the MSI
message not changing, so there's no need to propagate the resulting msi_msg
to the hardware, and the contents can be ignored.

Additionally add a comment to clarify that msi_desc->msg must always
contain the untranslated MSI message.

Fixes: a5e25908d18d ('VT-d: introduce new fields in msi_desc to track binding with guest interrupt')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v4:
 - Initialize the local msi_msg with the contents of msi_desc->msg.

Changes since v3:
 - New in this version.
---
 xen/arch/x86/hvm/vmx/vmx.c     | 4 +++-
 xen/arch/x86/include/asm/msi.h | 2 +-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 0241303b4bf4..23b7ecd77f85 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -396,6 +396,7 @@ static int cf_check vmx_pi_update_irte(const struct vcpu *v,
     const struct pi_desc *pi_desc = v ? &v->arch.hvm.vmx.pi_desc : NULL;
     struct irq_desc *desc;
     struct msi_desc *msi_desc;
+    struct msi_msg msg;
     int rc;
 
     desc = pirq_spin_lock_irq_desc(pirq, NULL);
@@ -410,12 +411,13 @@ static int cf_check vmx_pi_update_irte(const struct vcpu *v,
     }
     msi_desc->pi_desc = pi_desc;
     msi_desc->gvec = gvec;
+    msg = msi_desc->msg;
 
     spin_unlock_irq(&desc->lock);
 
     ASSERT_PDEV_LIST_IS_READ_LOCKED(msi_desc->dev->domain);
 
-    return iommu_update_ire_from_msi(msi_desc, &msi_desc->msg);
+    return iommu_update_ire_from_msi(msi_desc, &msg);
 
  unlock_out:
     spin_unlock_irq(&desc->lock);
diff --git a/xen/arch/x86/include/asm/msi.h b/xen/arch/x86/include/asm/msi.h
index 378b85ee947b..975d0f26b35d 100644
--- a/xen/arch/x86/include/asm/msi.h
+++ b/xen/arch/x86/include/asm/msi.h
@@ -124,7 +124,7 @@ struct msi_desc {
     int irq;
     int remap_index;         /* index in interrupt remapping table */
 
-    struct msi_msg msg;      /* Last set MSI message */
+    struct msi_msg msg;      /* Last set MSI message (untranslated) */
 };
 
 /*
-- 
2.48.1

On 11.03.2025 16:27, Roger Pau Monne wrote:
> The current usage of msi_desc->msg in vmx_pi_update_irte() will make the
> field contain a translated MSI message, instead of the expected
> untranslated one.  This breaks dump_msi(), that use the data in
> msi_desc->msg to print the interrupt details.
> 
> Fix this by introducing a dummy local msi_msg, and use it with
> iommu_update_ire_from_msi().  vmx_pi_update_irte() relies on the MSI
> message not changing, so there's no need to propagate the resulting msi_msg
> to the hardware, and the contents can be ignored.
> 
> Additionally add a comment to clarify that msi_desc->msg must always
> contain the untranslated MSI message.
> 
> Fixes: a5e25908d18d ('VT-d: introduce new fields in msi_desc to track binding with guest interrupt')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

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