lkml.org 
[lkml]   [2008]   [Dec]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [mmotm 2008-12-22-16-14] NULL pointer dereference in dma_alloc_from_coherent().
On Tue, Dec 23, 2008 at 11:37:05PM -0800, Andrew Morton wrote:
> On Wed, 24 Dec 2008 15:34:06 +0900 Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote:
>
> > IP: [<c0159475>] dma_alloc_from_coherent+0x35/0xa0
>
> Thanks.

I hope you don't have that one already!

> dma-coherent-catch-oversized-requests-to-dma_alloc_from_coherent.patch does
>
> --- a/kernel/dma-coherent.c~dma-coherent-catch-oversized-requests-to-dma_alloc_from_coherent
> +++ a/kernel/dma-coherent.c
> @@ -112,6 +112,9 @@ int dma_alloc_from_coherent(struct devic
> struct dma_coherent_mem *mem = dev ? dev->dma_mem : NULL;
> int order = get_order(size);
>
> + if (unlikely(size > mem->size))
> + return 0;
> +
> if (mem) {
> int page = bitmap_find_free_region(mem->bitmap, mem->size,
> order);
>
> which can plainly oops if dev==NULL or if dev->dma_mem=NULL.
>
> That function is fairly stinky, so prior to altering it, let's clean it up:
>
> From: Andrew Morton <akpm@linux-foundation.org>
>
> This thing was rather stupidly coded. Rework it all prior to making
> changes.
>
> Also, rename local variable `page': kernel readers expect something called
> `page' to have type `struct page *'.
>
> Cc: Guennadi Liakhovetski <lg@denx.de>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Pekka Enberg <penberg@cs.helsinki.fi>
> Cc: Dmitry Baryshkov <dbaryshkov@gmail.com>
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Cc: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
>
> kernel/dma-coherent.c | 27 ++++++++++++++++-----------
> 1 file changed, 16 insertions(+), 11 deletions(-)
>
> diff -puN kernel/dma-coherent.c~dma_alloc_coherent-clean-it-up kernel/dma-coherent.c
> --- a/kernel/dma-coherent.c~dma_alloc_coherent-clean-it-up
> +++ a/kernel/dma-coherent.c
> @@ -109,20 +109,25 @@ EXPORT_SYMBOL(dma_mark_declared_memory_o
> int dma_alloc_from_coherent(struct device *dev, ssize_t size,
> dma_addr_t *dma_handle, void **ret)
> {
> - struct dma_coherent_mem *mem = dev ? dev->dma_mem : NULL;
> + struct dma_coherent_mem *mem;
> int order = get_order(size);
> + int pageno;
> +
> + if (!dev)
> + return 0;
> + mem = dev->dma_mem;
> + if (!mem)
> + return 0;
>
> - if (mem) {
> - int page = bitmap_find_free_region(mem->bitmap, mem->size,
> - order);
> - if (page >= 0) {
> - *dma_handle = mem->device_base + (page << PAGE_SHIFT);
> - *ret = mem->virt_base + (page << PAGE_SHIFT);
> - memset(*ret, 0, size);
> - } else if (mem->flags & DMA_MEMORY_EXCLUSIVE)
> - *ret = NULL;
> + pageno = bitmap_find_free_region(mem->bitmap, mem->size, order);
> + if (pageno >= 0) {
> + *dma_handle = mem->device_base + (pageno << PAGE_SHIFT);
> + *ret = mem->virt_base + (pageno << PAGE_SHIFT);
> + memset(*ret, 0, size);
> + } else if (mem->flags & DMA_MEMORY_EXCLUSIVE) {
> + *ret = NULL;
> }
> - return (mem != NULL);
> + return 1;
> }
> EXPORT_SYMBOL(dma_alloc_from_coherent);

Yep, looks much better.

> Then
> dma-coherent-catch-oversized-requests-to-dma_alloc_from_coherent.patch
> becomes:
>
> --- a/kernel/dma-coherent.c~dma-coherent-catch-oversized-requests-to-dma_alloc_from_coherent
> +++ a/kernel/dma-coherent.c
> @@ -118,6 +118,8 @@ int dma_alloc_from_coherent(struct devic
> mem = dev->dma_mem;
> if (!mem)
> return 0;
> + if (unlikely(size > mem->size))
> + return 0;
>
> pageno = bitmap_find_free_region(mem->bitmap, mem->size, order);
> if (pageno >= 0) {
> _
>

Great, thank you Andrew. Sorry for the explosion :/

Hannes


\
 
 \ /
  Last update: 2008-12-24 16:47    [from the cache]
©2003-2014 Jasper Spaans. Advertise on this site