linux-kernel - Re: [PATCH] iommu: Remove redundant null check in iommu_iotlb_gather

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAOUjTQukFZd2bgB67u+3gn6b4S3RLur07+iZS3fiznjU26e_+Q@mail.gmail.com>
Date: Mon, 24 Nov 2025 17:07:07 -0800
From: Aashish Sharma <aashish@...hishsharma.net>
To: Robin Murphy <robin.murphy@....com>
Cc: Will Deacon <will@...nel.org>, Vineeth Pillai <vineeth@...byteword.org>, 
	Dmytro Maluka <dmaluka@...gle.com>, Joerg Roedel <joro@...tes.org>, iommu@...ts.linux.dev, 
	linux-kernel@...r.kernel.org, Aashish Sharma <shraash@...gle.com>
Subject: Re: [PATCH] iommu: Remove redundant null check in iommu_iotlb_gather_queued

On Mon, Nov 24, 2025 at 9:16 AM Robin Murphy <robin.murphy@....com> wrote:
>
> On 2025-11-24 5:03 pm, Will Deacon wrote:
> > On Mon, Nov 10, 2025 at 09:19:52PM -0800, Aashish Sharma wrote:
> >> The callers of 'iommu_iotlb_gather_queued' always have a non-null
> >> 'gather', so we don't need to check for the same in the function.
> >
> > That's not obviously true to me, how did you check it?
> >
> > For example, msm_iommu_pagetable_unmap() appears to call into
> > io_pgtable_ops::unmap_pages() with a NULL gather pointer.
>
> Indeed I think the only redundant check is the one which seems to have
> snuck into __arm_lpae_unmap() not so long ago (which the other callers
> never had and still don't.)

Apologies, you are correct; I missed the invocations where gather can
indeed be NULL. Please disregard the patch in its current form.

However, I'd like to share the motivation behind the change, as it
might point to a semantic fragility in the current API.

While auditing the ARM io-pgtable code, I noticed that whenever
iommu_iotlb_gather_queued() returns false, the code immediately calls
io_pgtable_tlb_add_page(), except in __arm_lpae_unmap(). This
eventually invokes iommu_flush_ops::tlb_add_page(). Many
implementations of this op assume gather is non-NULL and dereference
it directly.

This creates a confusing situation: iommu_iotlb_gather_queued()
returns false for two distinct conditions:
1. gather is NULL.
2. gather is valid but the queue is empty.

The callers, however, seem to implicitly assume the second case (empty
queue) and proceed to operations that would crash if gather were
actually NULL. For example, if __arm_v7s_unmap() is called with a NULL
gather, it can lead to a crash:

__arm_v7s_unmap -> io_pgtable_tlb_add_page ->
arm_smmu_tlb_inv_page_nosync -> iommu_iotlb_gather_add_page -> crash

While the lack of reported crashes suggests gather isn't NULL in these
specific paths, relying on iommu_iotlb_gather_queued() to handle NULL
pointers feels inherently risky given how the return value is
interpreted.

Do you think it would be valuable to refactor this so that the helper
strictly checks the queue status, making the callers explicitly
responsible for NULL checks, like in __arm_lpae_unmap()?

Thanks
Aashish