Re: [PATCH 1/2] xen/arm: smmuv3: fix UB during deassign

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi,

> On 24 Jul 2025, at 02:59, dmkhn@xxxxxxxxx wrote:
> 
> On Wed, Jul 23, 2025 at 06:54:19PM -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@xxxxxxx>
>> ---
>> 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);
> 
> I think positive case first will be more readable.
> E.g.:
> 
> if ( dom )
>    return container_of(dom, struct arm_smmu_domain, domain);
> 
> return NULL;
> 

Exiting early on error case instead of doing the handling inside a if sounds 
cleaner
to me.

If more needs to be done inside the function then exiting early will make things
easier. It does not really matter now but might in the future hence why i acked
the original version instead of your suggestion.

Regards
Bertrand

>> }
>> 
>> 
>> base-commit: 5c798ac8854af3528a78ca5a622c9ea68399809b
>> --
>> 2.50.1