* [PATCH v2 1/2] xen/arm: smmuv3: fix UB during deassign
@ 2025-07-25 17:45 Stewart Hildebrand
2025-07-25 17:45 ` [PATCH v2 2/2] xen/arm: smmuv3: fix xl pci-assignable-remove Stewart Hildebrand
2025-07-25 20:17 ` [PATCH v2 1/2] xen/arm: smmuv3: fix UB during deassign dmkhn
0 siblings, 2 replies; 4+ messages in thread
From: Stewart Hildebrand @ 2025-07-25 17:45 UTC (permalink / raw)
To: xen-devel
Cc: Stewart Hildebrand, Bertrand Marquis, Rahul Singh,
Stefano Stabellini, Julien Grall, Michal Orzel, Volodymyr Babchuk
In arm_smmu_deassign_dev(), the return value from to_smmu_domain() is
NULL-checked. However, the implementation of to_smmu_domain() is a
container_of lookup, so the return value is unlikely to ever be NULL. In
case of a NULL argument to to_smmu_domain(), we will attempt to
dereference the non-NULL return value and encounter undefined behavior
and a crash:
$ xl pci-assignable-remove 00:01.0
(XEN) ================================================================================
(XEN) UBSAN: Undefined behaviour in drivers/passthrough/arm/smmu-v3.c:221:9
(XEN) applying non-zero offset ffffffffffffffc0 to null pointer
(XEN) Xen WARN at common/ubsan/ubsan.c:174
(XEN) ----[ Xen-4.21-unstable arm64 debug=y ubsan=y Tainted: C ]----
...
(XEN) Xen call trace:
(XEN) [<00000a0000350b2c>] ubsan.c#ubsan_epilogue+0x14/0xf0 (PC)
(XEN) [<00000a00003523e0>] __ubsan_handle_pointer_overflow+0x94/0x13c (LR)
(XEN) [<00000a00003523e0>] __ubsan_handle_pointer_overflow+0x94/0x13c
(XEN) [<00000a0000392f9c>] smmu-v3.c#to_smmu_domain+0x3c/0x40
(XEN) [<00000a000039e428>] smmu-v3.c#arm_smmu_deassign_dev+0x54/0x43c
(XEN) [<00000a00003a0300>] smmu-v3.c#arm_smmu_reassign_dev+0x74/0xc8
(XEN) [<00000a00003a7040>] pci.c#deassign_device+0x5fc/0xe0c
(XEN) [<00000a00003ade7c>] iommu_do_pci_domctl+0x7b4/0x90c
(XEN) [<00000a00003a34c0>] iommu_do_domctl+0x58/0xf4
(XEN) [<00000a00002ca66c>] do_domctl+0x2690/0x2a04
(XEN) [<00000a0000454d88>] traps.c#do_trap_hypercall+0xcf4/0x15b0
(XEN) [<00000a0000458588>] do_trap_guest_sync+0xa88/0xdd8
(XEN) [<00000a00003f8480>] entry.o#guest_sync_slowpath+0xa8/0xd8
(XEN)
(XEN) ================================================================================
(XEN) Data Abort Trap. Syndrome=0x4
(XEN) Walking Hypervisor VA 0xfffffffffffffff8 on CPU1 via TTBR 0x00000000406d0000
(XEN) 0TH[0x1ff] = 0x0
(XEN) CPU1: Unexpected Trap: Data Abort
(XEN) ----[ Xen-4.21-unstable arm64 debug=y ubsan=y Tainted: C ]----
...
(XEN) Xen call trace:
(XEN) [<00000a000039e494>] smmu-v3.c#arm_smmu_deassign_dev+0xc0/0x43c (PC)
(XEN) [<00000a000039e428>] smmu-v3.c#arm_smmu_deassign_dev+0x54/0x43c (LR)
(XEN) [<00000a00003a0300>] smmu-v3.c#arm_smmu_reassign_dev+0x74/0xc8
(XEN) [<00000a00003a7040>] pci.c#deassign_device+0x5fc/0xe0c
(XEN) [<00000a00003ade7c>] iommu_do_pci_domctl+0x7b4/0x90c
(XEN) [<00000a00003a34c0>] iommu_do_domctl+0x58/0xf4
(XEN) [<00000a00002ca66c>] do_domctl+0x2690/0x2a04
(XEN) [<00000a0000454d88>] traps.c#do_trap_hypercall+0xcf4/0x15b0
(XEN) [<00000a0000458588>] do_trap_guest_sync+0xa88/0xdd8
(XEN) [<00000a00003f8480>] entry.o#guest_sync_slowpath+0xa8/0xd8
Fix by changing to_smmu_domain() to return NULL in case of a NULL
argument.
Fixes: 452ddbe3592b ("xen/arm: smmuv3: Add support for SMMUv3 driver")
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
Acked-by: Bertrand Marquis <bertrand.marquis@arm.com>
---
v1->v2:
* add Bertrand's A-b
I'm unsure if that's the right Fixes: tag since I'm not sure if it can
be triggered prior to 63919fc4d1ca ("xen/arm: smmuv3: Add PCI devices
support for SMMUv3").
---
xen/drivers/passthrough/arm/smmu-v3.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/xen/drivers/passthrough/arm/smmu-v3.c b/xen/drivers/passthrough/arm/smmu-v3.c
index 58f3331520df..db08d3c04269 100644
--- a/xen/drivers/passthrough/arm/smmu-v3.c
+++ b/xen/drivers/passthrough/arm/smmu-v3.c
@@ -218,6 +218,9 @@ static struct arm_smmu_option_prop arm_smmu_options[] = {
static struct arm_smmu_domain *to_smmu_domain(struct iommu_domain *dom)
{
+ if ( !dom )
+ return NULL;
+
return container_of(dom, struct arm_smmu_domain, domain);
}
base-commit: 6cd9b9226c65c962b0f0e040e7d3d5c4053f8e06
--
2.50.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH v2 2/2] xen/arm: smmuv3: fix xl pci-assignable-remove
2025-07-25 17:45 [PATCH v2 1/2] xen/arm: smmuv3: fix UB during deassign Stewart Hildebrand
@ 2025-07-25 17:45 ` Stewart Hildebrand
2025-07-25 20:17 ` dmkhn
2025-07-25 20:17 ` [PATCH v2 1/2] xen/arm: smmuv3: fix UB during deassign dmkhn
1 sibling, 1 reply; 4+ messages in thread
From: Stewart Hildebrand @ 2025-07-25 17:45 UTC (permalink / raw)
To: xen-devel
Cc: Stewart Hildebrand, Bertrand Marquis, Rahul Singh,
Stefano Stabellini, Julien Grall, Michal Orzel, Volodymyr Babchuk
When attempting to xl pci-assignable-remove a PCI device, we encounter:
$ xl pci-assignable-remove 00:01.0
(XEN) SMMUv3: <no-node>: not attached to domain 32753
(XEN) d[IO]: deassign (0000:00:01.0) failed (-3)
libxl: error: libxl_pci.c:910:libxl__device_pci_assignable_remove: failed to de-quarantine 0000:00:01.0
When a PCI device is being deassigned from domIO,
arm_smmu_deassign_dev() should return before checking the smmu domain.
Fixes: 63919fc4d1ca ("xen/arm: smmuv3: Add PCI devices support for SMMUv3")
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
Acked-by: Bertrand Marquis <bertrand.marquis@arm.com>
---
v1->v2:
* use %pd in print format
* add Bertrand's A-b
---
xen/drivers/passthrough/arm/smmu-v3.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/xen/drivers/passthrough/arm/smmu-v3.c b/xen/drivers/passthrough/arm/smmu-v3.c
index db08d3c04269..5e9e3e048e34 100644
--- a/xen/drivers/passthrough/arm/smmu-v3.c
+++ b/xen/drivers/passthrough/arm/smmu-v3.c
@@ -2747,11 +2747,6 @@ static int arm_smmu_deassign_dev(struct domain *d, uint8_t devfn, struct device
struct arm_smmu_domain *smmu_domain = to_smmu_domain(io_domain);
struct arm_smmu_master *master = dev_iommu_priv_get(dev);
- if (!smmu_domain || smmu_domain->d != d) {
- dev_err(dev, " not attached to domain %d\n", d->domain_id);
- return -ESRCH;
- }
-
#ifdef CONFIG_HAS_PCI
if ( dev_is_pci(dev) )
{
@@ -2767,6 +2762,11 @@ static int arm_smmu_deassign_dev(struct domain *d, uint8_t devfn, struct device
}
#endif
+ if (!smmu_domain || smmu_domain->d != d) {
+ dev_err(dev, " not attached to %pd\n", d);
+ return -ESRCH;
+ }
+
spin_lock(&xen_domain->lock);
arm_smmu_detach_dev(master);
--
2.50.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2 1/2] xen/arm: smmuv3: fix UB during deassign
2025-07-25 17:45 [PATCH v2 1/2] xen/arm: smmuv3: fix UB during deassign Stewart Hildebrand
2025-07-25 17:45 ` [PATCH v2 2/2] xen/arm: smmuv3: fix xl pci-assignable-remove Stewart Hildebrand
@ 2025-07-25 20:17 ` dmkhn
1 sibling, 0 replies; 4+ messages in thread
From: dmkhn @ 2025-07-25 20:17 UTC (permalink / raw)
To: Stewart Hildebrand
Cc: xen-devel, Bertrand Marquis, Rahul Singh, Stefano Stabellini,
Julien Grall, Michal Orzel, Volodymyr Babchuk
On Fri, Jul 25, 2025 at 01:45:50PM -0400, Stewart Hildebrand wrote:
> In arm_smmu_deassign_dev(), the return value from to_smmu_domain() is
> NULL-checked. However, the implementation of to_smmu_domain() is a
> container_of lookup, so the return value is unlikely to ever be NULL. In
> case of a NULL argument to to_smmu_domain(), we will attempt to
> dereference the non-NULL return value and encounter undefined behavior
> and a crash:
>
> $ xl pci-assignable-remove 00:01.0
> (XEN) ================================================================================
> (XEN) UBSAN: Undefined behaviour in drivers/passthrough/arm/smmu-v3.c:221:9
> (XEN) applying non-zero offset ffffffffffffffc0 to null pointer
> (XEN) Xen WARN at common/ubsan/ubsan.c:174
> (XEN) ----[ Xen-4.21-unstable arm64 debug=y ubsan=y Tainted: C ]----
> ...
> (XEN) Xen call trace:
> (XEN) [<00000a0000350b2c>] ubsan.c#ubsan_epilogue+0x14/0xf0 (PC)
> (XEN) [<00000a00003523e0>] __ubsan_handle_pointer_overflow+0x94/0x13c (LR)
> (XEN) [<00000a00003523e0>] __ubsan_handle_pointer_overflow+0x94/0x13c
> (XEN) [<00000a0000392f9c>] smmu-v3.c#to_smmu_domain+0x3c/0x40
> (XEN) [<00000a000039e428>] smmu-v3.c#arm_smmu_deassign_dev+0x54/0x43c
> (XEN) [<00000a00003a0300>] smmu-v3.c#arm_smmu_reassign_dev+0x74/0xc8
> (XEN) [<00000a00003a7040>] pci.c#deassign_device+0x5fc/0xe0c
> (XEN) [<00000a00003ade7c>] iommu_do_pci_domctl+0x7b4/0x90c
> (XEN) [<00000a00003a34c0>] iommu_do_domctl+0x58/0xf4
> (XEN) [<00000a00002ca66c>] do_domctl+0x2690/0x2a04
> (XEN) [<00000a0000454d88>] traps.c#do_trap_hypercall+0xcf4/0x15b0
> (XEN) [<00000a0000458588>] do_trap_guest_sync+0xa88/0xdd8
> (XEN) [<00000a00003f8480>] entry.o#guest_sync_slowpath+0xa8/0xd8
> (XEN)
> (XEN) ================================================================================
> (XEN) Data Abort Trap. Syndrome=0x4
> (XEN) Walking Hypervisor VA 0xfffffffffffffff8 on CPU1 via TTBR 0x00000000406d0000
> (XEN) 0TH[0x1ff] = 0x0
> (XEN) CPU1: Unexpected Trap: Data Abort
> (XEN) ----[ Xen-4.21-unstable arm64 debug=y ubsan=y Tainted: C ]----
> ...
> (XEN) Xen call trace:
> (XEN) [<00000a000039e494>] smmu-v3.c#arm_smmu_deassign_dev+0xc0/0x43c (PC)
> (XEN) [<00000a000039e428>] smmu-v3.c#arm_smmu_deassign_dev+0x54/0x43c (LR)
> (XEN) [<00000a00003a0300>] smmu-v3.c#arm_smmu_reassign_dev+0x74/0xc8
> (XEN) [<00000a00003a7040>] pci.c#deassign_device+0x5fc/0xe0c
> (XEN) [<00000a00003ade7c>] iommu_do_pci_domctl+0x7b4/0x90c
> (XEN) [<00000a00003a34c0>] iommu_do_domctl+0x58/0xf4
> (XEN) [<00000a00002ca66c>] do_domctl+0x2690/0x2a04
> (XEN) [<00000a0000454d88>] traps.c#do_trap_hypercall+0xcf4/0x15b0
> (XEN) [<00000a0000458588>] do_trap_guest_sync+0xa88/0xdd8
> (XEN) [<00000a00003f8480>] entry.o#guest_sync_slowpath+0xa8/0xd8
>
> Fix by changing to_smmu_domain() to return NULL in case of a NULL
> argument.
>
> Fixes: 452ddbe3592b ("xen/arm: smmuv3: Add support for SMMUv3 driver")
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
> Acked-by: Bertrand Marquis <bertrand.marquis@arm.com>
Reviewed-by: Denis Mukhin <dmukhin@ford.com>
> ---
> v1->v2:
> * add Bertrand's A-b
>
> I'm unsure if that's the right Fixes: tag since I'm not sure if it can
> be triggered prior to 63919fc4d1ca ("xen/arm: smmuv3: Add PCI devices
> support for SMMUv3").
> ---
> xen/drivers/passthrough/arm/smmu-v3.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/xen/drivers/passthrough/arm/smmu-v3.c b/xen/drivers/passthrough/arm/smmu-v3.c
> index 58f3331520df..db08d3c04269 100644
> --- a/xen/drivers/passthrough/arm/smmu-v3.c
> +++ b/xen/drivers/passthrough/arm/smmu-v3.c
> @@ -218,6 +218,9 @@ static struct arm_smmu_option_prop arm_smmu_options[] = {
>
> static struct arm_smmu_domain *to_smmu_domain(struct iommu_domain *dom)
> {
> + if ( !dom )
> + return NULL;
> +
> return container_of(dom, struct arm_smmu_domain, domain);
> }
>
>
> base-commit: 6cd9b9226c65c962b0f0e040e7d3d5c4053f8e06
> --
> 2.50.1
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2 2/2] xen/arm: smmuv3: fix xl pci-assignable-remove
2025-07-25 17:45 ` [PATCH v2 2/2] xen/arm: smmuv3: fix xl pci-assignable-remove Stewart Hildebrand
@ 2025-07-25 20:17 ` dmkhn
0 siblings, 0 replies; 4+ messages in thread
From: dmkhn @ 2025-07-25 20:17 UTC (permalink / raw)
To: Stewart Hildebrand
Cc: xen-devel, Bertrand Marquis, Rahul Singh, Stefano Stabellini,
Julien Grall, Michal Orzel, Volodymyr Babchuk
On Fri, Jul 25, 2025 at 01:45:51PM -0400, Stewart Hildebrand wrote:
> When attempting to xl pci-assignable-remove a PCI device, we encounter:
>
> $ xl pci-assignable-remove 00:01.0
> (XEN) SMMUv3: <no-node>: not attached to domain 32753
> (XEN) d[IO]: deassign (0000:00:01.0) failed (-3)
> libxl: error: libxl_pci.c:910:libxl__device_pci_assignable_remove: failed to de-quarantine 0000:00:01.0
>
> When a PCI device is being deassigned from domIO,
> arm_smmu_deassign_dev() should return before checking the smmu domain.
>
> Fixes: 63919fc4d1ca ("xen/arm: smmuv3: Add PCI devices support for SMMUv3")
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
> Acked-by: Bertrand Marquis <bertrand.marquis@arm.com>
Reviewed-by: Denis Mukhin <dmukhin@ford.com>
> ---
> v1->v2:
> * use %pd in print format
> * add Bertrand's A-b
> ---
> xen/drivers/passthrough/arm/smmu-v3.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/xen/drivers/passthrough/arm/smmu-v3.c b/xen/drivers/passthrough/arm/smmu-v3.c
> index db08d3c04269..5e9e3e048e34 100644
> --- a/xen/drivers/passthrough/arm/smmu-v3.c
> +++ b/xen/drivers/passthrough/arm/smmu-v3.c
> @@ -2747,11 +2747,6 @@ static int arm_smmu_deassign_dev(struct domain *d, uint8_t devfn, struct device
> struct arm_smmu_domain *smmu_domain = to_smmu_domain(io_domain);
> struct arm_smmu_master *master = dev_iommu_priv_get(dev);
>
> - if (!smmu_domain || smmu_domain->d != d) {
> - dev_err(dev, " not attached to domain %d\n", d->domain_id);
> - return -ESRCH;
> - }
> -
> #ifdef CONFIG_HAS_PCI
> if ( dev_is_pci(dev) )
> {
> @@ -2767,6 +2762,11 @@ static int arm_smmu_deassign_dev(struct domain *d, uint8_t devfn, struct device
> }
> #endif
>
> + if (!smmu_domain || smmu_domain->d != d) {
> + dev_err(dev, " not attached to %pd\n", d);
> + return -ESRCH;
> + }
> +
> spin_lock(&xen_domain->lock);
>
> arm_smmu_detach_dev(master);
> --
> 2.50.1
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-07-25 20:18 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-07-25 17:45 [PATCH v2 1/2] xen/arm: smmuv3: fix UB during deassign Stewart Hildebrand
2025-07-25 17:45 ` [PATCH v2 2/2] xen/arm: smmuv3: fix xl pci-assignable-remove Stewart Hildebrand
2025-07-25 20:17 ` dmkhn
2025-07-25 20:17 ` [PATCH v2 1/2] xen/arm: smmuv3: fix UB during deassign dmkhn
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.