[mmotm 2008-12-22-16-14] NULL pointer dereference in dma_alloc_from_coherent().

[mmotm 2008-12-22-16-14] NULL pointer dereference in dma_alloc_from_coherent().

[Tetsuo Handa]

Hello.

I'm using Debian Sarge on VMware Workstation 6.5.1 .

Kernel config is at http://I-love.SAKURA.ne.jp/tmp/config-2.6.28-rc9-mm1

Regards.
----------
Linux version 2.6.28-rc9-mm1 (root@tomoyo) (gcc version 3.3.5 (Debian 1:3.3.5-13)) #2 SMP PREEMPT Wed Dec 24 15:00:36 JST 2008
(.....snipped.....)
initrd-tools: 0.1.81.1
mount: unknown filesystem type 'devfs'
NET: Registered protocol family 1
SCSI subsystem initialized
pci 0000:00:10.0: PCI INT A -> GSI 17 (level, low) -> IRQ 17
scsi: ***** BusLogic SCSI Driver Version 2.1.16 of 18 July 2002 *****
scsi: Copyright 1995-1998 by Leonard N. Zubkoff <lnz@dandelion.com>
scsi0: Configuring BusLogic Model BT-958 PCI Wide Ultra SCSI Host Adapter
scsi0:   Firmware Version: 5.07B, I/O Address: 0x10C0, IRQ Channel: 17/Level
scsi0:   PCI Bus: 0, Device: 16, Address: 0xD8800000, Host Adapter SCSI ID: 7
scsi0:   Parity Checking: Enabled, Extended Translation: Enabled
scsi0:   Synchronous Negotiation: Ultra, Wide Negotiation: Enabled
scsi0:   Disconnect/Reconnect: Enabled, Tagged Queuing: Enabled
scsi0:   Scatter/Gather Limit: 128 of 8192 segments, Mailboxes: 211
scsi0:   Driver Queue Depth: 211, Host Adapter Queue Depth: 192
scsi0:   Tagged Queue Depth: Automatic, Untagged Queue Depth: 3
BUG: unable to handle kernel NULL pointer dereference at 00000008
IP: [<c0159475>] dma_alloc_from_coherent+0x35/0xa0
*pde = 00000000 
Oops: 0000 [#1] PREEMPT SMP 
last sysfs file: 
Modules linked in: BusLogic(+) sg scsi_mod unix

Pid: 947, comm: modprobe Not tainted (2.6.28-rc9-mm1 #2) VMware Virtual Platform
EIP: 0060:[<c0159475>] EFLAGS: 00010247 CPU: 1
EIP is at dma_alloc_from_coherent+0x35/0xa0
EAX: f73b3c58 EBX: 00000000 ECX: 00000001 EDX: 00001e68
ESI: 00001e68 EDI: 00000020 EBP: 00000000 ESP: f6b0fcc0
 DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
Process modprobe (pid: 947, ti=f6b0e000 task=f73ab850 task.ti=f6b0e000)
Stack:
 f6b0fce8 c0c00294 f73b3c58 00000020 c03be720 f80e604f f6b0fce4 00000c2c
 f80e7e8c c0c002ba c0c00294 f6b14000 c0c00350 c0c03344 00000c2c f80e8539
 f80dfeb0 00000000 00000000 f80e33e0 00000014 c0c00000 c0c00294 00000000
Call Trace:
 [<f80e604f>] BusLogic_CreateInitialCCBs+0x4f/0x120 [BusLogic]
 [<f80e7e8c>] BusLogic_AcquireResources+0x5c/0xf0 [BusLogic]
 [<f80e8539>] BusLogic_init+0x2d9/0x440 [BusLogic]
 [<c014ecb6>] each_symbol+0x46/0x1f0
 [<c014ee60>] find_symbol_in_section+0x0/0xf0
 [<c014ee60>] find_symbol_in_section+0x0/0xf0
 [<f80e8260>] BusLogic_init+0x0/0x440 [BusLogic]
 [<c0101115>] do_one_initcall+0x25/0x190
 [<c011f740>] update_curr+0x50/0xc0
 [<c0101fd3>] __switch_to+0x23/0x1c0
 [<c031121e>] _spin_unlock_irq+0xe/0x30
 [<c0124a16>] finish_task_switch+0x46/0xb0
 [<c030f6c6>] __sched_text_start+0x2a6/0x570
 [<c01202f3>] check_preempt_wakeup+0xd3/0x120
 [<c02081a3>] sub_alloc+0x73/0x150
 [<c02082fc>] idr_get_empty_slot+0x7c/0x140
 [<c0208aa6>] ida_get_new_above+0x96/0x180
 [<c01d5210>] sysfs_ilookup_test+0x0/0x10
 [<c01d5210>] sysfs_ilookup_test+0x0/0x10
 [<c01d55ba>] sysfs_find_dirent+0x1a/0x30
 [<c01d5328>] __sysfs_add_one+0x48/0x90
 [<c019cc76>] ilookup5+0x36/0x50
 [<c01d5382>] sysfs_add_one+0x12/0x50
 [<c01d551e>] sysfs_addrm_finish+0xe/0x90
 [<c01d4924>] sysfs_add_file_mode+0x54/0x80
 [<c01d69ab>] create_files+0xbb/0xe0
 [<c017e28e>] __vunmap+0x8e/0xc0
 [<c0150410>] mod_sysfs_setup+0x60/0xb0
 [<c0151841>] load_module+0x9f1/0xcc0
 [<c0151841>] load_module+0x9f1/0xcc0
 [<c0158dd1>] tracepoint_update_probe_range+0x21/0x60
 [<c0159277>] tracepoint_module_notify+0x27/0x30
 [<c01446ee>] notifier_call_chain+0x3e/0x60
 [<c014495d>] __blocking_notifier_call_chain+0x4d/0x70
 [<c0151c05>] sys_init_module+0xf5/0x1b0
 [<c010347a>] syscall_call+0x7/0xb
Code: 04 85 c0 89 0c 24 74 06 8b a8 98 00 00 00 8d 5e ff b9 ff ff ff ff c1 eb 0b 8d b6 00 00 00 00 8d bc 27 00 00 00 00 41 d1 eb 75 fb <8b> 55 08 31 c0 39 d6 7f 49 85 ed 74 3e 8b 45 10 e8 a6 7d 0b 00 
EIP: [<c0159475>] dma_alloc_from_coherent+0x35/0xa0 SS:ESP 0068:f6b0fcc0
---[ end trace d20c97140e4dab92 ]---
Segmentation fault
input: PC Speaker as /class/input/input1
parport_pc 00:08: reported by Plug and Play ACPI
parport0: PC-style at 0x378, irq 7 [PCSPP,TRISTATE]
Floppy drive(s): fd0 is 1.44M
FDC 0 is a post-1991 82077
ACPI: I/O resource piix4_smbus [0x1040-0x1047] conflicts with ACPI region SMB_ [0x1040-0x104b]
ACPI: Device needs an ACPI driver
piix4_smbus: probe of 0000:00:07.3 failed with error -16
FATAL: Could not open '/lib/modules/2.6.28-rc9-mm1/kernel/drivers/net/pcnet32.ko': No such file or directory
Driver 'sd' needs updating - please use bus_type methods
umount: devfs: not mounted
mount: unknown filesystem type 'devfs'
umount: devfs: not mounted
pivot_root: No such file or directory
/sbin/init: 432: cannot open dev/console: No such file
Kernel panic - not syncing: Attempted to kill init!

Re: [mmotm 2008-12-22-16-14] NULL pointer dereference in dma_alloc_from_coherent().

[Andrew Morton]

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.

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);
 
_



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) {
_

Re: [mmotm 2008-12-22-16-14] NULL pointer dereference in dma_alloc_from_coherent().

[Johannes Weiner]

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

Re: [mmotm 2008-12-22-16-14] NULL pointer dereference in dma_alloc_from_coherent().

[Andrew Morton]

On Wed, 24 Dec 2008 16:45:02 +0100 Johannes Weiner <hannes@cmpxchg.org> wrote:

> > 
> > 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.
> 

Looking at it again, that function has a bug:

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;
	int order = get_order(size);

	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;
	}
	return (mem != NULL);
}

