linux-kernel - RE: [PATCH v6 4/6] swiotlb: Fix alignment checks when both allocation and DMA masks are present

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] [thread-next>] [day] [month] [year] [list]

Date: Mon, 11 Mar 2024 21:36:10 +0000
From: Michael Kelley <mhklinux@...look.com>
To: Petr Tesařík <petr@...arici.cz>, Will Deacon
	<will@...nel.org>, Nicolin Chen <nicolinc@...dia.com>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"kernel-team@...roid.com" <kernel-team@...roid.com>, "iommu@...ts.linux.dev"
	<iommu@...ts.linux.dev>, Christoph Hellwig <hch@....de>, Marek Szyprowski
	<m.szyprowski@...sung.com>, Robin Murphy <robin.murphy@....com>, Petr Tesarik
	<petr.tesarik1@...wei-partners.com>, Dexuan Cui <decui@...rosoft.com>
Subject: RE: [PATCH v6 4/6] swiotlb: Fix alignment checks when both allocation
 and DMA masks are present

From: Petr Tesařík <petr@...arici.cz>
> On Fri,  8 Mar 2024 15:28:27 +0000
> Will Deacon <will@...nel.org> wrote:

[snip]

> >
> > diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> > index c20324fba814..c381a7ed718f 100644
> > --- a/kernel/dma/swiotlb.c
> > +++ b/kernel/dma/swiotlb.c
> > @@ -981,8 +981,7 @@ static int swiotlb_search_pool_area(struct device *dev, struct io_tlb_pool *pool
> >  	dma_addr_t tbl_dma_addr =
> >  		phys_to_dma_unencrypted(dev, pool->start) & boundary_mask;
> >  	unsigned long max_slots = get_max_slots(boundary_mask);
> > -	unsigned int iotlb_align_mask =
> > -		dma_get_min_align_mask(dev) & ~(IO_TLB_SIZE - 1);
> > +	unsigned int iotlb_align_mask = dma_get_min_align_mask(dev);
> >  	unsigned int nslots = nr_slots(alloc_size), stride;
> >  	unsigned int offset = swiotlb_align_offset(dev, orig_addr);
> >  	unsigned int index, slots_checked, count = 0, i;
> > @@ -993,6 +992,14 @@ static int swiotlb_search_pool_area(struct device *dev, struct io_tlb_pool *pool
> >  	BUG_ON(!nslots);
> >  	BUG_ON(area_index >= pool->nareas);
> >
> > +	/*
> > +	 * Ensure that the allocation is at least slot-aligned and update
> > +	 * 'iotlb_align_mask' to ignore bits that will be preserved when
> > +	 * offsetting into the allocation.
> > +	 */
> > +	alloc_align_mask |= (IO_TLB_SIZE - 1);
> > +	iotlb_align_mask &= ~alloc_align_mask;
> > +
> 
> I have started writing the KUnit test suite, and the results look
> incorrect to me for this case.
> 
> I'm calling swiotlb_tbl_map_single() with:
> 
> * alloc_align_mask = 0xfff
> * a device with min_align_mask = 0xfff
> * the 12 lowest bits of orig_addr are 0xfa0
> 
> The min_align_mask becomes zero after the masking added by this patch,
> and the 12 lowest bits of the returned address are 0x7a0, i.e. not
> equal to 0xfa0.

The address returned by swiotlb_tbl_map_single() is the slot index
converted to an address, plus the offset modulo the min_align_mask for
the device.  The local variable "offset" in swiotlb_tbl_map_single()
should be 0xfa0.  The slot index should be an even number to meet
the alloc_align_mask requirement.  And the pool->start address should
be at least page aligned, producing a page-aligned address *before* the
offset is added. Can you debug which of these isn't true for the case
you are seeing?

> 
> In other words, the min_align_mask constraint is not honored. Of course,
> given the above values, it is not possible to honor both min_align_mask
> and alloc_align_mask. 

When orig_addr is specified and min_align_mask is set, alloc_align_mask
governs the address of the _allocated_ space, which is not necessarily the
returned physical address.  The min_align_mask may dictate some
pre-padding of size "offset" within the allocated space, and the returned
address is *after* that pre-padding.  In this way, both can be honored.

> I find it somewhat surprising that NVMe does not
> in fact require that the NVME_CTRL_PAGE_SHIFT low bits are preserved,
> as suggested by Nicolin's successful testing.
> 
> Why is that?

I saw only one stack trace from Nicolin, and it was file system buffer
flushing code that initiated the I/O.  In such cases, it's very likely that the
original address is at least 4K aligned.  Hence the offset is zero and
the low bits will typically be correct.

> 
> Does IOMMU do some additional post-processing of the bounce buffer
> address to restore the value of bit 11?

Not that I can see.

> 
> Or is this bit always zero in all real-world scenarios?

For file system initiated I/Os, probably yes.  But for raw disk I/Os
initiated from user space, not necessarily.

Michael