Re: [PATCH v7 3/5] iommu/arm-smmu-v3: Fix a UAF in the probe_device error path

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Calling `rb_erase()` on an unlinked, zero-initialized stream node silently corrupts the SMMU streams rbtree by zeroing the tree root.
--

commit 785579083a69b017d9dafe6dce3c844bea656662
Author: Pranjal Shrivastava <praan@google.com>

iommu/arm-smmu-v3: Fix a UAF in the probe_device error path

Clear the iommu->priv to NULL while returning an error from probe_device.

> 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 a10affb483a4f..54c7f1ae3a841 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -4206,6 +4206,7 @@ static struct iommu_device *arm_smmu_probe_device(struct device *dev)
>  	arm_smmu_disable_pasid(master);
>  	arm_smmu_remove_master(master);

[Severity: Critical]
This is a pre-existing issue, but could calling arm_smmu_remove_master()
here silently corrupt the smmu streams rbtree if there are duplicate
Stream IDs?

In arm_smmu_insert_master(), master->streams is allocated with kzalloc,
leaving the rb_node zero-initialized. If a device has duplicate Stream IDs
(expected for bridged PCI devices), it skips inserting the duplicate node
into smmu->streams:

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c:arm_smmu_insert_master() {
    ...
			/* Bridged PCI devices may end up with duplicated IDs */
			if (existing_master == master)
				continue;
    ...
}

However, arm_smmu_remove_master() unconditionally calls rb_erase() on all
nodes:

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c:arm_smmu_remove_master() {
    ...
	for (i = 0; i < fwspec->num_ids; i++)
		rb_erase(&master->streams[i].node, &smmu->streams);
    ...
}

Since the rb_erase() implementation interprets a zero-initialized node as a
root node with no children, it clears it by setting root->rb_node = NULL.
Does this detach the entire stream tracking tree?

The error path in arm_smmu_insert_master() also unconditionally calls
rb_erase() on processed nodes:

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c:arm_smmu_insert_master() {
    ...
		for (i--; i >= 0; i--)
			rb_erase(&master->streams[i].node, &smmu->streams);
    ...
}

Would this also trigger the same corruption if a failure happens after
skipping a duplicate ID?

>  err_free_master:
> +	dev_iommu_priv_set(dev, NULL);
>  	kfree(master);
>  	return ERR_PTR(ret);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260601143644.2358771-1-praan@google.com?part=3