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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <xa1tip7u14tq.fsf@mina86.com>
Date:	Sat, 22 Dec 2012 16:31:29 +0100
From:	Michal Nazarewicz <mpn@...gle.com>
To:	David Rientjes <rientjes@...gle.com>,
	Andrew Morton <akpm@...ux-foundation.org>
Cc:	Marek Szyprowski <m.szyprowski@...sung.com>, linux-mm@...ck.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] cma: use unsigned type for count argument

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@...gle.com>--------------ooO--(_)--Ooo--

Content of type "application/pgp-signature" skipped