Re: [PATCH v5 05/15] iommu/arm-smmu-v3: Flush in-flight fault work on domain detach

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] iommu/arm-smmu-v3: memory leak of master_domain on timeout path
--

commit 93bfbb1f6ed056db517f551ddf8e2b1b26915570
Author: Nicolin Chen <nicolinc@nvidia.com>

iommu/arm-smmu-v3: Flush in-flight fault work on domain detach

This commit flushes in-flight fault work on domain detach and skips IOPF
teardown if the hardware queue drain times out. This aims to prevent a
use-after-free by intentionally leaking the master_domain and iopf refcount
on timeout.

> If arm_smmu_wait_for_queue_drained() times out, fault work may still be in
> flight, and iopf_queue_remove_device() would free iopf groups that the work
> also references. Skip the iopf teardown and leak the master_domain, rather
> than risk a use-after-free.

[Severity: Medium]
The commit message states the intent to leak master_domain to avoid a
use-after-free. Is this description accurate?

The risk appears to apply exclusively to the iopf_group structures that
would be freed by the skipped iopf_queue_remove_device() call. The
master_domain structure is entirely driver-internal, has already been
unlinked from the smmu_domain->devices list in
arm_smmu_remove_master_domain(), and is not referenced by in-flight IOPF
works or any other background threads.

> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index 9e657d075f387..c8fb1c38090d1 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
[ ... ]
> @@ -3422,8 +3423,38 @@ void arm_smmu_attach_release(struct arm_smmu_attach_state *state)
[ ... ]
> +	/* Lastly, flush the fault work that the drained events queued */
> +	if (master_domain->using_iopf) {
> +		iopf_queue_flush_dev(master->dev);
> +
> +		/*
> +		 * A timed-out drain may leave fault work in flight, and
> +		 * iopf_queue_remove_device() would free iopf groups that
> +		 * such work still references. Skip the iopf teardown and
> +		 * leak master_domain, rather than risk a UAF.
> +		 */
> +		if (WARN_ON(timed_out)) {
> +			state->old_master_domain = NULL;
> +			return;
> +		}

[Severity: Medium]
Does this early return leak master_domain unnecessarily?

Since kfree(master_domain) appears safe to call here even if the drain
times out, could we free it before returning to avoid leaking master_domain?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1789446520.git.nicolinc@nvidia.com?part=5