[CHECKER] 9 potential copy_*_user bugs in 2.4.1

[CHECKER] 9 potential copy_*_user bugs in 2.4.1

[Dawson Engler]

Hi,

I wrote an extension to gcc that does global analysis to determine
which pointers in 2.4.1 are ever treated as user space pointers (i.e,
passed to copy_*_user, verify_area, etc) and then makes sure they are
always treated that way.

It found what looks to be 9 errors, and  3 cases I'm not sure about.
I've tried to eliminate false positives, but if any remain, please let
me know.

Two questions: 
	1. if a verify_area is done to a pointer, can the pointer then
	be manipulated raw?  E.g., is this ok?
		if(!verify_area(VERIFY_READ, p, sizeof *p)))
			tmp = *p;

	2.  And, unrelated:  given the current locking discipline, is
	it bad to hold any type of lock (not just a spin lock) when you
	call a potentially blocking function?  (It at least seems bad
	for performance, since you'll hold the lock for milliseconds.)

Dawson
---------------------------------------------------------
[BUG] sends 'rt' as the user pointer to copy_to_user, but ipddp_find_route
      derferences it directly.

	ipddp_find derefs this struct.
	struct at_addr
	{
        	__u16   s_net;
        	__u8    s_node;
	};

/u2/engler/mc/oses/linux/2.4.1/drivers/net/appletalk/ipddp.c:274:ipddp_ioctl: ERROR:PARAM:277:274: tainted var 'rt' (from line 277) used as arg 0 to 'ipddp_create'

        {
		case SIOCADDIPDDPRT:
Error --->
                        return (ipddp_create(rt));

                case SIOCFINDIPDDPRT:
Start --->
                        if(copy_to_user(rt, ipddp_find_route(rt), sizeof(struct ipddp_route)))
                                return -EFAULT;
---------------------------------------------------------
[BUG] sends 'rt' as the user pointer to copy_to_user, but ipddp_find_route
      derferences it directly.

/u2/engler/mc/oses/linux/2.4.1/drivers/net/appletalk/ipddp.c:277:ipddp_ioctl: ERROR:PARAM:277:277: tainted var 'rt' (from line 277) used as arg 0 to 'ipddp_find_route'


                case SIOCFINDIPDDPRT:
Start --->
Error --->
                        if(copy_to_user(rt, ipddp_find_route(rt), sizeof(struct ipddp_route)))
                                return -EFAULT;
---------------------------------------------------------
[BUG] sends 'rt' as the user pointer to copy_to_user, but ipddp_find_route
      derferences it directly.

/u2/engler/mc/oses/linux/2.4.1/drivers/net/appletalk/ipddp.c:282:ipddp_ioctl: ERROR:PARAM:277:282: tainted var 'rt' (from line 277) used as arg 0 to 'ipddp_delete'


                case SIOCFINDIPDDPRT:
Start --->
                        if(copy_to_user(rt, ipddp_find_route(rt), sizeof(struct ipddp_route)))
                                return -EFAULT;
                        return 0;

                case SIOCDELIPDDPRT:
Error --->
                        return (ipddp_delete(rt));

---------------------------------------------------------
[BUG] copy_to_user taints "arg" on one switch branch, but
      capabilities check derefs it w/o checks on another

/u2/engler/mc/oses/linux/2.4.1/drivers/telephony/ixj.c:5046:ixj_ioctl: ERROR:PARAM:4687:5046: tainted var 'arg' (from line 4687) used as arg 1 to 'capabilities_check'

		break;
	case IXJCTL_VERSION:
Start --->
		if (copy_to_user((char *) arg, ixj_c_revision, strlen(ixj_c_revision)))
			return -EFAULT;
		break;
	case PHONE_RING_CADENCE:
		j->ring_cadence = arg;
		break;
	case IXJCTL_CIDCW:

	... DELETED 345 lines ...

	case PHONE_CAPABILITIES:
		retval = j->caps;
		break;
	case PHONE_CAPABILITIES_LIST:
		if (copy_to_user((char *) arg, j->caplist, sizeof(struct phone_capability) * j->caps))
			 return -EFAULT;
		break;
	case PHONE_CAPABILITIES_CHECK:
Error --->
		retval = capabilities_check(board, (struct phone_capability *) arg);
		break;
	case PHONE_PSTN_SET_STATE:
		daa_set_mode(board, arg);
		break;


static int capabilities_check(int board, struct phone_capability *pcreq)
{
        int cnt;
        IXJ *j = &ixj[board];
        int retval = 0;
        for (cnt = 0; cnt < j->caps; cnt++) {
                if (pcreq->captype == j->caplist[cnt].captype
                    && pcreq->cap == j->caplist[cnt].cap) {
                        retval = 1;
                        break;
                }
        }
        return retval;
}


---------------------------------------------------------
[BUG] Looks like a bug where the memcpy forgets to use the user_buf pointer.

/u2/engler/mc/oses/linux/2.4.1/drivers/usb/serial/digi_acceleport.c:1288:digi_write: ERROR:PARAM:1271:1288: tainted var 'buf' (from line 1271) used as arg 1 to '__constant_memcpy'

	/* copy user data (which can sleep) before getting spin lock */
	count = MIN( 64, MIN( count, port->bulk_out_size-2 ) );
Start --->
	if( from_user && copy_from_user( user_buf, buf, count ) ) {
		return( -EFAULT );
	}

	/* be sure only one write proceeds at a time */
	/* there are races on the port private buffer */
	/* and races to check write_urb->status */

	/* wait for urb status clear to submit another urb */
	if( port->write_urb->status == -EINPROGRESS
	|| priv->dp_write_urb_in_use ) {

		/* buffer data if count is 1 (probably put_char) if possible */
		if( count == 1 ) {
			new_len = MIN( count,
				DIGI_OUT_BUF_SIZE-priv->dp_out_buf_len );
Error --->
			memcpy( priv->dp_out_buf+priv->dp_out_buf_len, buf,
				new_len );
			priv->dp_out_buf_len += new_len;
		} else {
			new_len = 0;

---------------------------------------------------------
[BUG]  put_user taints "optlen", but the dereference doesn't to anything
       special to it.
/u2/engler/mc/oses/linux/2.4.1/net/decnet/af_decnet.c:1433:__dn_getsockopt: ERROR:PARAM:1537:1433: Deref tainted var 'optlen' (tainted from line 1537)

	struct dn_scp *scp = DN_SK(sk);
	struct linkinfo_dn link;
Error --->
	int r_len = *optlen;
	void *r_data = NULL;
	int val;

	switch(optname) {
		case DSO_CONDATA:
			if (r_len > sizeof(struct optdata_dn))

	... DELETED 90 lines ...

				r_len = sizeof(unsigned char);
			r_data = &scp->info_rem;
			break;
	}

	if (r_data) {
		if (copy_to_user(optval, r_data, r_len))
			return -EFAULT;
Start --->
		if (put_user(r_len, optlen))
			return -EFAULT;
	}

	return 0;
---------------------------------------------------------
[BUG]   Looks like this code was grafted on later.  The put_user taints it,
	but the error dereference it without any checks.

/u2/engler/mc/oses/linux/2.4.1/net/decnet/af_decnet.c:1487:__dn_getsockopt: ERROR:PARAM:1537:1487: Deref tainted var 'optlen' (tainted from line 1537)

#ifdef CONFIG_NETFILTER
		{
Error --->
			int val, len = *optlen;
			val = nf_getsockopt(sk, PF_DECnet, optname, 
							optval, &len);
			if (val >= 0)
				val = put_user(len, optlen);
			return val;
		}

	... DELETED 36 lines ...

				r_len = sizeof(unsigned char);
			r_data = &scp->info_rem;
			break;
	}

	if (r_data) {
		if (copy_to_user(optval, r_data, r_len))
			return -EFAULT;
Start --->
		if (put_user(r_len, optlen))
			return -EFAULT;
	}

	return 0;

---------------------------------------------------------
[BUG] In debugging code though
/u2/engler/mc/oses/linux/2.4.1/drivers/usb/serial/omninet.c:308:omninet_write: ERROR:PARAM:315:308: tainted var 'buf' (from line 315) used as arg 3 to 'usb_serial_debug_data'

        }

Error --->
        usb_serial_debug_data (__FILE__, __FUNCTION__, count, buf);

        spin_lock_irqsave (&port->port_lock, flags);

        count = (count > OMNINET_BULKOUTSIZE) ? OMNINET_BULKOUTSIZE : count;

        if (from_user) {
Start --->
                copy_from_user(wport->write_urb->transfer_buffer + OMNINET_DATAOFFSET, buf, count);
        }
---------------------------------------------------------
[BUG] in debugging code though
/u2/engler/mc/oses/linux/2.4.1/drivers/usb/serial/usbserial.c:817:generic_write: ERROR:PARAM:820:817: tainted var 'buf' (from line 820) used as arg 3 to 'usb_serial_debug_data'

                count = (count > port->bulk_out_size) ? port->bulk_out_size : count;

Error --->
                usb_serial_debug_data (__FILE__, __FUNCTION__, count, buf);

                if (from_user) {
Start --->
                        copy_from_user(port->write_urb->transfer_buffer, buf, count);
                }

---------------------------------------------------------
[UNKNOWN]  does a verify_area on pDivalog and then calls Divalog which 
	   derefs it directly --- is this ok?

/u2/engler/mc/oses/linux/2.4.1/drivers/isdn/eicon/linchr.c:132:do_ioctl: ERROR:PARAM:130:132: tainted var 'pDivaLog' (from line 130) used as arg 0 to 'DivasLog'

                        pDivaLog = (dia_log_t *) arg;

Start --->
                        if (!verify_area(VERIFY_READ, pDivaLog, sizeof(dia_log_t)))
                        {
Error --->
                                DivasLog(pDivaLog);
                        }

/u2/engler/mc/oses/linux/2.4.1/drivers/isdn/eicon/linchr.c:173:do_ioctl: ERROR:PARAM:143:173: tainted var 'arg' (from line 143) used as arg 0 to 'DivasGetList'

/u2/engler/mc/oses/linux/2.4.1/drivers/isdn/eicon/linchr.c:187:do_ioctl: ERROR:PARAM:185:187: tainted var 'mem_block' (from line 185) used as arg 0 to 'DivasGetMem'

/u2/engler/mc/oses/linux/2.4.1/drivers/isdn/eicon/linchr.c:65:do_ioctl: ERROR:PARAM:63:65: tainted var 'pDivaConfig' (from line 63) used as arg 0 to 'DivasCardConfig'
---------------------------------------------------------
[UNKNOWN]  I'm not sure about this: "csum_partial_*" calls the generic
	   cksum routine which does guard against user pointers ---
	   is this redundant paranoia in this case?

/u2/engler/mc/oses/linux/2.4.1/net/ipv4/tcp_output.c:643:tcp_retrans_try_collapse: ERROR:PARAM:651:643: tainted var 'skb_put' (from line 651) used as arg 0 to '__constant_memcpy'

		if(skb->len % 4) {
			/* Must copy and rechecksum all data. */
Error --->
			memcpy(skb_put(skb, next_skb_size), next_skb->data, next_skb_size);
			skb->csum = csum_partial(skb->data, skb->len, 0);
		} else {
			/* Optimize, actually we could also combine next_skb->csum
			 * to skb->csum using a single add w/carry operation too.
			 */
			skb->csum = csum_partial_copy_nocheck(next_skb->data,
							      skb_put(skb, next_skb_size),
Start --->
							      next_skb_size, skb->csum);
		}

Re: [CHECKER] 9 potential copy_*_user bugs in 2.4.1

[Alexander Viro]

On Thu, 15 Mar 2001, Dawson Engler wrote:

> Hi,
> 
> I wrote an extension to gcc that does global analysis to determine
> which pointers in 2.4.1 are ever treated as user space pointers (i.e,
> passed to copy_*_user, verify_area, etc) and then makes sure they are
> always treated that way.
> 
> It found what looks to be 9 errors, and  3 cases I'm not sure about.
> I've tried to eliminate false positives, but if any remain, please let
> me know.

Looks like you've missed at least one place. Have you marked pointer
arguments of syscalls as tainted? Path in question looks so:

* 2nd argument of sys_write() (buf) is tainted	[SYSCALL]
* sys_write() passes buf  as 2nd argument to file_operations::write()
  (fs/read_write.c:159-160)
* proc_file_write() is an instance of the above (buffer is the 2nd argument)
  (fs/proc/generic.c:36--40)
* proc_file_write() passes buffer as 2nd argument to
proc_dir_entry::write_proc() (fs/proc/generic.c:136)
* proc_write_register() is an instance of the above (buffer is the 2nd
argument) (fs/binfmt_misc.c:494)
* proc_write_register() directly dereferences buffer. (fs/binfmt_misc.c:298)
(line numbers as per 2.4.2, 2.4.1 is essentially the same)

And yes, that's an oopsable bug (fixed in more or less recent -ac).
Since a lot of code is only accessed as methods... If you could add support
for that kind of checks you'd probably find much more.

Relevant rules:
	* all arguments of syscalls are tainted. Casting can't change that.
	* verify_area() cleans the value, but you'll be better off
considering these as dangerous - it only checks that range is OK and if
pointer arithmetics moves you out of that range or you access piece longer
than range in question...
	* if method's argument is ever tainted - all instances of that
method have that argument tainted.

Is it possible to implement? The last rule may be tricky - we need to
remember that field foo of structure bar has tainted nth argument and
keep track of all functions assigned to foo, either by initialization
or by direct assignment. Could that be done?
							Cheers,
								Al

Re: [CHECKER] 9 potential copy_*_user bugs in 2.4.1

[Dawson Engler]

> Looks like you've missed at least one place. Have you marked pointer
> arguments of syscalls as tainted? Path in question looks so:

In the exokernel param checker we do, but not for the one in linux ---
most of the pointers seemed to be devices, so I never added it.  Afer
your for bug example, I'll go hack the checker ;-)

> 	* if method's argument is ever tainted - all instances of that
> method have that argument tainted.
> 
> Is it possible to implement? The last rule may be tricky - we need to
> remember that field foo of structure bar has tainted nth argument and
> keep track of all functions assigned to foo, either by initialization
> or by direct assignment. Could that be done?

It should be.  We're using a trick similar to this one to build up
equivalence classes of interrupt handlers tracking which functions are
assigned to struct fields, or passed as the same parameter to a
function (request_irq being the prime example).  You'd expect that if 
any function passed/assigned to a given function/field is an 
interrupt handler then the rest are too.

The big win will be when checkers can get at global data structure
initializers.  From an outsiders view, it seems like most device
methods are registered that way.

Dawson
Dawson

Re: [CHECKER] 9 potential copy_*_user bugs in 2.4.1

[Jamie Lokier]

Alexander Viro wrote:
> 	* verify_area() cleans the value, but you'll be better off
> considering these as dangerous - it only checks that range is OK and if
> pointer arithmetics moves you out of that range or you access piece longer
> than range in question...

Note that verify_area's argument cannot be safely dereferenced if a
parallel thread is able to change the user-space mapping.  This is
usually possible.

-- Jamie

Re: [CHECKER] 9 potential copy_*_user bugs in 2.4.1

[Greg KH]

[-- Attachment #1: Type: text/plain, Size: 1889 bytes --]

On Thu, Mar 15, 2001 at 06:24:51PM -0800, Dawson Engler wrote:
> Hi,
> 
> I wrote an extension to gcc that does global analysis to determine
> which pointers in 2.4.1 are ever treated as user space pointers (i.e,
> passed to copy_*_user, verify_area, etc) and then makes sure they are
> always treated that way.
> 
> It found what looks to be 9 errors, and  3 cases I'm not sure about.
> I've tried to eliminate false positives, but if any remain, please let
> me know.

<snip>

> ---------------------------------------------------------
> [BUG] Looks like a bug where the memcpy forgets to use the user_buf pointer.
> 
> /u2/engler/mc/oses/linux/2.4.1/drivers/usb/serial/digi_acceleport.c:1288:digi_write: ERROR:PARAM:1271:1288: tainted var 'buf' (from line 1271) used as arg 1 to '__constant_memcpy'
> 
> 	/* copy user data (which can sleep) before getting spin lock */
> 	count = MIN( 64, MIN( count, port->bulk_out_size-2 ) );
> Start --->
> 	if( from_user && copy_from_user( user_buf, buf, count ) ) {
> 		return( -EFAULT );
> 	}
> 
> 	/* be sure only one write proceeds at a time */
> 	/* there are races on the port private buffer */
> 	/* and races to check write_urb->status */
> 
> 	/* wait for urb status clear to submit another urb */
> 	if( port->write_urb->status == -EINPROGRESS
> 	|| priv->dp_write_urb_in_use ) {
> 
> 		/* buffer data if count is 1 (probably put_char) if possible */
> 		if( count == 1 ) {
> 			new_len = MIN( count,
> 				DIGI_OUT_BUF_SIZE-priv->dp_out_buf_len );
> Error --->
> 			memcpy( priv->dp_out_buf+priv->dp_out_buf_len, buf,
> 				new_len );
> 			priv->dp_out_buf_len += new_len;
> 		} else {
> 			new_len = 0;
> 
> ---------------------------------------------------------

Al, Pete, does this patch look good to fix this problem?

(I'll send a separate patch for the other usb-serial problems.)

thanks,

greg k-h

-- 
greg@(kroah|wirex).com

[-- Attachment #2: digi_acceleport.patch --]
[-- Type: text/plain, Size: 454 bytes --]

--- digi_acceleport.c.original	Thu Mar 15 21:38:10 2001
+++ digi_acceleport.c	Thu Mar 15 21:38:46 2001
@@ -1285,8 +1285,8 @@
 		if( count == 1 ) {
 			new_len = MIN( count,
 				DIGI_OUT_BUF_SIZE-priv->dp_out_buf_len );
-			memcpy( priv->dp_out_buf+priv->dp_out_buf_len, buf,
-				new_len );
+			memcpy( priv->dp_out_buf+priv->dp_out_buf_len, 
+				from_user ? user_buf : buf, new_len );
 			priv->dp_out_buf_len += new_len;
 		} else {
 			new_len = 0;

Re: [CHECKER] 9 potential copy_*_user bugs in 2.4.1

[David S. Miller]

Dawson Engler writes:
 > ---------------------------------------------------------
 > [UNKNOWN]  I'm not sure about this: "csum_partial_*" calls the generic
 > 	   cksum routine which does guard against user pointers ---
 > 	   is this redundant paranoia in this case?
 > 
 > /u2/engler/mc/oses/linux/2.4.1/net/ipv4/tcp_output.c:643:tcp_retrans_try_collapse: ERROR:PARAM:651:643: tainted var 'skb_put' (from line 651) used as arg 0 to '__constant_memcpy'

csum_partial_copy_nocheck works on kernel pointers.

Later,
David S. Miller
davem@redhat.com

Re: [CHECKER] 9 potential copy_*_user bugs in 2.4.1

[David Woodhouse]

engler@csl.Stanford.EDU said:
>  I wrote an extension to gcc that does global analysis to determine
> which pointers in 2.4.1 are ever treated as user space pointers (i.e,
> passed to copy_*_user, verify_area, etc) and then makes sure they are
> always treated that way.

Nice work - thanks. One request though, to you and anyone else doing such
cleanups - please could you list the affected files separately near the
beginning of your mail, so that people can tell at a glance whether there's
anything in there that might be their fault.

--
dwmw2

Re: [CHECKER] 9 potential copy_*_user bugs in 2.4.1

[Russell King]

On Fri, Mar 16, 2001 at 10:06:48AM +0000, David Woodhouse wrote:
> Nice work - thanks. One request though, to you and anyone else doing such
> cleanups - please could you list the affected files separately near the
> beginning of your mail, so that people can tell at a glance whether there's
> anything in there that might be their fault.

Also, it'd be nice if the filenames were in alphabetic order.  Both points
make it much easier to examine the list of affected files.

--
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Locking question (was: [CHECKER] 9 potential copy_*_user bugs in 2.4.1)

[Nigel Gamble]

On Thu, 15 Mar 2001, Dawson Engler wrote:
> 	2.  And, unrelated:  given the current locking discipline, is
> 	it bad to hold any type of lock (not just a spin lock) when you
> 	call a potentially blocking function?  (It at least seems bad
> 	for performance, since you'll hold the lock for milliseconds.)

In general, yes.  The lock may be held for much longer than milliseconds
if the potentially blocking function is waiting for I/O from a network,
or a terminal, potentially causing all threads to block on the lock
until someone presses a key, in this extreme example.  If the lock is a
spinlock, then complete deadlock can occur.

You're probably aware that semaphores are used both as blocking mutex
locks, where the down (lock) and up (unlock) calls are made by the same
thread to protect critical data, and as a synchronization mechanism,
where the down and up calls are made by different threads.   The former
use is a "lock", while the latter down() use is a "potentially blocking
function" in terms of your question.  I don't know how easy it would be
for your analysis tools to distinguish between them.

Nigel Gamble                                    nigel@nrg.org
Mountain View, CA, USA.                         http://www.nrg.org/

MontaVista Software                             nigel@mvista.com

Re: [CHECKER] 9 potential copy_*_user bugs in 2.4.1

[Rusty Russell]

In message <200103160224.SAA03920@csl.Stanford.EDU> you write:
> Hi,
> 
> I wrote an extension to gcc that does global analysis to determine
> which pointers in 2.4.1 are ever treated as user space pointers (i.e,
> passed to copy_*_user, verify_area, etc) and then makes sure they are
> always treated that way.

Hi Dawson,

	FYI, you missed one, which was fixed in 2.4.2.  This is tricky
since ip_fw_ctl is defined in TWO (mutually exclusive) places:
ipfwadm_core.c and ipchains_core.c.

	Oh, I see in a later message that you do CONFIG=y.  Hmm, you
won't even get asked about these if you've said CONFIG=y to
CONFIG_IPTABLES.  You're best off trying CONFIG=m, which allows a
compile of everything, but that may be outside your framework, in
which case a series of different configurations might be in order...

diff -u --recursive --new-file v2.4.1/linux/net/ipv4/netfilter/ip_fw_compat.c linux/net/ipv4/netfilter/ip_fw_compat.c
--- v2.4.1/linux/net/ipv4/netfilter/ip_fw_compat.c	Mon Sep 18 15:09:55 2000
+++ linux/net/ipv4/netfilter/ip_fw_compat.c	Fri Feb  9 11:34:13 2001
@@ -9,6 +9,7 @@
 #include <linux/inetdevice.h>
 #include <linux/netdevice.h>
 #include <linux/module.h>
+#include <asm/uaccess.h>
 #include <net/ip.h>
 #include <net/route.h>
 #include <linux/netfilter_ipv4/compat_firewall.h>
@@ -197,14 +198,28 @@
 	return NF_ACCEPT;
 }
 
-extern int ip_fw_ctl(int optval, void *user, unsigned int len);
+extern int ip_fw_ctl(int optval, void *m, unsigned int len);
 
 static int sock_fn(struct sock *sk, int optval, void *user, unsigned int len)
 {
+	/* MAX of:
+	   2.2: sizeof(struct ip_fwtest) (~14x4 + 3x4 = 17x4)
+	   2.2: sizeof(struct ip_fwnew) (~1x4 + 15x4 + 3x4 + 3x4 = 22x4)
+	   2.0: sizeof(struct ip_fw) (~25x4)
+
+	   We can't include both 2.0 and 2.2 headers, they conflict.
+	   Hence, 200 is a good number. --RR */
+	char tmp_fw[200];
 	if (!capable(CAP_NET_ADMIN))
 		return -EPERM;
 
-	return -ip_fw_ctl(optval, user, len);
+	if (len > sizeof(tmp_fw) || len < 1)
+		return -EINVAL;
+
+	if (copy_from_user(&tmp_fw, user, len))
+		return -EFAULT;
+
+	return -ip_fw_ctl(optval, &tmp_fw, len);
 }
 
 static struct nf_hook_ops preroute_ops

Hope that helps, and keep up the great work!
Rusty.
--
Premature optmztion is rt of all evl. --DK