Re: [PATCH v2 6/7] arm64: dts: allwinner: sun50i-h6: Add missing IOMMU for video engine

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The sun50i-iommu driver fails to properly detach domains from the hardware due to an unbalanced refcount, leading to a use-after-free of the domain structure.
- [High] The sun50i-iommu driver assigns a separate IOMMU group/domain per device despite the hardware only supporting a single global translation table, causing devices to overwrite each other's page tables.
--

--- Patch [6]: [PATCH v2 6/7] arm64: dts: allwinner: sun50i-h6: Add missing IOMMU for video engine ---
commit 8a79dfe07f469620b0b60592b02052d1c198fe97
Author: Chen-Yu Tsai <wens@kernel.org>

arm64: dts: allwinner: sun50i-h6: Add missing IOMMU for video engine
    
This commit adds a missing IOMMU port ("VE_R") to the video engine node
in the sun50i-h6 device tree.

> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
> index 88c6e3e105c07..48c771223af4d 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
> @@ -206,7 +206,7 @@ video-codec@1c0e000 {
>  			resets = <&ccu RST_BUS_VE>;
>  			interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>;
>  			allwinner,sram = <&ve_sram 1>, <&de2_sram 1>;
> -			iommus = <&iommu 3>;
> +			iommus = <&iommu 3>, <&iommu 1>;

[Severity: High]
This is a pre-existing issue, but does adding a second IOMMU trigger a
use-after-free during device unbind?

Looking at the sun50i-iommu driver, sun50i_iommu_domain_alloc_paging() sets
the domain refcount to 1:

drivers/iommu/sun50i-iommu.c:sun50i_iommu_domain_alloc_paging() {
    ...
	refcount_set(&sun50i_domain->refcnt, 1);
    ...
}

Later, when a device is detached, sun50i_iommu_identity_attach()
decrements it:

drivers/iommu/sun50i-iommu.c:sun50i_iommu_identity_attach() {
    ...
	sun50i_domain = to_sun50i_domain(iommu->domain);
	if (refcount_dec_and_test(&sun50i_domain->refcnt))
		sun50i_iommu_detach_domain(iommu, sun50i_domain);
    ...
}

Because the reference count drops from 2 to 1 here, it fails to reach 0 and
bypasses the domain detachment. This leaves iommu->domain as a dangling
pointer which is later freed by sun50i_iommu_domain_free(). Could this
cause a kernel panic on a subsequent IOMMU page fault interrupt since it
dereferences the freed pointer?

[Severity: High]
This isn't a bug introduced by this patch, but does adding another IOMMU
reference here break DMA due to a single global translation table?

The sun50i-iommu driver assigns a separate IOMMU group per device:

drivers/iommu/sun50i-iommu.c:
static const struct iommu_ops sun50i_iommu_ops = {
    ...
	.device_group	= generic_single_device_group,
    ...
};

Since the hardware only supports a single global translation table
(IOMMU_TTB_REG), placing every device into its own independent group and
domain means multiple devices will repeatedly overwrite the shared IOMMU's
domain on probe. Will enabling this second IOMMU port overwrite previously
configured translation tables and corrupt memory for other devices?

>  		};
>  
>  		gpu: gpu@1800000 {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260711135959.3616919-1-wens@kernel.org?part=6