Re: [PATCH 1/3] iommu/msm: track a context master per device and IOMMU

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

New issues:
- [High] Incomplete Refactoring breaking Device Attachment for Shared IOMMUs.

Pre-existing issues:
- [High] Out-of-bounds Read / Undefined Behavior due to `list_first_entry()` on potentially empty `ctx_list`.
- [High] Lockless Access / Race Condition during TLB flushes iterating over `ctx_list`.
--

--- Patch [1]: [PATCH 1/3] iommu/msm: track a context master per device and IOMMU ---
commit c8d40e4c746f720804d5ad115bf7e9dd560b624f
Author: Dmitry Baryshkov <[email protected]>

iommu/msm: track a context master per device and IOMMU

insert_iommu_master() decided whether to allocate a struct
msm_iommu_ctx_dev by testing whether the IOMMU's ctx_list was empty,
caching the result in dev_iommu_priv, then unconditionally dereferenced
the master.

> diff --git a/drivers/iommu/msm_iommu.c b/drivers/iommu/msm_iommu.c
> index 4ef1e8c8244b0..aab87ccbcada9 100644
> --- a/drivers/iommu/msm_iommu.c
> +++ b/drivers/iommu/msm_iommu.c
> @@ -601,24 +601,27 @@ static int insert_iommu_master(struct device *dev,
>  				struct msm_iommu_dev **iommu,
>  				const struct of_phandle_args *spec)
>  {
[ ... ]
> -	if (list_empty(&(*iommu)->ctx_list)) {
> -		master = kzalloc_obj(*master, GFP_ATOMIC);
> -		if (!master) {
> -			dev_err(dev, "Failed to allocate iommu_master\n");
> -			return -ENOMEM;
> -		}
> -		master->of_node = dev->of_node;
> -		list_add(&master->list, &(*iommu)->ctx_list);
> -		dev_iommu_priv_set(dev, master);
> +	/* A master describes one device on one IOMMU instance. */
> +	list_for_each_entry(master, &(*iommu)->ctx_list, list)
> +		if (master->of_node == dev->of_node)
> +			goto add_sid;
> +
> +	master = kzalloc_obj(*master, GFP_ATOMIC);
> +	if (!master) {
> +		dev_err(dev, "Failed to allocate iommu_master\n");
> +		return -ENOMEM;
>  	}
> +	master->of_node = dev->of_node;
> +	list_add(&master->list, &(*iommu)->ctx_list);

[Severity: High]
Is it possible this change breaks device attachment for shared IOMMUs?

By allowing multiple masters to be added to ctx_list, find_iommu_for_dev()
and msm_iommu_attach_dev() might fail because they still only check the
first entry:

drivers/iommu/msm_iommu.c:find_iommu_for_dev() {
    master = list_first_entry(&iommu->ctx_list,
                              struct msm_iommu_ctx_dev,
                              list);
    if (master->of_node == dev->of_node) {
    ...
}

When a second device probes, it is added to the head of the list. When the
first device subsequently tries to attach, it would compare its of_node
against the new head (the second device), fail the match, and return -ENODEV.

Furthermore, when a match is found in msm_iommu_attach_dev(), it iterates
over all masters in ctx_list (even those belonging to other devices):

drivers/iommu/msm_iommu.c:msm_iommu_attach_dev() {
    ...
    list_for_each_entry(master, &iommu->ctx_list, list) {
        if (master->num) {
            dev_err(dev, "domain already attached");
    ...
}

Will this improperly attach all devices sharing the IOMMU to the same domain,
breaking domain isolation?

[Severity: High]
This isn't a bug introduced by this patch, but could calling
list_first_entry() on a potentially empty ctx_list cause an out-of-bounds
read?

During device probe, find_iommu_for_dev() and msm_iommu_attach_dev() iterate
over all registered IOMMUs in qcom_iommu_devices and unconditionally call
list_first_entry():

drivers/iommu/msm_iommu.c:find_iommu_for_dev() {
    master = list_first_entry(&iommu->ctx_list,
                              struct msm_iommu_ctx_dev,
                              list);
    if (master->of_node == dev->of_node) {
    ...
}

If an IOMMU has been registered but no devices have been attached to it yet
(such as during early bus probing), its ctx_list is empty. list_first_entry
on an empty list yields an invalid pointer.

Could this lead to a panic via page fault when master->of_node is
dereferenced?

[Severity: High]
This is a pre-existing issue, but does using list_add() here introduce a race
condition with concurrent lockless readers?

The TLB flush functions (__flush_iotlb and __flush_iotlb_range) iterate over
iommu->ctx_list without holding msm_iommu_lock or using RCU primitives:

drivers/iommu/msm_iommu.c:__flush_iotlb() {
    ...
    list_for_each_entry(master, &iommu->ctx_list, list)
        SET_CTX_TLBIALL(iommu->base, master->num, 0);
    ...
}

At the same time, insert_iommu_master() modifies this same ctx_list using
list_add() (a non-RCU primitive) while holding msm_iommu_lock.

Since this patch allows multiple masters per IOMMU, can a concurrent TLB
flush observe torn pointers or partially initialized nodes if a second device
is probed while the first device is actively performing DMA/unmap operations?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1