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

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

New issues:
- [Critical] Mismatch between `dmabuf->size` alignment check and `niov_shift` allocation causes zero-sized allocations and heap out-of-bounds writes.
- [High] Arithmetic right shift of a negative `ssize_t` causes out-of-bounds array access for dmabuf chunks larger than 2GB.

Pre-existing issues:
- [High] Truncation of 64-bit `dma_addr_t` to 32-bit `unsigned long` corrupts chunk address tracking, causing memory leaks.
--

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

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

Every devmem dmabuf binding today hands the page_pool PAGE_SIZE niovs.
This caps a single RX descriptor at PAGE_SIZE, burning CPU on buffer
churn for large flows.

Add a bind-time netlink attribute, NETDEV_A_DMABUF_RX_PAGE_SIZE, that
lets userspace request a larger niov size. The value must be a power of
two >= PAGE_SIZE.

> diff --git a/net/core/devmem.c b/net/core/devmem.c
> --- a/net/core/devmem.c
> +++ b/net/core/devmem.c

[ ... ]

> @@ -93,13 +93,14 @@ net_devmem_alloc_dmabuf(struct net_devmem_dmabuf_binding *binding)
>  	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;
>  
>  	offset = dma_addr - owner->base_dma_addr;
> -	index = offset / PAGE_SIZE;
> +	index = offset >> binding->niov_shift;

[Severity: High]
Will this right shift perform an arithmetic shift when offset is negative?

In net_devmem_alloc_dmabuf(), offset is declared as ssize_t (a 32-bit signed
integer on 32-bit architectures). For a DMA chunk larger than 2GB, the
difference between dma_addr and owner->base_dma_addr will overflow into a
negative value.

Previously, offset / PAGE_SIZE would implicitly promote offset to unsigned
long because PAGE_SIZE is unsigned, yielding the correct positive index. By
changing this to a right shift by binding->niov_shift, the result of the
shift retains the signed type of the left operand (ssize_t). Right-shifting
a negative signed value performs sign extension, resulting in a negative
index.

Could this lead to an out-of-bounds access on owner->area.niovs[index]?

>  	niov = &owner->area.niovs[index];
>  
>  	niov->desc.pp_magic = 0;

[ ... ]

> @@ -113,12 +114,13 @@ void net_devmem_free_dmabuf(struct net_iov *niov)
>  {
>  	struct net_devmem_dmabuf_binding *binding = net_devmem_iov_binding(niov);
>  	unsigned long dma_addr = net_devmem_get_dma_addr(niov);
> +	size_t niov_size = 1UL << binding->niov_shift;
>  
>  	if (WARN_ON(!gen_pool_has_addr(binding->chunk_pool, dma_addr,
> -				       PAGE_SIZE)))
> +				       niov_size)))

[Severity: High]
This is a pre-existing issue, but can the 64-bit dma_addr be truncated here
on 32-bit systems?

In net_devmem_free_dmabuf(), the 64-bit return value from
net_devmem_get_dma_addr() is assigned to an unsigned long dma_addr and passed
to gen_pool_has_addr().

A similar truncation occurs in net_devmem_bind_dmabuf() where
sg_dma_address(sg) is passed to the unsigned long virt parameter of
gen_pool_add_owner().

On 32-bit systems with PAE/LPAE, dma_addr_t is 64-bit. If the original 64-bit
address plus the chunk size crosses a 4GB boundary, the truncated 32-bit
end_addr in gen_pool wraps around, breaking the bounds check in
gen_pool_has_addr(). This causes the teardown path to trigger the WARN_ON
and abort without freeing the chunk.

Does this cause a permanent memory leak?

>  		return;
>  
> -	gen_pool_free(binding->chunk_pool, dma_addr, PAGE_SIZE);
> +	gen_pool_free(binding->chunk_pool, dma_addr, niov_size);
>  }

[ ... ]

> @@ -248,18 +256,14 @@ net_devmem_bind_dmabuf(struct net_device *dev, void *vdev,
>  			goto err_unmap;
>  		}
>  		binding->tx_vec = kvmalloc_objs(struct net_iov *,
> -						dmabuf->size / PAGE_SIZE);
> +						dmabuf->size >> niov_shift);

[Severity: Critical]
Can tx_vec be allocated as a zero-sized array here?

In net_devmem_bind_dmabuf(), the alignment check prior to this allocation
still verifies that dmabuf->size is a multiple of PAGE_SIZE rather than
niov_size:

	if (!IS_ALIGNED(dmabuf->size, PAGE_SIZE)) {
		err = -EINVAL;
		NL_SET_ERR_MSG(extack, "TX dma-buf size must be a multiple of PAGE_SIZE");
		goto err_unmap;
	}

If userspace binds a dmabuf where PAGE_SIZE <= dmabuf->size < niov_size (for
example, 4K size with 64K niov_size), this check will pass, but the tx_vec
allocation (dmabuf->size >> niov_shift) will allocate 0 elements.

Later, if the exporter padded the scatterlist segment to niov_size, the
scatterlist parsing loop computes owner->area.num_niovs = 1 and writes to
tx_vec[0].

Could this result in a heap out-of-bounds write?

>  		if (!binding->tx_vec) {
>  			err = -ENOMEM;
>  			goto err_unmap;
>  		}
>  	}

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