[PATCH] AudioScience HPI driver: Unsafe memory management when allocating control cache

[PATCH] AudioScience HPI driver: Unsafe memory management when allocating control cache

[Jesper Juhl]

Hi,

I noticed that sound/pci/asihpi/hpicmn.c::hpi_alloc_control_cache() does 
not check the return value from kmalloc(), which may fail.
If kmalloc() fails we'll dereference a null pointer and things will go bad 
fast.
There are two memory allocations in that function and there's also the 
problem that the first may succeed and the second may fail and nothing is 
done about that either which will also go wrong down the line.

I tried to fix that up in the patch below, but since I don't know this 
code at all nor have the hardware to test anything the patch is based 
entirely on 30 minutes of investigation of the source and then compile 
tested only, so it may be the completely wrong way to fix the issues. Thus 
I am not suggesting that this patch be merged as-is unless someone else 
who know this code says it looks correct (in which case, by all means 
merge it), I'm mainly trying to bring the issue to the attention of the 
relevant people and providing an attempted fix seemed like the best way to 
do that.

Btw; please keep me on Cc if you reply.


Signed-off-by: Jesper Juhl <jj@chaosbits.net>
---
 hpi6000.c |    2 ++
 hpi6205.c |    2 ++
 hpicmn.c  |   12 +++++++++---
 3 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/sound/pci/asihpi/hpi6000.c b/sound/pci/asihpi/hpi6000.c
index f7e374e..1b9bf93 100644
--- a/sound/pci/asihpi/hpi6000.c
+++ b/sound/pci/asihpi/hpi6000.c
@@ -625,6 +625,8 @@ static short create_adapter_obj(struct hpi_adapter_obj *pao,
 			control_cache_size, (struct hpi_control_cache_info *)
 			&phw->control_cache[0]
 			);
+		if (!phw->p_cache)
+			pao->has_control_cache = 0;
 	} else
 		pao->has_control_cache = 0;
 
diff --git a/sound/pci/asihpi/hpi6205.c b/sound/pci/asihpi/hpi6205.c
index 22c5fc6..2672f65 100644
--- a/sound/pci/asihpi/hpi6205.c
+++ b/sound/pci/asihpi/hpi6205.c
@@ -644,6 +644,8 @@ static u16 create_adapter_obj(struct hpi_adapter_obj *pao,
 				interface->control_cache.size_in_bytes,
 				(struct hpi_control_cache_info *)
 				p_control_cache_virtual);
+			if (!phw->p_cache)
+				err = HPI_ERROR_MEMORY_ALLOC;
 		}
 		if (!err) {
 			err = hpios_locked_mem_get_phys_addr(&phw->
diff --git a/sound/pci/asihpi/hpicmn.c b/sound/pci/asihpi/hpicmn.c
index dda4f1c..d67f4d3 100644
--- a/sound/pci/asihpi/hpicmn.c
+++ b/sound/pci/asihpi/hpicmn.c
@@ -571,14 +571,20 @@ struct hpi_control_cache *hpi_alloc_control_cache(const u32
 {
 	struct hpi_control_cache *p_cache =
 		kmalloc(sizeof(*p_cache), GFP_KERNEL);
+	if (!p_cache)
+		return NULL;
+	p_cache->p_info =
+		kmalloc(sizeof(*p_cache->p_info) * number_of_controls,
+			GFP_KERNEL);
+	if (!p_cache->p_info) {
+		kfree(p_cache);
+		return NULL;
+	}
 	p_cache->cache_size_in_bytes = size_in_bytes;
 	p_cache->control_count = number_of_controls;
 	p_cache->p_cache =
 		(struct hpi_control_cache_single *)pDSP_control_buffer;
 	p_cache->init = 0;
-	p_cache->p_info =
-		kmalloc(sizeof(*p_cache->p_info) * p_cache->control_count,
-		GFP_KERNEL);
 	return p_cache;
 }
 

-- 
Jesper Juhl <jj@chaosbits.net>             http://www.chaosbits.net/
Plain text mails only, please      http://www.expita.com/nomime.html
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html

Re: [PATCH] AudioScience HPI driver: Unsafe memory management when allocating control cache

[Eliot Blennerhassett]

On 30/10/10 08:35, Jesper Juhl wrote:
> I noticed that sound/pci/asihpi/hpicmn.c::hpi_alloc_control_cache() does 
> not check the return value from kmalloc(), which may fail.
> If kmalloc() fails we'll dereference a null pointer and things will go bad 
> fast.
> There are two memory allocations in that function and there's also the 
> problem that the first may succeed and the second may fail and nothing is 
> done about that either which will also go wrong down the line.

Thanks Jesper,

your changes look fine.

Acked-by: Eliot Blennerhassett <linux@audioscience.com>

Re: [PATCH] AudioScience HPI driver: Unsafe memory management when allocating control cache

[Takashi Iwai]

At Tue, 02 Nov 2010 11:19:36 +1300,
Eliot Blennerhassett wrote:
> 
> On 30/10/10 08:35, Jesper Juhl wrote:
> > I noticed that sound/pci/asihpi/hpicmn.c::hpi_alloc_control_cache() does 
> > not check the return value from kmalloc(), which may fail.
> > If kmalloc() fails we'll dereference a null pointer and things will go bad 
> > fast.
> > There are two memory allocations in that function and there's also the 
> > problem that the first may succeed and the second may fail and nothing is 
> > done about that either which will also go wrong down the line.
> 
> Thanks Jesper,
> 
> your changes look fine.
> 
> Acked-by: Eliot Blennerhassett <linux@audioscience.com>

OK, applied now.  Thanks!


Takashi