if bitmap_find_free_region() fails and DMA_MEMORY_EXCLUSIVE is not set,
the function will fail to write anything to *ret and will return 1. 
This will cause dma_alloc_coherent() to return an uninitialised value,
crashing the kernel, perhaps via DMA to a random address (ugh).

So I think it should do this:

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;
	int order = get_order(size);

	if (mem) {
		int page = bitmap_find_free_region(mem->bitmap, mem->size,
						     order);
		if (page >= 0) {
			/*
			 * Memory was found in the per-device arena.
			 */
			*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) {
			/*
			 * The per-device arena is exhausted and we are not
			 * permitted to fall back to generic memory.
			 */
			*ret = NULL;
		} else {
			/*
			 * The per-device arena is exhausted and we are
			 * permitted to fall back to generic memory.
			 */
			 return 0;
		}
	}
	return (mem != NULL);
}

Can I have some review of this, please?

From: Andrew Morton <akpm@linux-foundation.org>

If bitmap_find_free_region() fails and DMA_MEMORY_EXCLUSIVE is not set,
the function will fail to write anything to *ret and will return 1.             This will cause dma_alloc_coherent() to return an uninitialised value,
crashing the kernel, perhaps via DMA to a random address.

Fix that by changing it to return zero in this case, so the caller will
proceed to allocate the memory from the generic memory allocator.

Cc: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Cc: Dmitry Baryshkov <dbaryshkov@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 kernel/dma-coherent.c |   16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff -puN kernel/dma-coherent.c~dma_alloc_from_coherent-fix-fallback-to-generic-memory kernel/dma-coherent.c
--- a/kernel/dma-coherent.c~dma_alloc_from_coherent-fix-fallback-to-generic-memory
+++ a/kernel/dma-coherent.c
@@ -116,11 +116,25 @@ int dma_alloc_from_coherent(struct devic
 		int page = bitmap_find_free_region(mem->bitmap, mem->size,
 						     order);
 		if (page >= 0) {
+			/*
+			 * Memory was found in the per-device arena.
+			 */
 			*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)
+		} else if (mem->flags & DMA_MEMORY_EXCLUSIVE) {
+			/*
+			 * The per-device arena is exhausted and we are not
+			 * permitted to fall back to generic memory.
+			 */
 			*ret = NULL;
+		} else {
+			/*
+			 * The per-device arena is exhausted and we are
+			 * permitted to fall back to generic memory.
+			 */
+			 return 0;
+		}
 	}
 	return (mem != NULL);
 }
_