Re: [PATCH RFC v3 1/3] iommu/arm-smmu-v3: Support shared SIDs in insert/remove_master

[Nicolin Chen]

On Wed, Sep 30, 2026 at 07:25:01PM +0800, Peng Fan (OSS) wrote:
> @@ -4115,7 +4128,10 @@ static int arm_smmu_insert_master(struct arm_smmu_device *smmu,
>  
>  		new_stream->id = fwspec->ids[i];
>  		new_stream->master = master;
> +		new_stream->ste_installed = false;

ste_installed is zero-ed.

> @@ -4136,23 +4152,29 @@ static int arm_smmu_insert_master(struct arm_smmu_device *smmu,
>  		existing = rb_find_add(&new_stream->node, &smmu->streams,
>  				       arm_smmu_streams_cmp_node);
>  		if (existing) {
> -			struct arm_smmu_master *existing_master =
> -				rb_entry(existing, struct arm_smmu_stream, node)
> -					->master;
> +			struct arm_smmu_stream *existing_stream =
> +				rb_entry(existing, struct arm_smmu_stream, node);
>  
>  			/* Bridged PCI devices may end up with duplicated IDs */
> -			if (existing_master == master)
> +			if (existing_stream->master == master)
>  				continue;
>  
> -			dev_warn(master->dev,
> -				 "Aliasing StreamID 0x%x (from %s) unsupported, expect DMA to be broken\n",
> -				 sid, dev_name(existing_master->dev));
> -			ret = -ENODEV;
> -			break;
> +			if (master->num_streams != 1) {
> +				dev_warn(master->dev,
> +					 "Shared StreamID 0x%x not supported for multi-SID masters\n",
> +					 sid);
> +				ret = -ENODEV;
> +				break;
> +			}
> +
> +			list_add_tail(&master->shared_masters_elm,
> +				      &existing_stream->shared_masters);

This version still uses the duplicated stream structures, not the
shared stream structure that I pointed out here:
https://lore.kernel.org/linux-iommu/arG6hmng3NddGEHm@xxxxxxxxxx/
?

This will have problems.

  master_b->stream[0] --> new shared stream (SID=X) {shared_masters}
           |
           ---------------- shared_masters_elm ----------|
                                                         v
  master_a->stream[0] --> existing stream (SID=X) {shared_masters}
           |
           ---------------- shared_masters_elm -----> {NULL}

First, the owner (master_a) doesn't add its shared_masters_elm to
any shared_masters list. This would fail the test on the owner:
    !list_empty(&master->shared_masters_elm)

Then, ...

> @@ -4167,14 +4189,38 @@ static void arm_smmu_remove_master(struct arm_smmu_master *master)
>  {
>  	int i;
>  	struct arm_smmu_device *smmu = master->smmu;
> -	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(master->dev);
>  
>  	if (!smmu || !master->streams)
>  		return;
>  
>  	mutex_lock(&smmu->streams_mutex);
> -	for (i = 0; i < fwspec->num_ids; i++)
> -		rb_erase(&master->streams[i].node, &smmu->streams);
> +	list_del_init(&master->shared_masters_elm);
> +	for (i = 0; i < master->num_streams; i++) {
> +		u32 sid = master->streams[i].id;
> +		struct arm_smmu_stream *stream;
> +
> +		if (i > 0 && master->streams[i - 1].id == sid)
> +			continue;
> +
> +		stream = arm_smmu_find_stream(smmu, sid);
> +		if (!stream)
> +			continue;
> +
> +		if (stream->master != master)
> +			continue;
> +
> +		if (list_empty(&stream->shared_masters)) {
> +			rb_erase(&stream->node, &smmu->streams);
> +		} else {
> +			struct arm_smmu_master *next;
> +
> +			next = list_first_entry(&stream->shared_masters,
> +					struct arm_smmu_master,
> +					shared_masters_elm);
> +			list_del_init(&next->shared_masters_elm);
> +			stream->master = next;
> +		}
> +	}
>  	mutex_unlock(&smmu->streams_mutex);
>  
>  	kfree(master->streams);

.. when the owner stream gets freed with the master_a, the shared
stream would UAF:

  master_b->stream[0] --> new shared stream (SID=X) {shared_masters}
           |
           ---------------- shared_masters_elm ----------|
                                                         v
                                                      {freed}

This should be changed to the model that I suggested in v2:

      |---------------------------------------------------------|
      |                                                         |
      |     |----------- shared_masters_elm --------|           |
      v     |                                       |           |
  master_a->stream[0] --|                           v           |
                        |--> stream (SID=X) {shared_masters; master;}
  master_b->stream[0] --|                           ^
            |                                       |
            |----------- shared_masters_elm --------|

When any master is removed:
 * Delink its shared_masters_elm
 * Free the shared stream when its shared_masters is empty

If shared_masters isn't empty but the owner is removed:
 * Give the ownership (stream->master pointer) to the next master

Nicolin