Fix user data corrupted by old value return of sysctl

If the user reads a sysctl entry which is of string type by sysctl syscall, this call probably corrupts the user data right after the old value buffer, the issue lies in sysctl_string seting 0 to oldval[len], len is the available buffer size specified by the user, obviously, this will write to the first byte of the user memory place immediate after the old value buffer, the correct way is that sysctl_string doesn't set 0, the user should do it by self in the program.

Quoted 10 lines · Yi Yang

If the user reads a sysctl entry which is of string type by sysctl syscall, this call probably corrupts the user data right after the old value buffer, the issue lies in sysctl_string seting 0 to oldval[len], len is the available buffer size specified by the user, obviously, this will write to the first byte of the user memory place immediate after the old value buffer, the correct way is that sysctl_string doesn't set 0, the user should do it by self in the program.

Hmm.. I think this patch is incomplete.

We _should_ zero-pad the data, at least if the result fits in the buffer.

So I think the correct fix is to just _copy_ the last zero if it fits in the buffer, rather than do the unconditional "add NUL at the end" thing. The simplest way to do that is to just make "l" be "strlen(str)+1", so that we count the ending NUL in the length (and then, if the buffer isn't big enough, we will truncate it).

In other words, I would instead suggest a patch like the appended.

But even that is questionable: one alternative is to always zero-pad (like we used to), but make sure that the buffer size is sufficient for it (ie instead of adding one to the length of the string, we'd subtract one from the buffer length and make sure that the '\0' fits..

Comments?

Linus
---

kernel/sysctl.c +1 -3

index 9990e10..ad0425a 100644
@@ -2201,14 +2201,12 @@ int sysctl_string(ctl_table *table, int 
 		if (get_user(len, oldlenp))
 			return -EFAULT;
 		if (len) {
-			l = strlen(table->data);
+			l = strlen(table->data)+1;
 			if (len > l) len = l;
 			if (len >= table->maxlen)
 				len = table->maxlen;
 			if(copy_to_user(oldval, table->data, len))
 				return -EFAULT;
-			if(put_user(0, ((char __user *) oldval) + len))
-				return -EFAULT;
 			if(put_user(len, oldlenp))
 				return -EFAULT;
 		}

Quoted 9 lines · Yi Yang

If the user reads a sysctl entry which is of string type by sysctl syscall, this call probably corrupts the user data right after the old value buffer, the issue lies in sysctl_string seting 0 to oldval[len], len is the available buffer size specified by the user, obviously, this will write to the first byte of the user memory place immediate after the old value buffer, the correct way is that sysctl_string doesn't set 0, the user should do it by self in the program.

Hmm.. I think this patch is incomplete.

We _should_ zero-pad the data, at least if the result fits in the buffer.

So I think the correct fix is to just _copy_ the last zero if it fits in the buffer, rather than do the unconditional "add NUL at the end" thing. The simplest way to do that is to just make "l" be "strlen(str)+1", so that we count the ending NUL in the length (and then, if the buffer isn't big enough, we will truncate it).

In other words, I would instead suggest a patch like the appended.

But even that is questionable: one alternative is to always zero-pad (like we used to), but make sure that the buffer size is sufficient for it (ie instead of adding one to the length of the string, we'd subtract one from the buffer length and make sure that the '\0' fits..

Comments?

Quoted 9 lines

If the user reads a sysctl entry which is of string type by sysctl syscall, this call probably corrupts the user data right after the old value buffer, the issue lies in sysctl_string seting 0 to oldval[len], len is the available buffer size specified by the user, obviously, this will write to the first byte of the user memory place immediate after the old value buffer, the correct way is that sysctl_string doesn't set 0, the user should do it by self in the program.

We _should_ zero-pad the data, at least if the result fits in the buffer.

But even that is questionable: one alternative is to always zero-pad (like we used to), but make sure that the buffer size is sufficient for it (ie instead of adding one to the length of the string, we'd subtract one from the buffer length and make sure that the '\0' fits..

If the user reads a sysctl entry which is of string type
by sysctl syscall, this call probably corrupts the user data right after the old value buffer, the issue lies in sysctl_string seting 0 to oldval[len], len is the available buffer size specified by the user, obviously, this will write to the first byte of the user memory place immediate after the old value buffer
, the correct way is that sysctl_string doesn't set 0, the user should do it by self in the program.

If the user reads a sysctl entry which is of string type
by sysctl syscall, this call probably corrupts the user data right after the old value buffer, the issue lies in sysctl_string seting 0 to oldval[len], len is the available buffer size specified by the user, obviously, this will write to the first byte of the user memory place immediate after the old value buffer
, the correct way is that sysctl_string doesn't set 0, the user should do it by self in the program.

The following program verifies this point:

#include <linux/unistd.h>
#include <linux/types.h>
#include <linux/sysctl.h>
#include <errno.h>

_syscall1(int, _sysctl, struct __sysctl_args *, args); int sysctl(int *name, int nlen, void *oldval, size_t *oldlenp,
void *newval, size_t newlen)

{
         struct __sysctl_args args
                = {name,nlen,oldval,oldlenp,newval,newlen};

         return _sysctl(&args);
}

#define SIZE(x) sizeof(x)/sizeof(x[0])
#define OSNAMESZ 4

struct mystruct {
        char osname[OSNAMESZ];
        int target;
        int osnamelth;
} myos;

int name[] = { CTL_KERN, KERN_NODENAME };

int main(int argc, char * argv[])

{
        myos.target = 1;
        printf("target = %d\n", myos.target);
        myos.osnamelth = SIZE(myos.osname);
         if (sysctl(name, SIZE(name), myos.osname,
                        &myos.osnamelth, 0, 0))
                 perror("sysctl");
        else {
                 printf("Current host name: %s\n", myos.osname);
        }
        printf("target = %d\n", myos.target);
         return 0;
}

Copy it to file sysctl-safe.c, then

$ hostname
mylocalmachine
$ gcc sysctl-safe.c
$ ./a.out
target = 1
Current host name: mylo
target = 0
$

After apply this patch:

$ hostname
mylocalmachine
$ gcc sysctl-safe.c
$ ./a.out
target = 1
Current host name: mylo

target = 1

Signed-off-by Yi Yang <[email protected]>

2005/12/30, Yi Yang <[email protected]>:

Quoted 62 lines

If the user reads a sysctl entry which is of string type
by sysctl syscall, this call probably corrupts the user data right after the old value buffer, the issue lies in sysctl_string seting 0 to oldval[len], len is the available buffer size specified by the user, obviously, this will write to the first byte of the user memory place immediate after the old value buffer
, the correct way is that sysctl_string doesn't set 0, the user should do it by self in the program.

The following program verifies this point:

#include <linux/unistd.h>
#include <linux/types.h>
#include <linux/sysctl.h>
#include <errno.h>

_syscall1(int, _sysctl, struct __sysctl_args *, args); int sysctl(int *name, int nlen, void *oldval, size_t *oldlenp,
void *newval, size_t newlen)

{
        struct __sysctl_args args
               = {name,nlen,oldval,oldlenp,newval,newlen};

        return _sysctl(&args);
}

#define SIZE(x) sizeof(x)/sizeof(x[0])
#define OSNAMESZ 4

struct mystruct {
       char osname[OSNAMESZ];
       int target;
       int osnamelth;
} myos;

int name[] = { CTL_KERN, KERN_NODENAME };

int main(int argc, char * argv[])

{
       myos.target = 1;
       printf("target = %d\n", myos.target);
       myos.osnamelth = SIZE(myos.osname);
        if (sysctl(name, SIZE(name), myos.osname,
                       &myos.osnamelth, 0, 0))
                perror("sysctl");
       else {
                printf("Current host name: %s\n", myos.osname);
       }
       printf("target = %d\n", myos.target);
        return 0;
}

Copy it to file sysctl-safe.c, then

$ hostname
mylocalmachine
$ gcc sysctl-safe.c
$ ./a.out
target = 1
Current host name: mylo
target = 0
$

Quoted 9 lines

After apply this patch:

$ hostname
mylocalmachine
$ gcc sysctl-safe.c
$ ./a.out
target = 1
Current host name: mylo

You didn't set the trailing '\0', I wonder how your printf did work properly ever. You've just been lucky or something.

-- Coywolf

Quoted 16 lines

target = 1

Signed-off-by Yi Yang <[email protected]>

kernel/sysctl.c 2005-12-30 15:58:15.000000000 +0000 int sysctl_string(ctl_table *table, int · line 2212

Quoted 8 lines

kernel/sysctl.c 2005-12-30 15:58:15.000000000 +0000 renamed from kernel/sysctl.c.orig 2005-12-30 09:21:34.000000000 +0000 +0 -2

@@ -2207,8 +2207,6 @@ int sysctl_string(ctl_table *table, int
                               len = table->maxlen;
                       if(copy_to_user(oldval, table->data, len))
                               return -EFAULT;
-                       if(put_user(0, ((char __user *) oldval) + len))
-                               return -EFAULT;
                       if(put_user(len, oldlenp))
                               return -EFAULT;
               }

--
Coywolf Qi Hunt

Quoted 6 lines · Coywolf Qi Hunt

You didn't set the trailing '\0', I wonder how your printf did work properly ever. You've just been lucky or something.

-- Coywolf

The variable target does it, its value is 0x00000001, so you mustn't worry it.
osname only has 4-bytes space, so if you set '\0' to its tail, a byte information will be lost.

2005/12/31, Yi Yang <[email protected]>:

Quoted 11 lines

Quoted 8 lines · Coywolf Qi Hunt

You didn't set the trailing '\0', I wonder how your printf did work properly ever. You've just been lucky or something.

-- Coywolf

Quoted 6 lines

The variable target does it, its value is 0x00000001, so you mustn't worry it.
osname only has 4-bytes space, so if you set '\0' to its tail, a byte information will be lost.

I'm worrying more. We should set '\0'. Let the one byte information lost, the caller deserve that. Actually here printf sees "mylo"+'\01' if little endian.

Linus, besides fixing bug, your commit certainly breaks userland compatibility. Please consider.
--
Coywolf Qi Hunt