[PATCH 1/1] swiotlb: fix out-of-bounds TLB allocations with CONFIG_SWIOTLB_DYNAMIC

[PATCH 1/1] swiotlb: fix out-of-bounds TLB allocations with CONFIG_SWIOTLB_DYNAMIC

[Petr Tesarik]

From: Petr Tesarik <petr.tesarik1@huawei-partners.com>

Limit the free list length to the size of the IO TLB. Transient pool can be
smaller than IO_TLB_SEGSIZE, but the free list is initialized with the
assumption that the total number of slots is a multiple of IO_TLB_SEGSIZE.
As a result, swiotlb_area_find_slots() may allocate slots past the end of
a transient IO TLB buffer.

Reported-by: Niklas Schnelle <schnelle@linux.ibm.com>
Closes: https://lore.kernel.org/linux-iommu/104a8c8fedffd1ff8a2890983e2ec1c26bff6810.camel@linux.ibm.com/
Fixes: 79636caad361 ("swiotlb: if swiotlb is full, fall back to a transient memory pool")
Cc: Halil Pasic <pasic@linux.ibm.com>
Cc: stable@vger.kernel.org
Signed-off-by: Petr Tesarik <petr.tesarik1@huawei-partners.com>
---
 kernel/dma/swiotlb.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
index 26202274784f..ec82524ba902 100644
--- a/kernel/dma/swiotlb.c
+++ b/kernel/dma/swiotlb.c
@@ -283,7 +283,8 @@ static void swiotlb_init_io_tlb_pool(struct io_tlb_pool *mem, phys_addr_t start,
 	}
 
 	for (i = 0; i < mem->nslabs; i++) {
-		mem->slots[i].list = IO_TLB_SEGSIZE - io_tlb_offset(i);
+		mem->slots[i].list = min(IO_TLB_SEGSIZE - io_tlb_offset(i),
+					 mem->nslabs - i);
 		mem->slots[i].orig_addr = INVALID_PHYS_ADDR;
 		mem->slots[i].alloc_size = 0;
 	}
-- 
2.42.1

Re: [PATCH 1/1] swiotlb: fix out-of-bounds TLB allocations with CONFIG_SWIOTLB_DYNAMIC

[Petr Tesařík]

On Wed,  8 Nov 2023 12:12:49 +0100
Petr Tesarik <petrtesarik@huaweicloud.com> wrote:

> From: Petr Tesarik <petr.tesarik1@huawei-partners.com>
> 
> Limit the free list length to the size of the IO TLB. Transient pool can be
> smaller than IO_TLB_SEGSIZE, but the free list is initialized with the
> assumption that the total number of slots is a multiple of IO_TLB_SEGSIZE.
> As a result, swiotlb_area_find_slots() may allocate slots past the end of
> a transient IO TLB buffer.

Just to make it clear, this patch addresses only the memory corruption
reported by Niklas, without addressing the underlying issues. Where
corruption happened before, allocations will fail with this patch.

I am still looking into improving the allocation strategy itself.

Petr T

> Reported-by: Niklas Schnelle <schnelle@linux.ibm.com>
> Closes: https://lore.kernel.org/linux-iommu/104a8c8fedffd1ff8a2890983e2ec1c26bff6810.camel@linux.ibm.com/
> Fixes: 79636caad361 ("swiotlb: if swiotlb is full, fall back to a transient memory pool")
> Cc: Halil Pasic <pasic@linux.ibm.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Petr Tesarik <petr.tesarik1@huawei-partners.com>
> ---
>  kernel/dma/swiotlb.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> index 26202274784f..ec82524ba902 100644
> --- a/kernel/dma/swiotlb.c
> +++ b/kernel/dma/swiotlb.c
> @@ -283,7 +283,8 @@ static void swiotlb_init_io_tlb_pool(struct io_tlb_pool *mem, phys_addr_t start,
>  	}
>  
>  	for (i = 0; i < mem->nslabs; i++) {
> -		mem->slots[i].list = IO_TLB_SEGSIZE - io_tlb_offset(i);
> +		mem->slots[i].list = min(IO_TLB_SEGSIZE - io_tlb_offset(i),
> +					 mem->nslabs - i);
>  		mem->slots[i].orig_addr = INVALID_PHYS_ADDR;
>  		mem->slots[i].alloc_size = 0;
>  	}

Re: [PATCH 1/1] swiotlb: fix out-of-bounds TLB allocations with CONFIG_SWIOTLB_DYNAMIC

[Christoph Hellwig]

On Wed, Nov 08, 2023 at 01:21:20PM +0100, Petr Tesařík wrote:
> On Wed,  8 Nov 2023 12:12:49 +0100
> Petr Tesarik <petrtesarik@huaweicloud.com> wrote:
> 
> > From: Petr Tesarik <petr.tesarik1@huawei-partners.com>
> > 
> > Limit the free list length to the size of the IO TLB. Transient pool can be
> > smaller than IO_TLB_SEGSIZE, but the free list is initialized with the
> > assumption that the total number of slots is a multiple of IO_TLB_SEGSIZE.
> > As a result, swiotlb_area_find_slots() may allocate slots past the end of
> > a transient IO TLB buffer.
> 
> Just to make it clear, this patch addresses only the memory corruption
> reported by Niklas, without addressing the underlying issues. Where
> corruption happened before, allocations will fail with this patch.

