lkml.org 
[lkml]   [1998]   [Aug]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patches in this message
/
Date
From
Subjectcopy_from_user() fix
Linus,

copy_from_user() function needs to be fixed to clear out the rest of
kernel memory when it fails to read user's pages.
There are few places in the kernel where uncleared pages may be exposed
to users. I consider it as a very important security problem.

I've produced a patch to fix the problem.
The first part of the patch:
- clears the remaining memory in outline __generic_copy_from_user()
in arch/i386/lib/usercopy.c keeping inline __copy_from_user() without
the extra code;
- makes sure that generic and i386 specific code don't use __copy_from_user()
in a way allowing information leak.
For the last purpose some places were patched:
- 2 ioctls in drivers/char/vt.c to use outline copy_from_user()
and report error in the case of fail;
- arch/i386/kernel/ptrace.c to use outline copy_from_user()
with removing of unnecessary access_ok() because all branches of the code
uses copy_from_user();
- arch/i386/math-emu/reg_ld_str.c to clear the memory after
__copy_from_user() if needed.

The second part of the patch makes dirty changes to copy_from_user()
implementation for other architectures except sparc64 where memory
clearing has already been implemented.
The changes for other architectures are obvious inefficient and untested
and I hope will be reimplemented by the port authors.

I've also prepared a list of kernel files where the result of
copy_from_user() seems to be ignored and it's not clear that a harm isn't
possible because of the absence of the check:
ftp://ftp.nc.orc.ru/pub/Linux/tmp/copy_from_user.search

Best wishes
Andrey V.
Savochkin
diff -ruN linux-2.1.117.orig/arch/i386/lib/usercopy.c linux-2.1.117/arch/i386/lib/usercopy.c
--- linux-2.1.117.orig/arch/i386/lib/usercopy.c Sat Jan 24 03:34:36 1998
+++ linux-2.1.117/arch/i386/lib/usercopy.c Fri Aug 21 17:00:00 1998
@@ -7,7 +7,7 @@
*/
#include <asm/uaccess.h>

-inline unsigned long
+unsigned long
__generic_copy_to_user(void *to, const void *from, unsigned long n)
{
if (access_ok(VERIFY_WRITE, to, n))
@@ -15,12 +15,22 @@
return n;
}

-inline unsigned long
+unsigned long
__generic_copy_from_user(void *to, const void *from, unsigned long n)
{
- if (access_ok(VERIFY_READ, from, n))
- __copy_user(to,from,n);
- return n;
+ unsigned long ret = n;
+ if (!access_ok(VERIFY_READ, from, ret))
+ goto out;
+ __copy_user(to,from,ret);
+ if (!ret)
+ return ret;
+ /* There are some places in the kernel where uncleared kernel
+ * memory becomes available for users because of copy_from_user
+ * fail. It leads to an information leak. We fix it here.
+ */
+ memset((char *)to + n - ret, 0, ret);
+out:
+ return ret;
}


diff -ruN linux-2.1.117.orig/drivers/char/vt.c linux-2.1.117/drivers/char/vt.c
--- linux-2.1.117.orig/drivers/char/vt.c Mon Aug 17 13:01:56 1998
+++ linux-2.1.117/drivers/char/vt.c Fri Aug 21 10:37:34 1998
@@ -637,32 +637,34 @@
case KDGKBDIACR:
{
struct kbdiacrs *a = (struct kbdiacrs *)arg;
+ int error;

- i = verify_area(VERIFY_WRITE, (void *) a, sizeof(struct kbdiacrs));
- if (i)
- return i;
- __put_user(accent_table_size, &a->kb_cnt);
- __copy_to_user(a->kbdiacr, accent_table,
- accent_table_size*sizeof(struct kbdiacr));
- return 0;
+ error = put_user(accent_table_size, &a->kb_cnt);
+ if (error)
+ return error;
+ if (copy_to_user(a->kbdiacr, accent_table,
+ accent_table_size*sizeof(struct kbdiacr)))
+ error = -EFAULT;
+ return error;
}

