Re: [PATCH] cma: use unsigned type for count argument

[Michal Nazarewicz <mpn@google.com>]

[-- Attachment #1: Type: text/plain, Size: 4221 bytes --]

On Fri, Dec 21 2012, David Rientjes wrote:
> > Specifying negative size of buffer makes no sense and thus this commit
> > changes the type of the count argument to unsigned.
> > 
> > --- a/arch/arm/mm/dma-mapping.c
> > +++ b/arch/arm/mm/dma-mapping.c
> > @@ -1038,9 +1038,9 @@ static struct page **__iommu_alloc_buffer(struct device *dev, size_t size,
> >  					  gfp_t gfp, struct dma_attrs *attrs)
> >  {
> >  	struct page **pages;
> > -	int count = size >> PAGE_SHIFT;
> > -	int array_size = count * sizeof(struct page *);
> > -	int i = 0;
> > +	unsigned int count = size >> PAGE_SHIFT;
> > +	unsigned int array_size = count * sizeof(struct page *);
> > +	unsigned int i = 0;

> I didn't ack this because there's no bounds checking on 
> dma_alloc_from_contiguous() and bitmap_set() has a dangerous side-effect 
> when called with an overflowed nr since it takes a signed argument.  

Mystery solved.  I recalled that there was some reason why the count is
specified as a signed int and thought bitmap_find_next_zero_area() was
the culprit, but now it seems that bitmap_set() was the reason.

> Marek, is there some sane upper bound we can put on count?

INT_MAX would be sufficient.  After all, it maps to a 8 TiB buffer (if
page is 4 KiB).

Moreover, in reality, the few places that call
dma_alloc_from_contiguous() pass a value that cannot be higher than
INT_MAX, ie. (listings heavily stripped):

arch/arm/mm/dma-mapping.c-static void *__alloc_from_contiguous(struct device *dev, size_t size,
arch/arm/mm/dma-mapping.c-                                   pgprot_t prot, struct page **ret_page)
arch/arm/mm/dma-mapping.c-{
arch/arm/mm/dma-mapping.c-      size_t count = size >> PAGE_SHIFT;
arch/arm/mm/dma-mapping.c:      page = dma_alloc_from_contiguous(dev, count, order);
arch/arm/mm/dma-mapping.c-}

arch/arm/mm/dma-mapping.c-static void *__alloc_from_contiguous(struct device *dev, size_t size,
arch/arm/mm/dma-mapping.c-                                   pgprot_t prot, struct page **ret_page)
arch/arm/mm/dma-mapping.c-{
arch/arm/mm/dma-mapping.c-      size_t count = size >> PAGE_SHIFT;
arch/arm/mm/dma-mapping.c:      page = dma_alloc_from_contiguous(dev, count, order);
arch/arm/mm/dma-mapping.c-}

arch/arm/mm/dma-mapping.c-static struct page **__iommu_alloc_buffer(struct device *dev, size_t size,
arch/arm/mm/dma-mapping.c-                                        gfp_t gfp, struct dma_attrs *attrs)
arch/arm/mm/dma-mapping.c-{
arch/arm/mm/dma-mapping.c-      unsigned int count = size >> PAGE_SHIFT;
arch/arm/mm/dma-mapping.c-      if (dma_get_attr(DMA_ATTR_FORCE_CONTIGUOUS, attrs)) {
arch/arm/mm/dma-mapping.c:              page = dma_alloc_from_contiguous(dev, count, order);
arch/arm/mm/dma-mapping.c-      }
arch/arm/mm/dma-mapping.c-}

arch/x86/kernel/pci-dma.c-void *dma_generic_alloc_coherent(struct device *dev, size_t size,
arch/x86/kernel/pci-dma.c-                               dma_addr_t *dma_addr, gfp_t flag,
arch/x86/kernel/pci-dma.c-                               struct dma_attrs *attrs)
arch/x86/kernel/pci-dma.c-{
arch/x86/kernel/pci-dma.c-      unsigned int count = PAGE_ALIGN(size) >> PAGE_SHIFT;
arch/x86/kernel/pci-dma.c-      if (!(flag & GFP_ATOMIC))
arch/x86/kernel/pci-dma.c:              page = dma_alloc_from_contiguous(dev, count, get_order(size));
arch/x86/kernel/pci-dma.c-}

So I think just adding the following, should be sufficient to make
everyone happy:

diff --git a/drivers/base/dma-contiguous.c b/drivers/base/dma-contiguous.c
index e34e3e0..e91743b 100644
--- a/drivers/base/dma-contiguous.c
+++ b/drivers/base/dma-contiguous.c
@@ -320,7 +320,7 @@ struct page *dma_alloc_from_contiguous(struct device *dev, unsigned int count,
 	pr_debug("%s(cma %p, count %u, align %u)\n", __func__, (void *)cma,
 		 count, align);
 
-	if (!count)
+	if (!count || count > INT_MAX)
 		return NULL;
 
 	mask = (1 << align) - 1;

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +----<email/xmpp: mpn@google.com>--------------ooO--(_)--Ooo--

[-- Attachment #2.1: Type: text/plain, Size: 0 bytes --]



[-- Attachment #2.2: Type: application/pgp-signature, Size: 835 bytes --]