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

[Yi Liu <yi.l.liu@intel.com>]

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