Re: [CHECKER] Probable security holes in 2.6.5

[Chris Wright <chrisw@osdl.org>]

> [BUG] minor
> /home/kash/linux/linux-2.6.5/drivers/net/wan/sdla.c:1206:sdla_xfer:
> ERROR:TAINT: 1201:1206:Passing unbounded user value "(mem).len" as arg 0
> to function "kmalloc", which uses it unsafely in model
> [SOURCE_MODEL=(lib,copy_from_user,user,taintscalar)]
> [SINK_MODEL=(lib,kmalloc,user,trustingsink)]  [MINOR]  [PATH=] [Also
> used at, line 1219 in argument 0 to function "kmalloc"]
> static int sdla_xfer(struct net_device *dev, struct sdla_mem *info, int
> read)
> {
> 	struct sdla_mem mem;
> 	char	*temp;
> 
> Start --->
> 	if(copy_from_user(&mem, info, sizeof(mem)))
> 		return -EFAULT;
> 		
> 	if (read)
> 	{	
> Error --->
> 		temp = kmalloc(mem.len, GFP_KERNEL);
> 		if (!temp)
> 			return(-ENOMEM);
> 		sdla_read(dev, mem.addr, temp, mem.len);

Hrm, I believe you could use this to read 128k of kernel memory.
sdla_read() takes len as a short, whereas mem.len is an int.  So,
if mem.len == 0x20000, the allocation could still succeed.  When cast
to short, len will be 0x0, causing the read loop to copy nothing into
the buffer.  At least it's protected by a capable() check.  I don't
know what proper upper bound is for this hardware, or how much it's
used/cared about.  Simple memset() is trivial fix.

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

===== drivers/net/wan/sdla.c 1.19 vs edited =====
--- 1.19/drivers/net/wan/sdla.c	Sat Jan 10 16:15:24 2004
+++ edited/drivers/net/wan/sdla.c	Fri Apr 16 17:55:18 2004
@@ -1206,6 +1206,7 @@
 		temp = kmalloc(mem.len, GFP_KERNEL);
 		if (!temp)
 			return(-ENOMEM);
+		memset(temp, 0, mem.len);
 		sdla_read(dev, mem.addr, temp, mem.len);
 		if(copy_to_user(mem.data, temp, mem.len))
 		{