Re: [PATCH] iommu-dma: Fix Oops on zero-sized DMA memory allocation

[Leon Romanovsky <leon@kernel.org>]

On Wed, Jan 15, 2025 at 02:32:07PM +0000, Robin Murphy wrote:
> [ BTW I apparently never received the original mail and don't see it on the
> list either... ]
> 
> On 2025-01-15 12:39 pm, Leon Romanovsky wrote:
> > On Wed, Jan 15, 2025 at 08:21:03PM +0800, hejiean@foxmail.com wrote:
> > > From: Jiean He <hejiean@foxmail.com>
> > > 
> > > In the `iommu_dma_ops.alloc` and `iommu_dma_ops.alloc_noncontiguous`
> > > interfaces, the `size` parameter is user-provided. When `size` is 0,
> > > an Oops occurs in `__iommu_dma_alloc_noncontiguous`, which is called
> > > by both interfaces.
> > > 
> > > The root cause lies in the fact that `__iommu_dma_alloc_noncontiguous`
> > > calls `kvcalloc`, The `count` parameter of `kvcalloc` is controlled by
> > > the user-provided `size`, and when `size` is 0, `count` is also 0.
> > > `__iommu_dma_alloc_noncontiguous` uses `if (!pages)` to check the
> > > return value of `kvcalloc`, but it returns `ZERO_SIZE_PTR` instead of
> > > `NULL` while `count==0`. This leads to a kernel Oops when `pages` is
> > > dereferenced in subsequent operations,as the following trace:
> > > 
> > > kernel NULL pointer dereference at virtual address 0000000000000010.
> > > Call trace:
> > > sg_alloc_append_table_from_pages+0x208/0x430
> 
> So... what you're saying is that there's a bug in
> sg_alloc_append_table_from_pages() where it can dereference more of "pages"
> than "n_pages" says is valid?
> 
> Of course that's not to say that's the *only* issue that this scenario could
> or would run into - indeed many allocation paths in various DMA API
> implementations depend on get_order(size), and the kerneldoc explicitly
> states that get_order(0) is undefined. So if you really think it's worth
> formally accommodating zero-sized DMA API requests, rather than leaving them
> as a nonsensical thing that drivers should obviously not do in the first
> place, then iommu-dma is still not the logically appropriate place to do
> that.

I got his dump stack now and it is a bug in his testing module where he
is getting size from users and passing to DMA API as is.

Thanks

> 
> Thanks,
> Robin.
> 
> > > sg_alloc_table_from_pages_segment+0x3c/0xa0
> > > __iommu_dma_alloc_noncontiguous.constprop.0+0x130/0x278
> > 
> > Who is this user?
> > grep sg_alloc_append_table_from_pages shows only 4 users:
> > drivers/infiniband/core/umem.c
> > drivers/vfio/pci/mlx5/cmd.c
> > drivers/vfio/pci/virtio/migrate.c
> > lib/scatterlist.c
> > 
> > First 3 users have constant size and it is not 0.
> > The latter is used in wrapper sg_alloc_table_from_pages_segment(), which
> > is used by DRM and that layer already was supposed to catch nr_pages == 0.
> > 
> > Thanks
> > 
> > > 
> > > This commit addresses the issue by adding a check for the `size`
> > > parameter in `__iommu_dma_alloc_noncontiguous`. If 0 is passed as `size`,
> > > a warning is reported and `NULL` is returned. This approach clearly
> > > indicates that the user-provided size might be illegal, helping users
> > > adjust their programs. In contrast, using `ZERO_OR_NULL_PTR` to check the
> > > return value of `kmalloc` (used by `kvcalloc` internally) does not
> > > distinguish between insufficient memory and a zero-sized memory request.
> > > 
> > > Fixes: 0db2e5d18f76 ("iommu: Implement common IOMMU ops for DMA mapping")
> > > 
> > > Signed-off-by: Jiean He <hejiean@foxmail.com>
> > > ---
> > >   drivers/iommu/dma-iommu.c | 5 +++++
> > >   1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> > > index 2a9fa0c8cc00..3c9c905e7a8f 100644
> > > --- a/drivers/iommu/dma-iommu.c
> > > +++ b/drivers/iommu/dma-iommu.c
> > > @@ -956,6 +956,11 @@ static struct page **__iommu_dma_alloc_noncontiguous(struct device *dev,
> > >   	    iommu_deferred_attach(dev, domain))
> > >   		return NULL;
> > > +	if (unlikely(size == 0)) {
> > > +		dev_err(dev, "DMA alloc Zero-Sized memory is unsupported\n");
> > > +		return NULL;
> > > +	}
> > > +
> > >   	min_size = alloc_sizes & -alloc_sizes;
> > >   	if (min_size < PAGE_SIZE) {
> > >   		min_size = PAGE_SIZE;
> > > -- 
> > > 2.40.0.windows.1
> > > 
>