Re: [PATCH] iommu/vt-d: Fix kernel NULL pointer dereference in cache_tag_flush_range_np()

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

On 2024/12/12 18:01, Duan, Zhenzhong wrote:
> Hi Yi,
> 
>> -----Original Message-----
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Sent: Thursday, December 12, 2024 5:29 PM
>> Subject: Re: [PATCH] iommu/vt-d: Fix kernel NULL pointer dereference in
>> cache_tag_flush_range_np()
>>
>> On 2024/12/12 16:19, Duan, Zhenzhong wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Liu, Yi L <yi.l.liu@intel.com>
>>>> Sent: Thursday, December 12, 2024 3:45 PM
>>>> Subject: Re: [PATCH] iommu/vt-d: Fix kernel NULL pointer dereference in
>>>> cache_tag_flush_range_np()
>>>>
>>>> On 2024/12/12 15:24, Zhenzhong Duan wrote:
>>>>> When setup mapping on an paging domain before the domain is attached to
>>>> any
>>>>> device, a NULL pointer dereference triggers as below:
>>>>>
>>>>>     BUG: kernel NULL pointer dereference, address: 0000000000000200
>>>>>     #PF: supervisor read access in kernel mode
>>>>>     #PF: error_code(0x0000) - not-present page
>>>>>     RIP: 0010:cache_tag_flush_range_np+0x114/0x1f0
>>>>>     ...
>>>>>     Call Trace:
>>>>>      <TASK>
>>>>>      ? __die+0x20/0x70
>>>>>      ? page_fault_oops+0x80/0x150
>>>>>      ? do_user_addr_fault+0x5f/0x670
>>>>>      ? pfn_to_dma_pte+0xca/0x280
>>>>>      ? exc_page_fault+0x78/0x170
>>>>>      ? asm_exc_page_fault+0x22/0x30
>>>>>      ? cache_tag_flush_range_np+0x114/0x1f0
>>>>>      intel_iommu_iotlb_sync_map+0x16/0x20
>>>>>      iommu_map+0x59/0xd0
>>>>>      batch_to_domain+0x154/0x1e0
>>>>>      iopt_area_fill_domains+0x106/0x300
>>>>>      iopt_map_pages+0x1bc/0x290
>>>>>      iopt_map_user_pages+0xe8/0x1e0
>>>>>      ? xas_load+0x9/0xb0
>>>>>      iommufd_ioas_map+0xc9/0x1c0
>>>>>      iommufd_fops_ioctl+0xff/0x1b0
>>>>>      __x64_sys_ioctl+0x87/0xc0
>>>>>      do_syscall_64+0x50/0x110
>>>>>      entry_SYSCALL_64_after_hwframe+0x76/0x7e
>>>>>
>>>>> qi_batch structure is allocated when domain is attached to a device for the
>>>>> first time, when a mapping is setup before that, qi_batch is referenced to
>>>>> do batched flush and trigger above issue.
>>>>>
>>>>> Fix it by checking qi_batch pointer and bypass batched flushing if no
>>>>> device is attached.
>>>>>
>>>>> Cc: stable@vger.kernel.org
>>>>> Fixes: 705c1cdf1e73 ("iommu/vt-d: Introduce batched cache invalidation")
>>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>>>> ---
>>>>>     drivers/iommu/intel/cache.c | 2 +-
>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/iommu/intel/cache.c b/drivers/iommu/intel/cache.c
>>>>> index e5b89f728ad3..bb9dae9a7fba 100644
>>>>> --- a/drivers/iommu/intel/cache.c
>>>>> +++ b/drivers/iommu/intel/cache.c
>>>>> @@ -264,7 +264,7 @@ static unsigned long
>>>> calculate_psi_aligned_address(unsigned long start,
>>>>>
>>>>>     static void qi_batch_flush_descs(struct intel_iommu *iommu, struct
>> qi_batch
>>>> *batch)
>>>>>     {
>>>>> -	if (!iommu || !batch->index)
>>>>> +	if (!iommu || !batch || !batch->index)
>>>>
>>>> this is the same issue in the below link. :) And we should fix it by
>>>> allocating the qi_batch for parent domains. The nested parent domains is
>>>> not going to be attached to device at all. It acts more as a background
>>>> domain, so this fix will miss the future cache flushes. It would have
>>>> bigger problems. :)
>>>>
>>>> https://lore.kernel.org/linux-iommu/20241210130322.17175-1-
>>>> yi.l.liu@intel.com/
>>>
>>> Ah, just see this😊
>>> This patch tries to fix an issue that mapping setup on a paging domain before
>>> it's attached to a device, your patch fixed an issue that nested parent
>>> domain's qi_batch is not allocated even if nested domain is attached to
>>> a device. I think they are different issues?
>>
>> Oops. I see. I think your case is allocating a hwpt based on an IOAS that
>> already has mappings. When the hwpt is added to it, all the mappings of
>> this IOAS would be pushing to the hwpt. But the hwpt has not been attached
>> yet, so hit this issue. I remember there is a immediate_attach bool to let
>> iommufd_hwpt_paging_alloc() do an attach before calling
>> iopt_table_add_domain() which pushes the IOAS mappings to domain.
>>
>> One possible fix is to set the immediate_attach to be true. But I doubt if
>> it will be agreed since it was introduced due to some gap on ARM side. If
>> that gap has been resolved, this behavior would go away as well.
>>
>> So back to this issue, with this change, the flush would be skipped. It
>> looks ok to me to skip cache flush for map path. And we should not expect
>> any unmap on this domain since there is no device attached (parent domain
>> is an exception), hence nothing to be flushed even there is unmap in the
>> domain's IOAS. So it appears to be a acceptable fix. @Baolu, your opinion?
> 
> Hold on, it looks I'm wrong on analyzing related code qi_batch_flush_descs().
> The iommu should always be NULL in my suspected case, so
> qi_batch_flush_descs() will return early without issue.
> 
> I reproduced the backtrace when playing with iommufd qemu nesting, I think your
> previous comment is correct, I misunderstood the root cause of it, it's indeed
> due to nesting parent domain not paging domain. Please ignore this patch.

Great. I also had a try to allocate hwpt with an IOAS that has already got
a bunch of mappings, it can work as the iommu is null.

@Baolu, you may ignore this patch as it's already fixed.

-- 
Regards,
Yi Liu