Re: [PATCH net-next v8 1/3] net: devmem: allow rx-page-size > PAGE_SIZE per dmabuf binding

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Truncation of 64-bit DMA addresses to 32-bit unsigned long causes allocator state and memory corruption.
- [High] Valid DMA address 0 is treated as an allocation failure, causing a permanent resource leak and kernel panic.
--

--- Patch [1]: [PATCH net-next v8 1/3] net: devmem: allow rx-page-size > PAGE_SIZE per dmabuf binding ---
commit aedd523e1f497a9daaf91448a092f69629705696
Author: Bobby Eshleman <[email protected]>

net: devmem: allow rx-page-size > PAGE_SIZE per dmabuf binding

This commit introduces a netlink attribute to allow setting a larger
dmabuf rx-page-size for the page_pool. The value must be a power of two
and greater than or equal to PAGE_SIZE.

> diff --git a/net/core/devmem.c b/net/core/devmem.c
> index 957d6b96216b3..f4d60654ce7fd 100644
> --- a/net/core/devmem.c
> +++ b/net/core/devmem.c
[ ... ]
> @@ -87,13 +93,14 @@ struct net_iov *
>  net_devmem_alloc_dmabuf(struct net_devmem_dmabuf_binding *binding)
>  {
>  	struct dmabuf_genpool_chunk_owner *owner;
>  	unsigned long dma_addr;
>  	struct net_iov *niov;
>  	ssize_t offset;
>  	ssize_t index;
>  
> -	dma_addr = gen_pool_alloc_owner(binding->chunk_pool, PAGE_SIZE,
> +	dma_addr = gen_pool_alloc_owner(binding->chunk_pool,
> +					1UL << binding->niov_shift,
>  					(void **)&owner);
>  	if (!dma_addr)
>  		return NULL;

[Severity: High]
This isn't a bug introduced by this patch, but does this check incorrectly
handle a valid allocation when the dmabuf is mapped at DMA address 0?

Since gen_pool_alloc_owner() can return 0 on success if the chunk starts at
virtual address 0, checking for !dma_addr might treat a valid DMA address 0
as an allocation failure.

This could cause the allocation to leak in the pool without a tracking net_iov.
Later, when the dmabuf is unbound, gen_pool_destroy() will check that all
chunks are free and could hit a BUG_ON if this allocation was leaked.

>  
>  	offset = dma_addr - owner->base_dma_addr;
> -	index = offset / PAGE_SIZE;
> +	index = offset >> binding->niov_shift;
>  	niov = &owner->area.niovs[index];

[Severity: High]
This is a pre-existing issue, but can this lead to an out-of-bounds array
access on 32-bit platforms with 64-bit DMA addresses (e.g., LPAE/PAE)?

Since dma_addr is derived from gen_pool_alloc_owner(), which returns an
unsigned long, it will be a 32-bit value on 32-bit systems.

If base_dma_addr is 64-bit and the actual address is above 4GB, the truncated
dma_addr subtracted from the 64-bit base_dma_addr would result in a massive
underflow.

Could this huge offset result in indexing out of bounds when initializing
niovs[index]?

[ ... ]
> @@ -288,7 +295,7 @@ net_devmem_bind_dmabuf(struct net_device *dev, void *vdev,
>  
>  		owner->area.base_virtual = virtual;
>  		owner->base_dma_addr = dma_addr;
> -		owner->area.num_niovs = len / PAGE_SIZE;
> +		owner->area.num_niovs = len >> niov_shift;
>  		owner->binding = binding;
>  
>  		err = gen_pool_add_owner(binding->chunk_pool, dma_addr,
>  					 dma_addr, len, dev_to_node(&dev->dev),
>  					 owner);

[Severity: High]
This isn't a bug introduced by this patch, but does calling
gen_pool_add_owner() silently truncate the 64-bit DMA address on 32-bit
systems?

The gen_pool_add_owner() function takes an unsigned long for the virt
argument. When passing dma_addr_t dma_addr on 32-bit architectures, this
might truncate the upper 32 bits of the DMA address.

If a DMA address exceeds 4GB, could this truncation cause overlapping virtual
address ranges in the gen_pool?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1