1. Would it be better to use copy_to_user() rather than
access_ok() followed immediately by __copy_to_user()?

2. Is it OK to dereference *lenp directly?  Is lenp a pointer into user
memory or kernel memory?  If it points to user memory, why is it safe to
dereference it directly?  (What about TOCTTOU bugs?)  Should there be
some sparse annotations here to ensure the code is not dereferencing
user pointers directly?  Later on, proc_do_xprt() also dereferences
*lenp and *ppos directly.

3. 'len' is declared as a signed int.  len will be converted to size_t
before doing the comparison, so if len can ever be negative (e.g.,
svc_print_xprts() returns -1 because of an error), this patch will do
the wrong thing.  Looks like the current definition of svc_print_xprts()
won't ever do that, as that code currently stands, so at present this
is not a bug.  However from a security point of view there are benefits
to code whose correctness is 'locally obvious', all else being equal.
In particular this seems like a possible maintenance hazard.  Would it be
better to use type size_t for lengths like this that are never supposed
to be negative?

4. Is proc_dostring() relevant here?
--

[ message continues ]

[David Wagner - Sat, Aug 30, 2008 at 10:55:51PM +0000]
| Cyrill Gorcunov  wrote:
| >Index: linux-2.6.git/net/sunrpc/sysctl.c
| >===================================================================
| >--- linux-2.6.git.orig/net/sunrpc/sysctl.c	2008-07-20 11:40:14.000000000 +0400
| >+++ linux-2.6.git/net/sunrpc/sysctl.c	2008-08-30 23:05:30.000000000 +0400
| >@@ -69,6 +69,8 @@ static int proc_do_xprt(ctl_table *table
| > 		return -EINVAL;
| > 	else {
| > 		len = svc_print_xprts(tmpbuf, sizeof(tmpbuf));
| >+		if (*lenp < len)
| >+			return -EFAULT;
| > 		if (!access_ok(VERIFY_WRITE, buffer, len))
| > 			return -EFAULT;
| 
| 1. Would it be better to use copy_to_user() rather than
| access_ok() followed immediately by __copy_to_user()?
| 
| 2. Is it OK to dereference *lenp directly?  Is lenp a pointer into user
| memory or kernel memory?  If it points to user memory, why is it safe to
| dereference it directly?  (What about TOCTTOU bugs?)  Should there be
| some sparse annotations here to ensure the code is not dereferencing
| user pointers directly?  Later on, proc_do_xprt() also dereferences
| *lenp and *ppos directly.

Didn't check for this - will do.

| 
| 3. 'len' is declared as a signed int.  len will be converted to size_t
| before doing the comparison, so if len can ever be negative (e.g.,
| svc_print_xprts() returns -1 because of an error), this patch will do
| the wrong thing.  Looks like the current definition of svc_print_xprts()
| won't ever do that, as that code currently stands, so at present this
| is not a bug.  However from a security point of view there are benefits
| to code whose correctness is 'locally obvious', all else being equal.
| In particular this seems like a possible maintenance hazard.  Would it be
| better to use type size_t for lengths like this that are never supposed
| to be negative?
| 
| 4. Is proc_dostring() relevant here?
| 

Yes David, I think proc_dostring is better candidate to use here, thanks!

		- Cyrill -
--

[ message continues ]

[David Wagner - Sat, Aug 30, 2008 at 10:55:51PM +0000]
| Cyrill Gorcunov  wrote:
| >Index: linux-2.6.git/net/sunrpc/sysctl.c
| >===================================================================
| >--- linux-2.6.git.orig/net/sunrpc/sysctl.c	2008-07-20 11:40:14.000000000 +0400
| >+++ linux-2.6.git/net/sunrpc/sysctl.c	2008-08-30 23:05:30.000000000 +0400
| >@@ -69,6 +69,8 @@ static int proc_do_xprt(ctl_table *table
| > 		return -EINVAL;
| > 	else {
| > 		len = svc_print_xprts(tmpbuf, sizeof(tmpbuf));
| >+		if (*lenp < len)
| >+			return -EFAULT;
| > 		if (!access_ok(VERIFY_WRITE, buffer, len))
| > 			return -EFAULT;
| 
| 1. Would it be better to use copy_to_user() rather than
| access_ok() followed immediately by __copy_to_user()?

Yes, thanks

| 
| 2. Is it OK to dereference *lenp directly?  Is lenp a pointer into user
| memory or kernel memory?  If it points to user memory, why is it safe to
| dereference it directly?  (What about TOCTTOU bugs?)  Should there be
| some sparse annotations here to ensure the code is not dereferencing
| user pointers directly?  Later on, proc_do_xprt() also dereferences
| *lenp and *ppos directly.

Not only proc_do_xprt do that so I think it's safe (check for NULL
on highr level I suspect).

| 
| 3. 'len' is declared as a signed int.  len will be converted to size_t
| before doing the comparison, so if len can ever be negative (e.g.,
| svc_print_xprts() returns -1 because of an error), this patch will do
| the wrong thing.  Looks like the current definition of svc_print_xprts()
| won't ever do that, as that code currently stands, so at present this
| is not a bug.  However from a security point of view there are benefits
| to code whose correctness is 'locally obvious', all else being equal.
| In particular this seems like a possible maintenance hazard.  Would it be
| better to use type size_t for lengths like this that are never supposed
| to be negative?

thanks, I changed it to size_t and as you mentoined negative is never ...
[ message continues ]

[Cyrill Gorcunov - Sun, Aug 31, 2008 at 02:30:26PM +0400]
...
| 
| | 
| | 2. Is it OK to dereference *lenp directly?  Is lenp a pointer into user
| | memory or kernel memory?  If it points to user memory, why is it safe to
| | dereference it directly?  (What about TOCTTOU bugs?)  Should there be
| | some sparse annotations here to ensure the code is not dereferencing
| | user pointers directly?  Later on, proc_do_xprt() also dereferences
| | *lenp and *ppos directly.

on second view: will check for TOCTTOU bug (iirc vfs layer does
latch file descriptor for these kind of operations)

| 
| Not only proc_do_xprt do that so I think it's safe (check for NULL
| on highr level I suspect).
| 
...

		- Cyrill -
--

[ message continues ]