Re: [PATCH v3 4/8] iommu/msm: Look up masters per IOMMU instance

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] msm_iommu_attach_dev and detach unconditionally loop over all shared masters, breaking IOMMU isolation, causing attach failures, and corrupting domain lists.
- [High] msm_iommu_iova_to_phys() performs address translation using the wrong hardware context bank for shared IOMMUs.
- [High] Error path in msm_iommu_attach_dev leaks IOMMU clocks.
- [High] Buffer overflow in insert_iommu_master when writing stream IDs to master->mids array.
- [High] msm_iommu_probe modifies a global list without acquiring the required lock, causing race conditions.
--

commit a857d6e3a3b0380237f9c6c7b9ba76932246dfa4
Author: Alexandre MINETTE <contact@alex-min.fr>

iommu/msm: Look up masters per IOMMU instance

This commit updates the MSM IOMMU driver to look up context descriptors on
each IOMMU instance instead of using a global device provider. It introduces
a helper to search the target IOMMU context list when probing and attaching
devices. This aims to prevent null pointer dereferences when a device
references multiple IOMMU provider nodes.

> diff --git a/drivers/iommu/msm_iommu.c b/drivers/iommu/msm_iommu.c
> index 0ad5ff431d5b6..9c03878d0d2cb 100644
> --- a/drivers/iommu/msm_iommu.c
> +++ b/drivers/iommu/msm_iommu.c
> @@ -357,17 +357,25 @@ static int msm_iommu_domain_config(struct msm_priv *priv)
>  	return 0;
>  }
>  
> +static struct msm_iommu_ctx_dev *find_iommu_master(struct msm_iommu_dev *iommu,
> +						   struct device *dev)
> +{
> +	struct msm_iommu_ctx_dev *master;
> +
> +	list_for_each_entry(master, &iommu->ctx_list, list)
> +		if (master->of_node == dev->of_node)
> +			return master;
> +
> +	return NULL;
> +}

Since this patch allows multiple masters to be added to an IOMMU's ctx_list,
are there other functions that still assume the list only contains a single
element?

For example, doesn't msm_iommu_iova_to_phys() hardcode
list_first_entry(&iommu->ctx_list, ...) to select the master and query its
specific hardware context bank?

If translation is requested for any device other than the first one attached
to the IOMMU, could the driver read the hardware registers for the wrong
context bank, returning incorrect physical addresses?

>  /* Must be called under msm_iommu_lock */
>  static struct msm_iommu_dev *find_iommu_for_dev(struct device *dev)
>  {
>  	struct msm_iommu_dev *iommu, *ret = NULL;
> -	struct msm_iommu_ctx_dev *master;
>  
>  	list_for_each_entry(iommu, &qcom_iommu_devices, dev_node) {

This problem wasn't introduced by this patch, but msm_iommu_probe() adds the
newly allocated device to the global qcom_iommu_devices list without holding
the msm_iommu_lock spinlock:

msm_iommu_probe() {
    ...
    list_add(&iommu->dev_node, &qcom_iommu_devices);
    ...
}

Since other functions (like find_iommu_for_dev) concurrently access this
list under the lock, could this unprotected list mutation lead to list
corruption or a crash in asynchronous probing scenarios?

> -		master = list_first_entry(&iommu->ctx_list,
> -					  struct msm_iommu_ctx_dev,
> -					  list);
> -		if (master->of_node == dev->of_node) {
> +		if (find_iommu_master(iommu, dev)) {
>  			ret = iommu;
>  			break;
>  		}

[ ... ]

> @@ -405,10 +413,7 @@ static int msm_iommu_attach_dev(struct iommu_domain *domain, struct device *dev,
>  
>  	spin_lock_irqsave(&msm_iommu_lock, flags);
>  	list_for_each_entry(iommu, &qcom_iommu_devices, dev_node) {
> -		master = list_first_entry(&iommu->ctx_list,
> -					  struct msm_iommu_ctx_dev,
> -					  list);
> -		if (master->of_node == dev->of_node) {
> +		if (find_iommu_master(iommu, dev)) {
>  			ret = __enable_clocks(iommu);
>  			if (ret)
>  				goto fail;

With this change, multiple masters can be present in the ctx_list, but
msm_iommu_attach_dev() and msm_iommu_identity_attach() still unconditionally
iterate over the entire ctx_list instead of filtering for the target master:

msm_iommu_attach_dev() {
    ...
    if (find_iommu_master(iommu, dev)) {
        ...
        list_for_each_entry(master, &iommu->ctx_list, list) {
            ...

When the first device attaches, does this incorrectly provision hardware
contexts for all shared devices using its own domain, breaking IOMMU
isolation?

Also, because msm_iommu_alloc_ctx() can return 0, the safety check
if (master->num) evaluates to false for context bank 0. When a subsequent
device attaches, the loop re-allocates contexts and executes
list_add(&iommu->dom_node, &priv->list_attached) a second time. Could this
result in corruption of the doubly-linked domain list, leading to an
infinite loop and hang?

If a non-zero context bank was allocated, subsequent attaches hit
if (master->num) and unconditionally fail with -EEXIST. Would this prevent
shared IOMMUs from functioning?

Furthermore, does detaching one device via msm_iommu_identity_attach()
unintentionally tear down active DMA mappings for all other shared devices?

If the loop encounters a master with a non-zero context bank
(if (master->num)), it sets ret = -EEXIST and goes to the fail label:

msm_iommu_attach_dev() {
    ...
    if (master->num) {
        dev_err(dev, "domain already attached");
        ret = -EEXIST;
        goto fail;
    }

Since the fail label unlocks the spinlock and returns without calling
__disable_clocks(iommu), and shared IOMMUs will hit this reliably, does
this cause a permanent clock resource leak during probe/attach?

[ ... ]

> @@ -601,10 +606,10 @@ static int insert_iommu_master(struct device *dev,
>  				struct msm_iommu_dev **iommu,
>  				const struct of_phandle_args *spec)
>  {
> -	struct msm_iommu_ctx_dev *master = dev_iommu_priv_get(dev);
> +	struct msm_iommu_ctx_dev *master = find_iommu_master(*iommu, dev);
>  	int sid;
>  
> -	if (list_empty(&(*iommu)->ctx_list)) {
> +	if (!master) {
>  		master = kzalloc_obj(*master, GFP_ATOMIC);
>  		if (!master) {
>  			dev_err(dev, "Failed to allocate iommu_master\n");
> @@ -612,7 +617,6 @@ static int insert_iommu_master(struct device *dev,
>  		}
>  		master->of_node = dev->of_node;
>  		list_add(&master->list, &(*iommu)->ctx_list);
> -		dev_iommu_priv_set(dev, master);
>  	}
>  
>  	for (sid = 0; sid < master->num_mids; sid++)

This wasn't introduced by this patch, but does this function lack bounds
checking when appending stream IDs to the master->mids array?

If a maliciously crafted or malformed Device Tree provides more than
MAX_NUM_MIDS (32) stream IDs for a single device, could this result in an
out-of-bounds write, corrupting adjacent heap memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260519-mainline-send-v1-sending-v3-0-3dd7aa125353@alex-min.fr?part=4