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/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