case KDSKBDIACR:
{
struct kbdiacrs *a = (struct kbdiacrs *)arg;
unsigned int ct;
+ int error;

if (!perm)
return -EPERM;
- i = verify_area(VERIFY_READ, (void *) a, sizeof(struct kbdiacrs));
- if (i)
- return i;
- __get_user(ct,&a->kb_cnt);
+ error = get_user(ct,&a->kb_cnt);
+ if (error)
+ return error;
if (ct >= MAX_DIACR)
return -EINVAL;
accent_table_size = ct;
- __copy_from_user(accent_table, a->kbdiacr, ct*sizeof(struct kbdiacr));
- return 0;
+ if (copy_from_user(accent_table, a->kbdiacr, ct*sizeof(struct kbdiacr)))
+ error = -EFAULT;
+ return error;
}

/* the ioctls below read/set the flags usually shown in the leds */
diff -ruN linux-2.1.117.orig/arch/i386/kernel/ptrace.c linux-2.1.117/arch/i386/kernel/ptrace.c
--- linux-2.1.117.orig/arch/i386/kernel/ptrace.c Thu Jul 30 20:03:33 1998
+++ linux-2.1.117/arch/i386/kernel/ptrace.c Sat Aug 22 08:17:31 1998
@@ -636,28 +636,27 @@
};

