Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [Critical] Integer overflow and signedness bug leading to a negative array index and out-of-bounds memory corruption on 32-bit architectures. Pre-existing issues: - [High] Sleeping in atomic context due to kvfree() inside an RCU read-side critical section. -- commit b8e27f9eee44549dc6869beb99adddc8150160f6 Author: Bobby Eshleman <[email protected]> net: devmem: allow rx-buf-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_BUF_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 > index 957d6b96216b3..3ce3cc14bec07 100644 > --- 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: Critical] Does this bitwise shift cause a negative array index on 32-bit architectures? Since a dma-buf scatterlist entry size (len) can be up to 4GB-1, the offset within a chunk can be >= 2GB. On 32-bit architectures, ssize_t is a 32-bit signed integer, so an offset >= 2GB overflows and becomes negative. In the original code, the division promoted offset to an unsigned long because PAGE_SIZE is unsigned, resulting in a correct positive index: index = offset / PAGE_SIZE; However, the type of a bitwise shift result is determined solely by the promoted left operand. By changing this to a bitwise right shift: index = offset >> binding->niov_shift; the result remains signed. An arithmetic right shift of a negative number produces a negative index, which can lead to an out-of-bounds write on owner->area.niovs[index] later in the function. > niov = &owner->area.niovs[index]; > > niov->desc.pp_magic = 0; [ ... ] > @@ -273,9 +277,12 @@ net_devmem_bind_dmabuf(struct net_device *dev, void > *vdev, > size_t len = sg_dma_len(sg); > struct net_iov *niov; > > - if (!IS_ALIGNED(len, PAGE_SIZE)) { > + if (!IS_ALIGNED(dma_addr, niov_size) || > + !IS_ALIGNED(len, niov_size)) { > err = -EINVAL; > - NL_SET_ERR_MSG(extack, "dma-buf SG length must be > PAGE_SIZE aligned"); > + NL_SET_ERR_MSG_FMT(extack, > + "dmabuf sg entry (addr=%pad, > len=%zu) not aligned to niov size %zu", > + &dma_addr, len, niov_size); > goto err_free_chunks; [Severity: High] This is a pre-existing issue, but can this error path lead to a panic by sleeping in atomic context? When goto err_free_chunks is taken, the error path handles cleanup using gen_pool_for_each_chunk(), which iterates over chunks with rcu_read_lock() held: lib/genalloc.c:gen_pool_for_each_chunk() { rcu_read_lock(); list_for_each_entry_rcu(chunk, &(pool)->chunks, next_chunk) func(pool, chunk, data); rcu_read_unlock(); } The callback net_devmem_dmabuf_free_chunk_owner() then frees the memory: net/core/devmem.c:net_devmem_dmabuf_free_chunk_owner() { ... kvfree(owner->area.niovs); ... } If the allocated array is large enough to use the vmalloc fallback, kvfree() internally invokes vfree(). Since vfree() is not allowed in atomic context and calls might_sleep(), won't this trigger a 'scheduling while atomic' bug when the RCU read-side critical section is held? > } > [ ... ] -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
