Re: [CHECKER] 9 potential copy_*_user bugs in 2.4.1
In message <[EMAIL PROTECTED]> 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
#include
#include
+#include
#include
#include
#include
@@ -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(_fw, user, len))
+ return -EFAULT;
+
+ return -ip_fw_ctl(optval, _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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [CHECKER] 9 potential copy_*_user bugs in 2.4.1
In message [EMAIL PROTECTED] 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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [CHECKER] 9 potential copy_*_user bugs in 2.4.1
On Fri, Mar 16, 2001 at 10:06:48AM +, 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 ([EMAIL PROTECTED])The developer of ARM Linux http://www.arm.linux.org.uk/personal/aboutme.html - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [CHECKER] 9 potential copy_*_user bugs in 2.4.1
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [CHECKER] 9 potential copy_*_user bugs in 2.4.1
[EMAIL PROTECTED] 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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [CHECKER] 9 potential copy_*_user bugs in 2.4.1
[EMAIL PROTECTED] 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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [CHECKER] 9 potential copy_*_user bugs in 2.4.1
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [CHECKER] 9 potential copy_*_user bugs in 2.4.1
On Fri, Mar 16, 2001 at 10:06:48AM +, 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 ([EMAIL PROTECTED])The developer of ARM Linux http://www.arm.linux.org.uk/personal/aboutme.html - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [CHECKER] 9 potential copy_*_user bugs in 2.4.1
> 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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [CHECKER] 9 potential copy_*_user bugs in 2.4.1
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 [EMAIL PROTECTED] - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [CHECKER] 9 potential copy_*_user bugs in 2.4.1
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.
> -
> [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
--- 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
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [CHECKER] 9 potential copy_*_user bugs in 2.4.1
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [CHECKER] 9 potential copy_*_user bugs in 2.4.1
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
--- 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
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