case PTRACE_SETFPREGS: { /* Set the child FPU state. */
- if (!access_ok(VERIFY_READ, (unsigned *)data,
- sizeof(struct user_i387_struct)))
- {
- ret = -EIO;
- goto out;
- }
child->used_math = 1;
#ifdef CONFIG_MATH_EMULATION
if ( boot_cpu_data.hard_math ) {
#endif
- __copy_from_user(&child->tss.i387.hard, (void *)data,
- sizeof(struct user_i387_struct));
- child->flags &= ~PF_USEDFPU;
- stts();
+ if (copy_from_user(&child->tss.i387.hard,
+ (void *)data,
+ sizeof(struct user_i387_struct))) {
+ child->flags &= ~PF_USEDFPU;
+ stts();
+ ret = 0;
+ goto out;
+ }
#ifdef CONFIG_MATH_EMULATION
} else {
- restore_i387_soft(&child->tss.i387.soft,
+ ret = restore_i387_soft(&child->tss.i387.soft,
(struct _fpstate *)data);
+ if (!ret)
+ goto out;
}
#endif
- ret = 0;
- goto out;
+ /* fall through */
};

default:
diff -ruN linux-2.1.117.orig/arch/i386/math-emu/reg_ld_str.c linux-2.1.117/arch/i386/math-emu/reg_ld_str.c
--- linux-2.1.117.orig/arch/i386/math-emu/reg_ld_str.c Sun Jan 25 22:01:48 1998
+++ linux-2.1.117/arch/i386/math-emu/reg_ld_str.c Sat Aug 22 08:46:27 1998
@@ -89,10 +89,15 @@
int FPU_load_extended(long double *s, int stnr)
{
FPU_REG *sti_ptr = &st(stnr);
+ unsigned long n;

RE_ENTRANT_CHECK_OFF;
FPU_verify_area(VERIFY_READ, s, 10);
- __copy_from_user(sti_ptr, s, 10);
+ n = __copy_from_user(sti_ptr, s, 10);
+ if ( n ) {
+ memset(sti_ptr + 10 - n, 0, n);
+ math_abort(FPU_info,SIGSEGV);
+ }
RE_ENTRANT_CHECK_ON;

return FPU_tagof(sti_ptr);
@@ -1271,13 +1276,23 @@
int i, regnr;
u_char *s = fldenv(addr_modes, data_address);
int offset = (top & 7) * 10, other = 80 - offset;
+ unsigned long n;

/* Copy all registers in stack order. */
RE_ENTRANT_CHECK_OFF;
FPU_verify_area(VERIFY_READ,s,80);
- __copy_from_user(register_base+offset, s, other);
- if ( offset )
- __copy_from_user(register_base, s+other, offset);
+ n = __copy_from_user(register_base+offset, s, other);
+ if ( n ) {
+ memset(register_base+offset+other-n, 0, n);
+ math_abort(FPU_info,SIGSEGV);
+ }
+ if ( offset ) {
+ n = __copy_from_user(register_base, s+other, offset);
+ if ( n ) {
+ memset(register_base+offset-n, 0, n);
+ math_abort(FPU_info,SIGSEGV);
+ }
+ }
RE_ENTRANT_CHECK_ON;

for ( i = 0; i < 8; i++ )diff -ruN linux-2.1.117.orig/include/asm-alpha/uaccess.h linux-2.1.117/include/asm-alpha/uaccess.h
--- linux-2.1.117.orig/include/asm-alpha/uaccess.h Thu Jun 25 01:30:11 1998
+++ linux-2.1.117/include/asm-alpha/uaccess.h Sat Aug 22 10:29:26 1998
@@ -361,8 +361,39 @@
#define __copy_to_user(to,from,n) __copy_tofrom_user_nocheck((to),(from),(n))
#define __copy_from_user(to,from,n) __copy_tofrom_user_nocheck((to),(from),(n))

-#define copy_to_user(to,from,n) __copy_tofrom_user((to),(from),(n),__cu_to)
-#define copy_from_user(to,from,n) __copy_tofrom_user((to),(from),(n),__cu_from)
+#define copy_to_user(to,from,n) \
+({ \
+ register void * __cu_to __asm__("$6") = (to); \
+ register const void * __cu_from __asm__("$7") = (from); \
+ register long __cu_len __asm__("$0") = (n); \
+ if (__access_ok(((long)(__cu_to)),__cu_len,get_fs())) { \
+ __asm__ __volatile__( \
+ "jsr $28,(%3),__copy_user" \
+ : "=r" (__cu_len), "=r" (__cu_from), "=r" (__cu_to) \
+ : "r" (__copy_user), "0" (__cu_len), \
+ "1" (__cu_from), "2" (__cu_to) \
+ : "$1","$2","$3","$4","$5","$28","memory"); \
+ } \
+ __cu_len; \
+})
+#define copy_from_user(to,from,n) \
+({ \
+ register void * __cu_to __asm__("$6") = (to); \
+ register const void * __cu_from __asm__("$7") = (from); \
+ register long __cu_len __asm__("$0") = (n); \
+ char *__cu_end = (char *)__cu_to + __cu_len; \
+ if (__access_ok(((long)(__cu_from)),__cu_len,get_fs())) { \
+ __asm__ __volatile__( \
+ "jsr $28,(%3),__copy_user" \
+ : "=r" (__cu_len), "=r" (__cu_from), "=r" (__cu_to) \
+ : "r" (__copy_user), "0" (__cu_len), \
+ "1" (__cu_from), "2" (__cu_to) \
+ : "$1","$2","$3","$4","$5","$28","memory"); \
+ if (__cu_len) \
+ memset(__cu_end - __cu_len, 0, __cu_len); \
+ } \
+ __cu_len; \
+})

extern void __copy_user(void);

@@ -378,22 +409,6 @@
"1" (__cu_from), "2" (__cu_to) \
: "$1","$2","$3","$4","$5","$28","memory"); \
__cu_len; \
-})
-
-#define __copy_tofrom_user(to,from,n,v) \
-({ \
- register void * __cu_to __asm__("$6") = (to); \
- register const void * __cu_from __asm__("$7") = (from); \
- register long __cu_len __asm__("$0") = (n); \
- if (__access_ok(((long)(v)),__cu_len,get_fs())) { \
- __asm__ __volatile__( \
- "jsr $28,(%3),__copy_user" \
- : "=r" (__cu_len), "=r" (__cu_from), "=r" (__cu_to) \
- : "r" (__copy_user), "0" (__cu_len), \
- "1" (__cu_from), "2" (__cu_to) \
- : "$1","$2","$3","$4","$5","$28","memory"); \
- } \
- __cu_len; \
})

