linux-kernel - Re: [PATCH 8/8] iommu/vt-d: Add set_dev

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: <a19031b0-7c30-45e6-b171-c53e3578b867@intel.com>
Date:   Thu, 14 Dec 2023 10:55:25 +0800
From:   "Yang, Weijiang" <weijiang.yang@...el.com>
To:     Yi Liu <yi.l.liu@...el.com>, <baolu.lu@...ux.intel.com>
CC:     <robin.murphy@....com>, <kevin.tian@...el.com>, <jgg@...dia.com>,
        <alex.williamson@...hat.com>, <joro@...tes.org>,
        <cohuck@...hat.com>, <eric.auger@...hat.com>,
        <nicolinc@...dia.com>, <kvm@...r.kernel.org>,
        <mjrosato@...ux.ibm.com>, <chao.p.peng@...ux.intel.com>,
        <yi.y.sun@...ux.intel.com>, <peterx@...hat.com>,
        <jasowang@...hat.com>, <shameerali.kolothum.thodi@...wei.com>,
        <lulu@...hat.com>, <suravee.suthikulpanit@....com>,
        <iommu@...ts.linux.dev>, <linux-kernel@...r.kernel.org>,
        <linux-kselftest@...r.kernel.org>, <zhenzhong.duan@...el.com>,
        <joao.m.martins@...cle.com>, <xin.zeng@...el.com>,
        <yan.y.zhao@...el.com>
Subject: Re: [PATCH 8/8] iommu/vt-d: Add set_dev_pasid callback for nested
 domain

On 11/27/2023 2:34 PM, Yi Liu wrote:
> From: Lu Baolu <baolu.lu@...ux.intel.com>
>
> This allows the upper layers to set a nested type domain to a PASID of a
> device if the PASID feature is supported by the IOMMU hardware.
>
> The set_dev_pasid callback for non-nest domain has already be there, so
> this only needs to add it for nested domains.
>
> Signed-off-by: Lu Baolu <baolu.lu@...ux.intel.com>
> Signed-off-by: Yi Liu <yi.l.liu@...el.com>
> ---
>   drivers/iommu/intel/nested.c | 47 ++++++++++++++++++++++++++++++++++++
>   1 file changed, 47 insertions(+)
>
> diff --git a/drivers/iommu/intel/nested.c b/drivers/iommu/intel/nested.c
> index 44ad48db7ea0..f6f687750104 100644
> --- a/drivers/iommu/intel/nested.c
> +++ b/drivers/iommu/intel/nested.c
> @@ -68,6 +68,52 @@ static int intel_nested_attach_dev(struct iommu_domain *domain,
>   	return 0;
>   }
>   
> +static int intel_nested_set_dev_pasid(struct iommu_domain *domain,
> +				      struct device *dev, ioasid_t pasid)
> +{
> +	struct device_domain_info *info = dev_iommu_priv_get(dev);
> +	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
> +	struct intel_iommu *iommu = info->iommu;
> +	struct dev_pasid_info *dev_pasid;
> +	unsigned long flags;
> +	int ret = 0;
> +
> +	if (!pasid_supported(iommu))
> +		return -EOPNOTSUPP;
> +
> +	if (iommu->agaw < dmar_domain->s2_domain->agaw)
> +		return -EINVAL;
> +
> +	ret = prepare_domain_attach_device(&dmar_domain->s2_domain->domain, dev);
> +	if (ret)
> +		return ret;
> +
> +	dev_pasid = kzalloc(sizeof(*dev_pasid), GFP_KERNEL);
> +	if (!dev_pasid)
> +		return -ENOMEM;
> +
> +	ret = domain_attach_iommu(dmar_domain, iommu);
> +	if (ret)
> +		goto err_free;
> +
> +	ret = intel_pasid_setup_nested(iommu, dev, pasid, dmar_domain);
> +	if (ret)
> +		goto err_detach_iommu;
> +
> +	dev_pasid->dev = dev;
> +	dev_pasid->pasid = pasid;
> +	spin_lock_irqsave(&dmar_domain->lock, flags);
> +	list_add(&dev_pasid->link_domain, &dmar_domain->dev_pasids);
> +	spin_unlock_irqrestore(&dmar_domain->lock, flags);

---> list_add(&dev_pasid->link_domain, &dmar_domain->dev_pasids);

dev_pasid is linked at later time, this leads to domain->has_iotlb_device is not correctly set, which finally results into a missing of device iotlb flush in iommu_flush_dev_iotlb()when it's called.
Check this call path:
domain_attach_iommu()->domain_update_iommu_cap()->domain_update_iotlb()->domain->has_iotlb_device = has_iotlb_device; The ugly fixup is to call domain_update_iommu_cap() or domain_update_iotlb() here again before return.
The similar issue is in intel_iommu_set_dev_pasid() and intel_nested_attach_dev().
> +
> +	return 0;
> +err_detach_iommu:
> +	domain_detach_iommu(dmar_domain, iommu);
> +err_free:
> +	kfree(dev_pasid);
> +	return ret;
> +}
> +
>   static void intel_nested_domain_free(struct iommu_domain *domain)
>   {
>   	kfree(to_dmar_domain(domain));
> @@ -128,6 +174,7 @@ static int intel_nested_cache_invalidate_user(struct iommu_domain *domain,
>   
>   static const struct iommu_domain_ops intel_nested_domain_ops = {
>   	.attach_dev		= intel_nested_attach_dev,
> +	.set_dev_pasid		= intel_nested_set_dev_pasid,
>   	.free			= intel_nested_domain_free,
>   	.cache_invalidate_user	= intel_nested_cache_invalidate_user,
>   };