Thanks, I've applied so that we can get it into -rc1.

Re: [PATCH 1/1] swiotlb: fix out-of-bounds TLB allocations with CONFIG_SWIOTLB_DYNAMIC

[Niklas Schnelle]

On Wed, 2023-11-08 at 13:21 +0100, Petr Tesařík wrote:
> On Wed,  8 Nov 2023 12:12:49 +0100
> Petr Tesarik <petrtesarik@huaweicloud.com> wrote:
> 
> > From: Petr Tesarik <petr.tesarik1@huawei-partners.com>
> > 
> > Limit the free list length to the size of the IO TLB. Transient pool can be
> > smaller than IO_TLB_SEGSIZE, but the free list is initialized with the
> > assumption that the total number of slots is a multiple of IO_TLB_SEGSIZE.
> > As a result, swiotlb_area_find_slots() may allocate slots past the end of
> > a transient IO TLB buffer.
> 
> Just to make it clear, this patch addresses only the memory corruption
> reported by Niklas, without addressing the underlying issues. Where
> corruption happened before, allocations will fail with this patch.
> 
> I am still looking into improving the allocation strategy itself.
> 
> Petr T

I know this has already been applied but for what its worth I did
finally manage to test this with my reproducer and the allocation
overrun is fixed by this change. I also confirmed that at least my
ConnectX VF TCP/IP test case seems to handle the DMA error gracefully
enough.

Thanks,
Niklas

Re: [PATCH 1/1] swiotlb: fix out-of-bounds TLB allocations with CONFIG_SWIOTLB_DYNAMIC

[Petr Tesařík]

On Thu, 09 Nov 2023 13:24:48 +0100
Niklas Schnelle <schnelle@linux.ibm.com> wrote:

> On Wed, 2023-11-08 at 13:21 +0100, Petr Tesařík wrote:
> > On Wed,  8 Nov 2023 12:12:49 +0100
> > Petr Tesarik <petrtesarik@huaweicloud.com> wrote:
> >   
> > > From: Petr Tesarik <petr.tesarik1@huawei-partners.com>
> > > 
> > > Limit the free list length to the size of the IO TLB. Transient pool can be
> > > smaller than IO_TLB_SEGSIZE, but the free list is initialized with the
> > > assumption that the total number of slots is a multiple of IO_TLB_SEGSIZE.
> > > As a result, swiotlb_area_find_slots() may allocate slots past the end of
> > > a transient IO TLB buffer.  
> > 
> > Just to make it clear, this patch addresses only the memory corruption
> > reported by Niklas, without addressing the underlying issues. Where
> > corruption happened before, allocations will fail with this patch.
> > 
> > I am still looking into improving the allocation strategy itself.
> > 
> > Petr T  
> 
> I know this has already been applied but for what its worth I did
> finally manage to test this with my reproducer and the allocation
> overrun is fixed by this change. I also confirmed that at least my
> ConnectX VF TCP/IP test case seems to handle the DMA error gracefully
> enough.

Thank you for testing!

Inded, the failed request is often retried at a later time. For example
I tested with a SCSI driver, and by the time the SCSI layer retried the
request, a new standard pool was already available. But this situation
is not ideal. If nothing else, it incurs an unnecessary delay.

Petr T

Re: [PATCH 1/1] swiotlb: fix out-of-bounds TLB allocations with CONFIG_SWIOTLB_DYNAMIC

[Halil Pasic]

On Wed,  8 Nov 2023 12:12:49 +0100
Petr Tesarik <petrtesarik@huaweicloud.com> wrote:

> From: Petr Tesarik <petr.tesarik1@huawei-partners.com>
> 
> Limit the free list length to the size of the IO TLB. Transient pool can be
> smaller than IO_TLB_SEGSIZE, but the free list is initialized with the
> assumption that the total number of slots is a multiple of IO_TLB_SEGSIZE.
> As a result, swiotlb_area_find_slots() may allocate slots past the end of
> a transient IO TLB buffer.
> 
> Reported-by: Niklas Schnelle <schnelle@linux.ibm.com>
> Closes: https://lore.kernel.org/linux-iommu/104a8c8fedffd1ff8a2890983e2ec1c26bff6810.camel@linux.ibm.com/
> Fixes: 79636caad361 ("swiotlb: if swiotlb is full, fall back to a transient memory pool")
> Cc: Halil Pasic <pasic@linux.ibm.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Petr Tesarik <petr.tesarik1@huawei-partners.com>

Reviewed-by: Halil Pasic <pasic@linux.ibm.com>

I believe there is at least one stale comment in the code that says
"The number of slot in an area should be a multiple of IO_TLB_SEGSIZE"
but that is probably not stable material, so I do agree with keeping this
patch minimal.

Regards,
Halil