linux-kernel - Re: [PATCH v2 1/3] swiotlb: Fix allocation alignment requirement when searching slots

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]

Date: Thu, 1 Feb 2024 14:12:48 +0000
From: Will Deacon <will@...nel.org>
To: Robin Murphy <robin.murphy@....com>
Cc: linux-kernel@...r.kernel.org, kernel-team@...roid.com,
	iommu@...ts.linux.dev, Christoph Hellwig <hch@....de>,
	Marek Szyprowski <m.szyprowski@...sung.com>,
	Petr Tesarik <petr.tesarik1@...wei-partners.com>,
	Dexuan Cui <decui@...rosoft.com>
Subject: Re: [PATCH v2 1/3] swiotlb: Fix allocation alignment requirement
 when searching slots

On Thu, Feb 01, 2024 at 01:30:15PM +0000, Robin Murphy wrote:
> On 01/02/2024 12:46 pm, Will Deacon wrote:
> > On Wed, Jan 31, 2024 at 03:54:03PM +0000, Robin Murphy wrote:
> > > On 31/01/2024 12:25 pm, Will Deacon wrote:
> > > > Commit bbb73a103fbb ("swiotlb: fix a braino in the alignment check fix"),
> > > > which was a fix for commit 0eee5ae10256 ("swiotlb: fix slot alignment
> > > > checks"), causes a functional regression with vsock in a virtual machine
> > > > using bouncing via a restricted DMA SWIOTLB pool.
> > > > 
> > > > When virtio allocates the virtqueues for the vsock device using
> > > > dma_alloc_coherent(), the SWIOTLB search fails to take into account the
> > > > 8KiB buffer size and returns page-unaligned allocations if 'area->index'
> > > > was left unaligned by a previous allocation from the buffer:
> > > 
> > > Hmm, but isn't this fundamentally swiotlb_alloc()'s fault for assuming it's
> > > going to get a page-aligned address back despite asking for 0 alignment in
> > > the first place? I'm not sure SWIOTLB has ever promised implicit
> > > size-alignment, so it feels somewhat misplaced to be messing with the
> > > algorithm before fixing the obvious issue in the caller :/
> > 
> > It's hard to tell which guarantees are intentional here given that this
> > interface is all internal to swiotlb.c, but the 'alloc_align_mask'
> > parameter didn't even exist prior to e81e99bacc9f ("swiotlb: Support
> > aligned swiotlb buffers") and practically the implementation has ensured
> > page-aligned allocations for buffers >= PAGE_SIZE prior to 0eee5ae10256
> > ("swiotlb: fix slot alignment checks") by virtue of aligning the search
> > index to the stride.
> > 
> > In any case, this patch is required because the current state of
> > swiotlb_search_pool_area() conflates the DMA alignment mask, the
> > allocation alignment mask and the stride so that even if a non-zero
> > 'alloc_align_mask' is passed in, it won't necessarily be honoured.
> 
> Sure, I didn't mean to suggest there wasn't anything to fix here - if the
> existing code was intending to align to PAGE_SIZE even for a
> alloc_align_mask=0 and failing then that's clearly its own bug - I'm mostly
> being confused by the example of returning an unsuitably-aligned address for
> an 8KB dma_alloc_coherent() 75% of the time, if the end result of this fix
> is that we'll *still* return an incorrectly-aligned buffer for that same
> request 50% of the time (which just happens to be less fatal), since there
> are two separate bugs in that path.

I'll have a go at improving the commit message a bit, since I wrote that
before I'd really appreciated that we weren't returning natural alignment
(and page-alignment seems to be sufficient for whatever vsock needs).

Thanks,

Will