linux-kernel - Re: [RFC 1/1] swiotlb: Replace BUG_ON() with graceful error handling

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: <CAOZ5it1EGwZJYVw42mA=BGwS7LFTo=QMuOuq7trvU5C+u7EiZg@mail.gmail.com>
Date: Fri, 6 Dec 2024 13:36:28 -0700
From: Brian Johannesmeyer <bjohannesmeyer@...il.com>
To: Robin Murphy <robin.murphy@....com>
Cc: Tianyu Lan <Tianyu.Lan@...rosoft.com>, Michael Kelley <mikelley@...rosoft.com>, 
	Christoph Hellwig <hch@....de>, Marek Szyprowski <m.szyprowski@...sung.com>, iommu@...ts.linux.dev, 
	linux-kernel@...r.kernel.org, Raphael Isemann <teemperor@...il.com>, 
	Cristiano Giuffrida <giuffrida@...vu.nl>, Herbert Bos <h.j.bos@...nl>, Greg KH <gregkh@...uxfoundation.org>
Subject: Re: [RFC 1/1] swiotlb: Replace BUG_ON() with graceful error handling

Thank you for your feedback and your patience. Apologies for the
delayed response --- I've had some personal matters arise in the past
couple weeks.

I had initially prepared responses addressing each of your points, but
after revisiting Robin's insightful observation below, it seems this
issue has already been resolved in the latest kernel. While I’m still
happy to address any further questions or comments, it appears that
additional discussion may no longer be necessary.

On Mon, Nov 25, 2024 at 8:03 AM Robin Murphy <robin.murphy@....com> wrote:
> Hmm, looking again, how exactly *does* this happen? To get here from
> swiotlb_unmap_single(), swiotlb_find_pool() has already determined that
> "tlb_addr" is within the range belonging to "mem", so if it is somehow
> possible for it to then convert into an out-of-bounds index, maybe that
> does actually imply some bug in SWIOTLB itself where "mem" is
> misconfigured...

Great observation. Upon reviewing the latest kernel, I realized I had
analyzed an older version of the code that lacked commit [0]. This
commit introduces swiotlb_find_pool() into swiotlb_tbl_unmap_single(),
effectively mitigating the issue. Specifically, if addr is corrupted,
swiotlb_find_pool() would return NULL, causing
swiotlb_tbl_unmap_single() to now return without invoking
__swiotlb_tbl_unmap_single(). Consequently, the failed BUG_ON in
swiotlb_release_slots() is avoided. My apologies for overlooking this
recent change and not verifying the code path in the latest kernel.

That said, converting the BUG_ON to a WARN_ON_ONCE might still be a
useful improvement, even if the immediate issue is resolved.

Sincerely,

Brian Johannesmeyer

[0] commit 7296f2301a057493e97b07739213c6e864f76891 ("swiotlb: reduce
swiotlb pool lookups")