linux-kernel - Re: [PATH v5 2/3] iommu/vt-d: debugfs: Create/remove debugfs file per {device, pasid}

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2590cecf-e1f9-4af9-8fbb-9b49f5e335c0@ijzerbout.nl>
Date: Tue, 12 Nov 2024 22:22:54 +0100
From: Kees Bakker <kees@...erbout.nl>
To: Jingqi Liu <Jingqi.liu@...el.com>, iommu@...ts.linux.dev,
 Lu Baolu <baolu.lu@...ux.intel.com>, Tian Kevin <kevin.tian@...el.com>,
 Joerg Roedel <joro@...tes.org>, Will Deacon <will@...nel.org>,
 Robin Murphy <robin.murphy@....com>
Cc: linux-kernel@...r.kernel.org
Subject: Re: [PATH v5 2/3] iommu/vt-d: debugfs: Create/remove debugfs file per
 {device, pasid}

Op 13-10-2023 om 15:58 schreef Jingqi Liu:
> Add a debugfs directory per pair of {device, pasid} if the mappings of
> its page table are created and destroyed by the iommu_map/unmap()
> interfaces. i.e. /sys/kernel/debug/iommu/intel/<device source id>/<pasid>.
> Create a debugfs file in the directory for users to dump the page
> table corresponding to {device, pasid}. e.g.
> /sys/kernel/debug/iommu/intel/0000:00:02.0/1/domain_translation_struct.
> For the default domain without pasid, it creates a debugfs file in the
> debugfs device directory for users to dump its page table. e.g.
> /sys/kernel/debug/iommu/intel/0000:00:02.0/domain_translation_struct.
>
> When setting a domain to a PASID of device, create a debugfs file in
> the pasid debugfs directory for users to dump the page table of the
> specified pasid. Remove the debugfs device directory of the device
> when releasing a device. e.g.
> /sys/kernel/debug/iommu/intel/0000:00:01.0
>
> Signed-off-by: Jingqi Liu <Jingqi.liu@...el.com>
> ---
>   drivers/iommu/intel/debugfs.c | 53 +++++++++++++++++++++++++++++++----
>   drivers/iommu/intel/iommu.c   |  7 +++++
>   drivers/iommu/intel/iommu.h   | 14 +++++++++
>   3 files changed, 69 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iommu/intel/debugfs.c b/drivers/iommu/intel/debugfs.c
> [...]
> +/* Remove the device pasid debugfs directory. */
> +void intel_iommu_debugfs_remove_dev_pasid(struct dev_pasid_info *dev_pasid)
> +{
> +	debugfs_remove_recursive(dev_pasid->debugfs_dentry);
> +}
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> [...]
> @@ -4710,6 +4713,7 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
>   	spin_unlock_irqrestore(&dmar_domain->lock, flags);
>   
>   	domain_detach_iommu(dmar_domain, iommu);
> +	intel_iommu_debugfs_remove_dev_pasid(dev_pasid);
>   	kfree(dev_pasid);
>   out_tear_down:
>   	intel_pasid_tear_down_entry(iommu, dev, pasid, false);
>

So, a call to intel_iommu_debugfs_remove_dev_pasid() was added.
There is a potential problem that dev_pasid can be NULL.
The diff doesn't show the whole context so let me give that here.
Today that piece of the code looks like this

         list_for_each_entry(curr, &dmar_domain->dev_pasids, link_domain) {
                 if (curr->dev == dev && curr->pasid == pasid) {
                         list_del(&curr->link_domain);
                         dev_pasid = curr;
                         break;
                 }
         }
         WARN_ON_ONCE(!dev_pasid);
         spin_unlock_irqrestore(&dmar_domain->lock, flags);

         cache_tag_unassign_domain(dmar_domain, dev, pasid);
         domain_detach_iommu(dmar_domain, iommu);
         intel_iommu_debugfs_remove_dev_pasid(dev_pasid);
         kfree(dev_pasid);

The for_each loop can exit without finding an entry.
The WARN_ON_ONCE also suggests that there can be a NULL.
After that the new function is called (see above) and it will
dereference the NULL pointer.

Can you have a closer look?
-- 
Kees Bakker