Re: [PATCH] Fix user data corrupted by old value return of sysctl

[Yi Yang]

Coywolf Qi Hunt wrote:

> 2005/12/30, Yi Yang <yang.y.yi@xxxxxxxxx>:
>  
>
> > 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
> >    
>
> 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.

>  
>
> > target = 1
> >
> > Signed-off-by: Yi Yang <yang.y.yi@xxxxxxxxx>
> >
> >
> > --- a/kernel/sysctl.c.orig      2005-12-30 09:21:34.000000000 +0000
> > +++ b/kernel/sysctl.c   2005-12-30 15:58:15.000000000 +0000
> > @@ -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
>
>  

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/