Re: [PATCH v2 04/11] iommu/arm-smmu-v3: Drain in-flight fault handlers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The EVTQ drain logic introduced to fix a Use-After-Free vulnerability is flawed: it fails to fully prevent the UAF and introduces a new deadlock vector.
--

commit 0c247207412e55b576df6d5bf6788056a0ed654c
Author: Nicolin Chen <nicolinc@nvidia.com>

iommu/arm-smmu-v3: Drain in-flight fault handlers

This commit attempts to fix a use-after-free vulnerability by draining
the SMMU hardware event queue and the IOMMU core IOPF software
workqueue during a domain detach or replace operation. It introduces
logic to poll the event queue and synchronize the IRQ handlers before
flushing the IOPF queue.

> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -3267,10 +3267,29 @@ void arm_smmu_attach_release(struct arm_smmu_attach_state *state)
>  	struct arm_smmu_master_domain *master_domain = state->old_master_domain;
>  	struct arm_smmu_master *master = state->master;
>  
> +	lockdep_assert_not_held(&arm_smmu_asid_lock);
>  	iommu_group_mutex_assert(master->dev);
>  
>  	if (!master_domain)
>  		return;
> +
> +	if (master_domain->using_iopf) {
> +		struct arm_smmu_device *smmu = master->smmu;
> +
> +		/* Drain the hardware eventq */
> +		if (master->stall_enabled) {
> +			arm_smmu_drain_queue_for_iopf(smmu, &smmu->evtq.q);
> +			/* Ensure pending events have reached the IOPF queue */
> +			if (smmu->evtq.q.irq)
> +				synchronize_irq(smmu->evtq.q.irq);

[Severity: High]
Since arm_smmu_attach_dev() installs the new Stream Table Entry (STE)
before calling arm_smmu_attach_release(), the SMMU can start generating
new faults under the new domain's configuration while this code executes.

Because the attach_dev driver callback hasn't returned yet, the IOMMU
core hasn't updated the device's attach_handle.

Could the concurrent event queue threaded handler read the old
attach_handle and route these new faults to the old domain's handler?

Since arm_smmu_drain_queue_for_iopf() only drains up to a snapshot of
the queue producer index, wouldn't these newly generated faults remain
queued against the old handle, leading to the same use-after-free this
attempts to fix when the core frees the old handle?

Additionally, is it safe to call synchronize_irq() here while holding
the iommu_group_mutex?

Since the event queue is shared across all devices on the SMMU, and the
arm_smmu_evtq_thread() handler processes faults in an unbounded loop,
could a continuous stream of faults from another device prevent the
handler from exiting? This might cause synchronize_irq() to block
indefinitely and deadlock the attach operation.

> +		}
> +		/* Pending events might be in the combined_irq handler */
[ ... ]

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