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/0x7eqi_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, structqi_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.