#define copy_to_user_ret(to,from,n,retval) ({ \
diff -ruN linux-2.1.117.orig/include/asm-arm/uaccess.h linux-2.1.117/include/asm-arm/uaccess.h
--- linux-2.1.117.orig/include/asm-arm/uaccess.h Wed Jan 21 03:39:43 1998
+++ linux-2.1.117/include/asm-arm/uaccess.h Sat Aug 22 10:36:26 1998
@@ -60,8 +60,11 @@

static __inline__ unsigned long copy_from_user(void *to, const void *from, unsigned long n)
{
- if (access_ok(VERIFY_READ, from, n))
+ char *end = (char *)to + n;
+ if (access_ok(VERIFY_READ, from, n)) {
__do_copy_from_user(to, from, n);
+ if (n) memset(end - n, 0, n);
+ }
return n;
}

diff -ruN linux-2.1.117.orig/include/asm-m68k/uaccess.h linux-2.1.117/include/asm-m68k/uaccess.h
--- linux-2.1.117.orig/include/asm-m68k/uaccess.h Fri Aug 21 09:49:43 1998
+++ linux-2.1.117/include/asm-m68k/uaccess.h Sat Aug 22 10:45:21 1998
@@ -712,9 +712,16 @@
}

#define copy_from_user(to, from, n) \
+{ void *__to = (to); \
+ void *__from = (from); \
+ unsigned long __n = (n); \
+ char *__end = (char *)__to + __n; \
+ unsigned long __res = \
(__builtin_constant_p(n) ? \
__constant_copy_from_user(to, from, n) : \
- __generic_copy_from_user(to, from, n))
+ __generic_copy_from_user(to, from, n)); \
+ if (__res) memset(__end - __res, 0, __res); \
+ res; }

#define copy_to_user(to, from, n) \
(__builtin_constant_p(n) ? \
diff -ruN linux-2.1.117.orig/include/asm-ppc/uaccess.h linux-2.1.117/include/asm-ppc/uaccess.h
--- linux-2.1.117.orig/include/asm-ppc/uaccess.h Tue Jan 13 02:18:13 1998
+++ linux-2.1.117/include/asm-ppc/uaccess.h Sat Aug 22 10:50:11 1998
@@ -211,9 +211,12 @@
extern inline unsigned long
copy_from_user(void *to, const void *from, unsigned long n)
{
- if (access_ok(VERIFY_READ, from, n))
- return __copy_tofrom_user(to, from, n);
- return n? -EFAULT: 0;
+ unsigned long res = n;
+ if (access_ok(VERIFY_READ, from, n)) {
+ res = __copy_tofrom_user(to, from, n);
+ if (res) memset((char *)to + n - res, 0, res);
+ }
+ return res;
}

extern inline unsigned long
@@ -221,7 +224,7 @@
{
if (access_ok(VERIFY_WRITE, to, n))
return __copy_tofrom_user(to, from, n);
- return n? -EFAULT: 0;
+ return n;
}

#define __copy_from_user(to, from, size) \
diff -ruN linux-2.1.117.orig/include/asm-sparc/uaccess.h linux-2.1.117/include/asm-sparc/uaccess.h
--- linux-2.1.117.orig/include/asm-sparc/uaccess.h Wed Apr 15 04:44:24 1998
+++ linux-2.1.117/include/asm-sparc/uaccess.h Sat Aug 22 10:34:48 1998
@@ -318,11 +318,14 @@
})

#define copy_from_user(to,from,n) ({ \
+void *__copy_to = (void *) (to); \
void *__copy_from = (void *) (from); \
__kernel_size_t __copy_size = (__kernel_size_t) (n); \
__kernel_size_t __copy_res; \
if(__copy_size && __access_ok((unsigned long)__copy_from, __copy_size)) { \
-__copy_res = __copy_user((void *) (to), __copy_from, __copy_size); \
+__copy_res = __copy_user(__copy_to, __copy_from, __copy_size); \
+if(__copy_res) \
+memset((char *)__copy_to + __copy_size - __copy_res, 0, __copy_res); \
} else __copy_res = __copy_size; \
__copy_res; })
\
 
 \ /
  Last update: 2005-03-22 13:44    [W:0.279 / U:0.100 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site