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@xxxxxxxxxx>--------------ooO--(_)--Ooo--Attachment:
pgpAjXRUK1Y0f.pgp
Description: PGP signature