Re: [PATCH v2 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] Lookup and attach paths fail to walk the full ctx_list, breaking support for multiple devices per IOMMU, and causing out-of-bounds reads on empty lists.

Pre-existing issues:
- [High] Memory leak of `io_pgtable_ops` and associated page tables in `msm_iommu_domain_free`.
- [High] Attaching multiple devices to the same domain overwrites the domain configuration, leaking page tables and destroying spinlocks.
--

--- Patch [1]: [PATCH v2 1/3] iommu/msm: track a context master per device and IOMMU ---
commit f63bf88b6236e0420150c158cf3fee05438de180
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:

[ ... ]

>     Neither key is right. A master describes one device on one IOMMU
>     instance: the attach and lookup paths find a device on an IOMMU by
>     walking that IOMMU's ctx_list for a master whose of_node matches. The

[Severity: High]
Does this code actually walk the list as described?

Looking at find_iommu_for_dev() and msm_iommu_attach_dev() in
drivers/iommu/msm_iommu.c, it appears they only check the first entry:

	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) {

Will this fail to attach any subsequently added devices on the same IOMMU,
and also cause an out-of-bounds read if the list is empty?

>     ctx_list emptiness test is per-IOMMU, and the dev_iommu_priv cache is
>     per-device, so the two disagree as soon as one device spans several

[ ... ]

> diff --git a/drivers/iommu/msm_iommu.c b/drivers/iommu/msm_iommu.c
> index 4ef1e8c8244b0..d6ef17cf6d108 100644
> --- a/drivers/iommu/msm_iommu.c
> +++ b/drivers/iommu/msm_iommu.c

[ ... ]

[Severity: High]
This is a pre-existing issue, but does msm_iommu_domain_free() leak the page
tables?

drivers/iommu/msm_iommu.c:msm_iommu_domain_free() {
    ...
    priv = to_msm_priv(domain);
    kfree(priv);
    ...
}

Should this call free_io_pgtable_ops(priv->iop) before freeing the priv
struct to avoid leaking the io_pgtable_ops and associated page tables?


[Severity: High]
This is a pre-existing issue, but does msm_iommu_attach_dev() overwrite the
domain configuration when attaching multiple devices to the same domain?

drivers/iommu/msm_iommu.c:msm_iommu_attach_dev() {
    ...
	priv->dev = dev;
	msm_iommu_domain_config(priv);
    ...
}

Does unconditionally calling msm_iommu_domain_config() for every attach
operation overwrite the priv->iop pointer (leaking the original page tables)
and re-initialize the page table lock (pgtlock), destroying any active lock
state?

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