Re: [CHECKER] potential dereference of user pointer errors

[Chris Wright <chris@wirex.com>]

* Junfeng Yang (yjf@stanford.edu) wrote:
> 
> [BUG] cmd is tainted. then they do copy_to_user (cmd->resbuf, ...) resbuf
> is declared as a pointer, not an array, so cmd->resbuf can point to
> anywhere.
> 
> /home/junfeng/linux-2.5.63/drivers/message/i2o/i2o_config.c:440:ioctl_parms:
> ERROR:TAINTED deferencing "cmd" tainted by [dist=0][(null)]

I don't think this could be used to corrupt kernel data because the
copy_to_user will, of course, still verify the resbuf pointer, and the
original copy_from_user validated the cmd pointer.  The possibility of
trashing the processes own memory space is there, but can be done anyway
on first pass of the cmd.  However, this is inconsistent with the rest
of the file, so here is a patch to use kcmd.resbuf.  I also added a NULL
check, as done in similar funcitons in this file.  Alan, this look ok?

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

===== drivers/message/i2o/i2o_config.c 1.14 vs edited =====
--- 1.14/drivers/message/i2o/i2o_config.c	Wed Feb 19 12:18:31 2003
+++ edited/drivers/message/i2o/i2o_config.c	Fri Mar 21 14:17:20 2003
@@ -394,6 +394,9 @@
 	if(get_user(reslen, kcmd.reslen))
 		return -EFAULT;
 
+	if(!kcmd.resbuf)
+		return -EFAULT;
+
 	c = i2o_find_controller(kcmd.iop);
 	if(!c)
 		return -ENXIO;
@@ -437,7 +440,7 @@
 	put_user(len, kcmd.reslen);
 	if(len > reslen)
 		ret = -ENOBUFS;
-	else if(copy_to_user(cmd->resbuf, res, len))
+	else if(copy_to_user(kcmd->resbuf, res, len))
 		ret = -EFAULT;
 
 	kfree(res);