[PATCH v2] iommu/vt-d: Fix qi_batch NULL pointer with nested parent domain

[PATCH v2] iommu/vt-d: Fix qi_batch NULL pointer with nested parent domain

[Yi Liu]

The qi_batch is allocated when assigning cache tag for a domain. While
for nested parent domain, it is missed. Hence, when trying to map pages
to the nested parent, NULL dereference occurred. Also, there is potential
memleak since there is no lock around domain->qi_batch allocation.

To solve it, add a helper for qi_batch allocation, and call it in both
the __cache_tag_assign_domain() and __cache_tag_assign_parent_domain().

  BUG: kernel NULL pointer dereference, address: 0000000000000200
  #PF: supervisor read access in kernel mode
  #PF: error_code(0x0000) - not-present page
  PGD 8104795067 P4D 0
  Oops: Oops: 0000 [#1] PREEMPT SMP NOPTI
  CPU: 223 UID: 0 PID: 4357 Comm: qemu-system-x86 Not tainted 6.13.0-rc1-00028-g4b50c3c3b998-dirty #2632
  Call Trace:
   ? __die+0x24/0x70
   ? page_fault_oops+0x80/0x150
   ? do_user_addr_fault+0x63/0x7b0
   ? exc_page_fault+0x7c/0x220
   ? asm_exc_page_fault+0x26/0x30
   ? cache_tag_flush_range_np+0x13c/0x260
   intel_iommu_iotlb_sync_map+0x1a/0x30
   iommu_map+0x61/0xf0
   batch_to_domain+0x188/0x250
   iopt_area_fill_domains+0x125/0x320
   ? rcu_is_watching+0x11/0x50
   iopt_map_pages+0x63/0x100
   iopt_map_common.isra.0+0xa7/0x190
   iopt_map_user_pages+0x6a/0x80
   iommufd_ioas_map+0xcd/0x1d0
   iommufd_fops_ioctl+0x118/0x1c0
   __x64_sys_ioctl+0x93/0xc0
   do_syscall_64+0x71/0x140
   entry_SYSCALL_64_after_hwframe+0x76/0x7e

Fixes: 705c1cdf1e73 ("iommu/vt-d: Introduce batched cache invalidation")
Co-developed-by: Lu Baolu <baolu.lu@linux.intel.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
Change log:
v2:
 - Fix it by allocating domain->qi_batch under domain->cache_lock (Baolu)
 - Simplify the calltrace log in commit message (Baolu)

v1: https://lore.kernel.org/linux-iommu/20241207120304.5710-1-yi.l.liu@intel.com/
---
 drivers/iommu/intel/cache.c | 34 +++++++++++++++++++++++++++-------
 1 file changed, 27 insertions(+), 7 deletions(-)

diff --git a/drivers/iommu/intel/cache.c b/drivers/iommu/intel/cache.c
index e5b89f728ad3..09694cca8752 100644
--- a/drivers/iommu/intel/cache.c
+++ b/drivers/iommu/intel/cache.c
@@ -105,12 +105,35 @@ static void cache_tag_unassign(struct dmar_domain *domain, u16 did,
 	spin_unlock_irqrestore(&domain->cache_lock, flags);
 }
 
+/* domain->qi_batch will be freed in iommu_free_domain() path. */
+static int domain_qi_batch_alloc(struct dmar_domain *domain)
+{
+	unsigned long flags;
+	int ret = 0;
+
+	spin_lock_irqsave(&domain->cache_lock, flags);
+	if (domain->qi_batch)
+		goto out_unlock;
+
+	domain->qi_batch = kzalloc(sizeof(*domain->qi_batch), GFP_ATOMIC);
+	if (!domain->qi_batch)
+		ret = -ENOMEM;
+out_unlock:
+	spin_unlock_irqrestore(&domain->cache_lock, flags);
+
+	return ret;
+}
+
 static int __cache_tag_assign_domain(struct dmar_domain *domain, u16 did,
 				     struct device *dev, ioasid_t pasid)
 {
 	struct device_domain_info *info = dev_iommu_priv_get(dev);
 	int ret;
 
+	ret = domain_qi_batch_alloc(domain);
+	if (ret)
+		return ret;
+
 	ret = cache_tag_assign(domain, did, dev, pasid, CACHE_TAG_IOTLB);
 	if (ret || !info->ats_enabled)
 		return ret;
@@ -139,6 +162,10 @@ static int __cache_tag_assign_parent_domain(struct dmar_domain *domain, u16 did,
 	struct device_domain_info *info = dev_iommu_priv_get(dev);
 	int ret;
 
+	ret = domain_qi_batch_alloc(domain);
+	if (ret)
+		return ret;
+
 	ret = cache_tag_assign(domain, did, dev, pasid, CACHE_TAG_NESTING_IOTLB);
 	if (ret || !info->ats_enabled)
 		return ret;
@@ -190,13 +217,6 @@ int cache_tag_assign_domain(struct dmar_domain *domain,
 	u16 did = domain_get_id_for_dev(domain, dev);
 	int ret;
 
-	/* domain->qi_bach will be freed in iommu_free_domain() path. */
-	if (!domain->qi_batch) {
-		domain->qi_batch = kzalloc(sizeof(*domain->qi_batch), GFP_KERNEL);
-		if (!domain->qi_batch)
-			return -ENOMEM;
-	}
-
 	ret = __cache_tag_assign_domain(domain, did, dev, pasid);
 	if (ret || domain->domain.type != IOMMU_DOMAIN_NESTED)
 		return ret;
-- 
2.34.1

RE: [PATCH v2] iommu/vt-d: Fix qi_batch NULL pointer with nested parent domain

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Tuesday, December 10, 2024 9:03 PM
> 
> The qi_batch is allocated when assigning cache tag for a domain. While
> for nested parent domain, it is missed. Hence, when trying to map pages
> to the nested parent, NULL dereference occurred. Also, there is potential
> memleak since there is no lock around domain->qi_batch allocation.

Out of curiosity. In which case does the VMM try to change the parent
domain mapping when nesting is enabled? I know this is allowed and this
patch does the right fix, but just curious how it's triggered.

> +/* domain->qi_batch will be freed in iommu_free_domain() path. */
> +static int domain_qi_batch_alloc(struct dmar_domain *domain)
> +{
> +	unsigned long flags;
> +	int ret = 0;
> +
> +	spin_lock_irqsave(&domain->cache_lock, flags);
> +	if (domain->qi_batch)
> +		goto out_unlock;
> +

V1 discussion ends up with an agreement to use domain-lock, but
not followed here. any reason?

Re: [PATCH v2] iommu/vt-d: Fix qi_batch NULL pointer with nested parent domain

[Yi Liu]

On 2024/12/11 16:35, Tian, Kevin wrote:
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Sent: Tuesday, December 10, 2024 9:03 PM
>>
>> The qi_batch is allocated when assigning cache tag for a domain. While
>> for nested parent domain, it is missed. Hence, when trying to map pages
>> to the nested parent, NULL dereference occurred. Also, there is potential
>> memleak since there is no lock around domain->qi_batch allocation.
> 
> Out of curiosity. In which case does the VMM try to change the parent
> domain mapping when nesting is enabled? I know this is allowed and this
> patch does the right fix, but just curious how it's triggered.

This happens when there is map on the domain. It can be easily produced
when userspace allocates a s2_hwpt but never attach it to device. When
there is page mapped to the IOAS, the mapping is then populated to the
hwpt and its domain.

In my test, I just allocate s2_hwpt with nested parent flag, and allocate
nested domain with it. Then I attach nested domain, and replay all the
GPA mappings to s2_hwpt. It hit this issue.

> 
>> +/* domain->qi_batch will be freed in iommu_free_domain() path. */
>> +static int domain_qi_batch_alloc(struct dmar_domain *domain)
>> +{
>> +	unsigned long flags;
>> +	int ret = 0;
>> +
>> +	spin_lock_irqsave(&domain->cache_lock, flags);
>> +	if (domain->qi_batch)
>> +		goto out_unlock;
>> +
> 
> V1 discussion ends up with an agreement to use domain-lock, but
> not followed here. any reason?

Aha, there is a confusion between Baolu and I. So we had an offline chat.
We confirmed there is no locking issue using domain->cache_lock. And using
it is better then domain->lock since this is all about cache. I should
have mentioned it in the change log. :)


Regards,
Yi Liu

RE: [PATCH v2] iommu/vt-d: Fix qi_batch NULL pointer with nested parent domain

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Wednesday, December 11, 2024 5:27 PM
> 
> On 2024/12/11 16:35, Tian, Kevin wrote:
> >> From: Liu, Yi L <yi.l.liu@intel.com>
> >> Sent: Tuesday, December 10, 2024 9:03 PM
> >>
> >> The qi_batch is allocated when assigning cache tag for a domain. While
> >> for nested parent domain, it is missed. Hence, when trying to map pages
> >> to the nested parent, NULL dereference occurred. Also, there is potential
> >> memleak since there is no lock around domain->qi_batch allocation.
> >
> > Out of curiosity. In which case does the VMM try to change the parent
> > domain mapping when nesting is enabled? I know this is allowed and this
> > patch does the right fix, but just curious how it's triggered.
> 
> This happens when there is map on the domain. It can be easily produced
> when userspace allocates a s2_hwpt but never attach it to device. When
> there is page mapped to the IOAS, the mapping is then populated to the
> hwpt and its domain.
> 
> In my test, I just allocate s2_hwpt with nested parent flag, and allocate
> nested domain with it. Then I attach nested domain, and replay all the
> GPA mappings to s2_hwpt. It hit this issue.

so this is a case where there is already a s2_hwpt with valid mappings
before creating this parent s2. When replay happens this bug is hit.

> 
> >
> >> +/* domain->qi_batch will be freed in iommu_free_domain() path. */
> >> +static int domain_qi_batch_alloc(struct dmar_domain *domain)
> >> +{
> >> +	unsigned long flags;
> >> +	int ret = 0;
> >> +
> >> +	spin_lock_irqsave(&domain->cache_lock, flags);
> >> +	if (domain->qi_batch)
> >> +		goto out_unlock;
> >> +
> >
> > V1 discussion ends up with an agreement to use domain-lock, but
> > not followed here. any reason?
> 
> Aha, there is a confusion between Baolu and I. So we had an offline chat.
> We confirmed there is no locking issue using domain->cache_lock. And using
> it is better then domain->lock since this is all about cache. I should
> have mentioned it in the change log. :)
> 

Okay.

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

Re: [PATCH v2] iommu/vt-d: Fix qi_batch NULL pointer with nested parent domain

[Yi Liu]

On 2024/12/12 13:46, Tian, Kevin wrote:
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Sent: Wednesday, December 11, 2024 5:27 PM
>>
>> On 2024/12/11 16:35, Tian, Kevin wrote:
>>>> From: Liu, Yi L <yi.l.liu@intel.com>
>>>> Sent: Tuesday, December 10, 2024 9:03 PM
>>>>
>>>> The qi_batch is allocated when assigning cache tag for a domain. While
>>>> for nested parent domain, it is missed. Hence, when trying to map pages
>>>> to the nested parent, NULL dereference occurred. Also, there is potential
>>>> memleak since there is no lock around domain->qi_batch allocation.
>>>
>>> Out of curiosity. In which case does the VMM try to change the parent
>>> domain mapping when nesting is enabled? I know this is allowed and this
>>> patch does the right fix, but just curious how it's triggered.
>>
>> This happens when there is map on the domain. It can be easily produced
>> when userspace allocates a s2_hwpt but never attach it to device. When
>> there is page mapped to the IOAS, the mapping is then populated to the
>> hwpt and its domain.
>>
>> In my test, I just allocate s2_hwpt with nested parent flag, and allocate
>> nested domain with it. Then I attach nested domain, and replay all the
>> GPA mappings to s2_hwpt. It hit this issue.
> 
> so this is a case where there is already a s2_hwpt with valid mappings
> before creating this parent s2. When replay happens this bug is hit.

yes. In QEMU, we will allocate parent hwpt when guest iommu enables
translation (TE bit). We also have a choice to reuse the hwpt created
during VM boot up (allocated by VFIO module within QEMU), but we have RO
mapping errata, so creating a parent hwpt that does not have RO mappings is
better. When this parent hwpt is created, we would replay the GPA->HPA
mappings, hence hit the problem as the parent hwpt has not been attached to
this device at all. This also means, this issue can be hidden if the parent
hwpt has been attached to the device. And this is exactly the reason we
didn't see it in the qi_batch merging. My test application has attached the
parent hwpt to PASIDs of the device to test out the pasid attach path, so
it WA the issue by accident. Recently, I'm clean up the tests by adding
them one by one, then hit this bug.

-- 
Regards,
Yi Liu

RE: [PATCH v2] iommu/vt-d: Fix qi_batch NULL pointer with nested parent domain

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Thursday, December 12, 2024 3:28 PM
> 
> On 2024/12/12 13:46, Tian, Kevin wrote:
> >> From: Liu, Yi L <yi.l.liu@intel.com>
> >> Sent: Wednesday, December 11, 2024 5:27 PM
> >>
> >> On 2024/12/11 16:35, Tian, Kevin wrote:
> >>>> From: Liu, Yi L <yi.l.liu@intel.com>
> >>>> Sent: Tuesday, December 10, 2024 9:03 PM
> >>>>
> >>>> The qi_batch is allocated when assigning cache tag for a domain. While
> >>>> for nested parent domain, it is missed. Hence, when trying to map
> pages
> >>>> to the nested parent, NULL dereference occurred. Also, there is
> potential
> >>>> memleak since there is no lock around domain->qi_batch allocation.
> >>>
> >>> Out of curiosity. In which case does the VMM try to change the parent
> >>> domain mapping when nesting is enabled? I know this is allowed and
> this
> >>> patch does the right fix, but just curious how it's triggered.
> >>
> >> This happens when there is map on the domain. It can be easily produced
> >> when userspace allocates a s2_hwpt but never attach it to device. When
> >> there is page mapped to the IOAS, the mapping is then populated to the
> >> hwpt and its domain.
> >>
> >> In my test, I just allocate s2_hwpt with nested parent flag, and allocate
> >> nested domain with it. Then I attach nested domain, and replay all the
> >> GPA mappings to s2_hwpt. It hit this issue.
> >
> > so this is a case where there is already a s2_hwpt with valid mappings
> > before creating this parent s2. When replay happens this bug is hit.
> 
> yes. In QEMU, we will allocate parent hwpt when guest iommu enables
> translation (TE bit). We also have a choice to reuse the hwpt created
> during VM boot up (allocated by VFIO module within QEMU), but we have
> RO
> mapping errata, so creating a parent hwpt that does not have RO mappings
> is
> better. When this parent hwpt is created, we would replay the GPA->HPA
> mappings, hence hit the problem as the parent hwpt has not been attached
> to
> this device at all. This also means, this issue can be hidden if the parent
> hwpt has been attached to the device. And this is exactly the reason we
> didn't see it in the qi_batch merging. My test application has attached the
> parent hwpt to PASIDs of the device to test out the pasid attach path, so
> it WA the issue by accident. Recently, I'm clean up the tests by adding
> them one by one, then hit this bug.
> 

Thanks. That's a clear story for how this issue was identified.