[Qemu-devel] [PATCH] efault - verify pages are in cache and are read/write

[Qemu-devel] [PATCH] efault - verify pages are in cache and are read/write

[Thayne Harbaugh]

[-- Attachment #1: Type: text/plain, Size: 415 bytes --]

This patch adds the function page_check_range() to verify that pages are
in the cache and that they are appropriately readable/writable.  It also
hooks up access_ok() to page_check_range() so that code patterns are
similar to kernel code.

When copying data from user space access_ok() is used to check that
pages are readable.  When copying data to user space access_ok() is used
to check that pages are writable.

[-- Attachment #2: 06_efault.patch.1.1 --]
[-- Type: text/x-patch, Size: 2306 bytes --]

Index: qemu/exec.c
===================================================================
--- qemu.orig/exec.c	2007-10-31 10:49:10.000000000 -0600
+++ qemu/exec.c	2007-10-31 10:55:50.000000000 -0600
@@ -1875,6 +1875,33 @@
     spin_unlock(&tb_lock);
 }
 
+int page_check_range(target_ulong start, target_ulong len, int flags)
+{
+    PageDesc *p;
+    target_ulong end;
+    target_ulong addr;
+
+    end = TARGET_PAGE_ALIGN(start+len); /* must do before we loose bits in the next step */
+    start = start & TARGET_PAGE_MASK;
+
+    if( end < start )
+        /* we've wrapped around */
+        return -1;
+    for(addr = start; addr < end; addr += TARGET_PAGE_SIZE) {
+        p = page_find(addr >> TARGET_PAGE_BITS);
+        if( !p )
+            return -1;
+        if( !(p->flags & PAGE_VALID) )
+            return -1;
+
+        if (!(p->flags & PAGE_READ) && (flags & PAGE_READ) )
+            return -1;
+        if (!(p->flags & PAGE_WRITE) && (flags & PAGE_WRITE) )
+            return -1;
+    }
+    return 0;
+}
+
 /* called from signal handler: invalidate the code and unprotect the
    page. Return TRUE if the fault was succesfully handled. */
 int page_unprotect(target_ulong address, unsigned long pc, void *puc)
Index: qemu/cpu-all.h
===================================================================
--- qemu.orig/cpu-all.h	2007-10-31 10:49:10.000000000 -0600
+++ qemu/cpu-all.h	2007-10-31 10:55:50.000000000 -0600
@@ -691,6 +691,7 @@
 int page_get_flags(target_ulong address);
 void page_set_flags(target_ulong start, target_ulong end, int flags);
 void page_unprotect_range(target_ulong data, target_ulong data_size);
+int page_check_range(target_ulong start, target_ulong len, int flags);
 
 CPUState *cpu_copy(CPUState *env);
 
Index: qemu/linux-user/qemu.h
===================================================================
--- qemu.orig/linux-user/qemu.h	2007-10-31 10:55:48.000000000 -0600
+++ qemu/linux-user/qemu.h	2007-10-31 10:55:50.000000000 -0600
@@ -185,7 +185,8 @@
 #define VERIFY_READ 0
 #define VERIFY_WRITE 1
 
-#define access_ok(type,addr,size) (1)
+#define access_ok(type,addr,size) \
+    (page_check_range((target_ulong)addr,size,(type==VERIFY_READ)?PAGE_READ:PAGE_WRITE)==0)
 
 /* NOTE get_user and put_user use host addresses.  */
 #define __put_user(x,ptr)\

[Qemu-devel] Re: [PATCH] efault - update __get_user() __put_user()

[Thayne Harbaugh]

[-- Attachment #1: Type: text/plain, Size: 103 bytes --]

This patch is a minor update to __get_user() and __put_user() to
emphasize that they take host points.

[-- Attachment #2: 06_efault.patch.1.2 --]
[-- Type: text/x-patch, Size: 2072 bytes --]

Index: qemu/linux-user/qemu.h
===================================================================
--- qemu.orig/linux-user/qemu.h	2007-10-31 11:03:03.000000000 -0600
+++ qemu/linux-user/qemu.h	2007-10-31 11:04:04.000000000 -0600
@@ -188,22 +188,22 @@
 #define access_ok(type,addr,size) \
     (page_check_range((target_ulong)addr,size,(type==VERIFY_READ)?PAGE_READ:PAGE_WRITE)==0)
 
-/* NOTE get_user and put_user use host addresses.  */
-#define __put_user(x,ptr)\
+/* NOTE __get_user and __put_user use host pointers and don't check access. */
+#define __put_user(x, hptr)\
 ({\
-    int size = sizeof(*ptr);\
+    int size = sizeof(*hptr);\
     switch(size) {\
     case 1:\
-        *(uint8_t *)(ptr) = (typeof(*ptr))(x);\
+        *(uint8_t *)(hptr) = (typeof(*hptr))(x);\
         break;\
     case 2:\
-        *(uint16_t *)(ptr) = tswap16((typeof(*ptr))(x));\
+        *(uint16_t *)(hptr) = tswap16((typeof(*hptr))(x));\
         break;\
     case 4:\
-        *(uint32_t *)(ptr) = tswap32((typeof(*ptr))(x));\
+        *(uint32_t *)(hptr) = tswap32((typeof(*hptr))(x));\
         break;\
     case 8:\
-        *(uint64_t *)(ptr) = tswap64((typeof(*ptr))(x));\
+        *(uint64_t *)(hptr) = tswap64((typeof(*hptr))(x));\
         break;\
     default:\
         abort();\
@@ -211,21 +211,21 @@
     0;\
 })
 
-#define __get_user(x, ptr) \
+#define __get_user(x, hptr) \
 ({\
-    int size = sizeof(*ptr);\
+    int size = sizeof(*hptr);\
     switch(size) {\
     case 1:\
-        x = (typeof(*ptr))*(uint8_t *)(ptr);\
+        x = (typeof(*hptr))*(uint8_t *)(hptr);\
         break;\
     case 2:\
-        x = (typeof(*ptr))tswap16(*(uint16_t *)(ptr));\
+        x = (typeof(*hptr))tswap16(*(uint16_t *)(hptr));\
         break;\
     case 4:\
-        x = (typeof(*ptr))tswap32(*(uint32_t *)(ptr));\
+        x = (typeof(*hptr))tswap32(*(uint32_t *)(hptr));\
         break;\
     case 8:\
-        x = (typeof(*ptr))tswap64(*(uint64_t *)(ptr));\
+        x = (typeof(*hptr))tswap64(*(uint64_t *)(hptr));\
         break;\
     default:\
         abort();\

[Qemu-devel] Re: [PATCH] efault - add data type to put_user()/get_user()

[Thayne Harbaugh]

[-- Attachment #1: Type: text/plain, Size: 657 bytes --]

This patch updates get_user() and put_user() to take a third argument of
data type.  get_user() and put_user() use target address which are
target_ulong and don't reflect the data type pointed to in target
memory.

Simply casting the target_ulong to a type before passing to
get/put_user() is poor because target_ulong isn't always a simple cast
to a host type (consider 32 bit on 64 bit where address are either
extended or truncate).  Also, simple casting of the argument to
get/put_user() results in several warnings when target and long pointer
sizes don't match.

This patch has additional updates to fix places where get/put_user() are
already used.


[-- Attachment #2: 06_efault.patch.1.3 --]
[-- Type: text/x-patch, Size: 10442 bytes --]

Index: qemu/linux-user/qemu.h
===================================================================
--- qemu.orig/linux-user/qemu.h	2007-10-30 07:56:57.000000000 -0600
+++ qemu/linux-user/qemu.h	2007-10-30 17:55:57.000000000 -0600
@@ -230,26 +231,47 @@
     0;\
 })
 
-#define put_user(x,ptr)\
+/* put_user()/get_user() take a guest address and check access */
+#define put_user(x, gaddr, target_type)\
 ({\
     int __ret;\
-    if (access_ok(VERIFY_WRITE, ptr, sizeof(*ptr)))\
-        __ret = __put_user(x, ptr);\
+    if (access_ok(VERIFY_WRITE, gaddr, sizeof(target_type)))\
+        __ret = __put_user(x, (target_type)g2h(gaddr));	\
     else\
-        __ret = -EFAULT;\
+        __ret = -TARGET_EFAULT;\
     __ret;\
 })
 
-#define get_user(x,ptr)\
+#define get_user(x, gaddr, target_type)\
 ({\
     int __ret;\
-    if (access_ok(VERIFY_READ, ptr, sizeof(*ptr)))\
-        __ret = __get_user(x, ptr);\
+    if (access_ok(VERIFY_READ, gaddr, sizeof(target_type)))\
+        __ret = __get_user(x, (target_type)g2h(gaddr));	\
     else\
-        __ret = -EFAULT;\
+        __ret = -TARGET_EFAULT;\
     __ret;\
 })
 
+#define copy_from_user(hptr, gaddr, len)         \
+({                                              \
+    int __cfu_ret=0;                            \
+    if (access_ok(VERIFY_READ, gaddr, (len)))    \
+        memcpy(hptr, g2h(gaddr), (len));         \
+    else                                        \
+        __cfu_ret = -TARGET_EFAULT;             \
+    __cfu_ret;                                  \
+})
+
+#define copy_to_user(gaddr, hptr, len)           \
+({                                              \
+    int __ctu_ret=0;                            \
+    if (access_ok(VERIFY_WRITE, gaddr, (len)))   \
+        memcpy(g2h(gaddr), hptr, (len));         \
+    else                                        \
+        __ctu_ret = -TARGET_EFAULT;             \
+    __ctu_ret;                                  \
+})
+
 /* Functions for accessing guest memory.  The tget and tput functions
    read/write single values, byteswapping as neccessary.  The lock_user
    gets a pointer to a contiguous area of guest memory, but does not perform
Index: qemu/linux-user/syscall.c
===================================================================
--- qemu.orig/linux-user/syscall.c	2007-10-30 07:56:57.000000000 -0600
+++ qemu/linux-user/syscall.c	2007-10-30 17:56:06.000000000 -0600
@@ -1757,7 +1757,7 @@
                 break;
 	    }
 	}
-	if (put_user(raddr, (target_ulong *)third))
+	if (put_user(raddr, third, target_ulong *))
             return -TARGET_EFAULT;
         ret = 0;
 	break;
@@ -3689,17 +3689,16 @@
             struct target_statfs *target_stfs;
 
             lock_user_struct(target_stfs, arg2, 0);
-            /* ??? put_user is probably wrong.  */
-            put_user(stfs.f_type, &target_stfs->f_type);
-            put_user(stfs.f_bsize, &target_stfs->f_bsize);
-            put_user(stfs.f_blocks, &target_stfs->f_blocks);
-            put_user(stfs.f_bfree, &target_stfs->f_bfree);
-            put_user(stfs.f_bavail, &target_stfs->f_bavail);
-            put_user(stfs.f_files, &target_stfs->f_files);
-            put_user(stfs.f_ffree, &target_stfs->f_ffree);
-            put_user(stfs.f_fsid.__val[0], &target_stfs->f_fsid.val[0]);
-            put_user(stfs.f_fsid.__val[1], &target_stfs->f_fsid.val[1]);
-            put_user(stfs.f_namelen, &target_stfs->f_namelen);
+            __put_user(stfs.f_type, &target_stfs->f_type);
+            __put_user(stfs.f_bsize, &target_stfs->f_bsize);
+            __put_user(stfs.f_blocks, &target_stfs->f_blocks);
+            __put_user(stfs.f_bfree, &target_stfs->f_bfree);
+            __put_user(stfs.f_bavail, &target_stfs->f_bavail);
+            __put_user(stfs.f_files, &target_stfs->f_files);
+            __put_user(stfs.f_ffree, &target_stfs->f_ffree);
+            __put_user(stfs.f_fsid.__val[0], &target_stfs->f_fsid.val[0]);
+            __put_user(stfs.f_fsid.__val[1], &target_stfs->f_fsid.val[1]);
+            __put_user(stfs.f_namelen, &target_stfs->f_namelen);
             unlock_user_struct(target_stfs, arg2, 1);
         }
         break;
@@ -3716,17 +3715,16 @@
             struct target_statfs64 *target_stfs;
 
             lock_user_struct(target_stfs, arg3, 0);
-            /* ??? put_user is probably wrong.  */
-            put_user(stfs.f_type, &target_stfs->f_type);
-            put_user(stfs.f_bsize, &target_stfs->f_bsize);
-            put_user(stfs.f_blocks, &target_stfs->f_blocks);
-            put_user(stfs.f_bfree, &target_stfs->f_bfree);
-            put_user(stfs.f_bavail, &target_stfs->f_bavail);
-            put_user(stfs.f_files, &target_stfs->f_files);
-            put_user(stfs.f_ffree, &target_stfs->f_ffree);
-            put_user(stfs.f_fsid.__val[0], &target_stfs->f_fsid.val[0]);
-            put_user(stfs.f_fsid.__val[1], &target_stfs->f_fsid.val[1]);
-            put_user(stfs.f_namelen, &target_stfs->f_namelen);
+            __put_user(stfs.f_type, &target_stfs->f_type);
+            __put_user(stfs.f_bsize, &target_stfs->f_bsize);
+            __put_user(stfs.f_blocks, &target_stfs->f_blocks);
+            __put_user(stfs.f_bfree, &target_stfs->f_bfree);
+            __put_user(stfs.f_bavail, &target_stfs->f_bavail);
+            __put_user(stfs.f_files, &target_stfs->f_files);
+            __put_user(stfs.f_ffree, &target_stfs->f_ffree);
+            __put_user(stfs.f_fsid.__val[0], &target_stfs->f_fsid.val[0]);
+            __put_user(stfs.f_fsid.__val[1], &target_stfs->f_fsid.val[1]);
+            __put_user(stfs.f_namelen, &target_stfs->f_namelen);
             unlock_user_struct(target_stfs, arg3, 0);
         }
         break;
@@ -4465,24 +4463,22 @@
                     struct target_eabi_stat64 *target_st;
                     lock_user_struct(target_st, arg2, 1);
                     memset(target_st, 0, sizeof(struct target_eabi_stat64));
-                    /* put_user is probably wrong.  */
-                    put_user(st.st_dev, &target_st->st_dev);
-                    put_user(st.st_ino, &target_st->st_ino);
+                    __put_user(st.st_dev, &target_st->st_dev);
+                    __put_user(st.st_ino, &target_st->st_ino);
 #ifdef TARGET_STAT64_HAS_BROKEN_ST_INO
-                    put_user(st.st_ino, &target_st->__st_ino);
+                    __put_user(st.st_ino, &target_st->__st_ino);
 #endif
-                    put_user(st.st_mode, &target_st->st_mode);
-                    put_user(st.st_nlink, &target_st->st_nlink);
-                    put_user(st.st_uid, &target_st->st_uid);
-                    put_user(st.st_gid, &target_st->st_gid);
-                    put_user(st.st_rdev, &target_st->st_rdev);
-                    /* XXX: better use of kernel struct */
-                    put_user(st.st_size, &target_st->st_size);
-                    put_user(st.st_blksize, &target_st->st_blksize);
-                    put_user(st.st_blocks, &target_st->st_blocks);
-                    put_user(st.st_atime, &target_st->target_st_atime);
-                    put_user(st.st_mtime, &target_st->target_st_mtime);
-                    put_user(st.st_ctime, &target_st->target_st_ctime);
+                    __put_user(st.st_mode, &target_st->st_mode);
+                    __put_user(st.st_nlink, &target_st->st_nlink);
+                    __put_user(st.st_uid, &target_st->st_uid);
+                    __put_user(st.st_gid, &target_st->st_gid);
+                    __put_user(st.st_rdev, &target_st->st_rdev);
+                    __put_user(st.st_size, &target_st->st_size);
+                    __put_user(st.st_blksize, &target_st->st_blksize);
+                    __put_user(st.st_blocks, &target_st->st_blocks);
+                    __put_user(st.st_atime, &target_st->target_st_atime);
+                    __put_user(st.st_mtime, &target_st->target_st_mtime);
+                    __put_user(st.st_ctime, &target_st->target_st_ctime);
                     unlock_user_struct(target_st, arg2, 0);
                 } else
 #endif
@@ -4490,24 +4486,23 @@
                     struct target_stat64 *target_st;
                     lock_user_struct(target_st, arg2, 1);
                     memset(target_st, 0, sizeof(struct target_stat64));
-                    /* ??? put_user is probably wrong.  */
-                    put_user(st.st_dev, &target_st->st_dev);
-                    put_user(st.st_ino, &target_st->st_ino);
+                    __put_user(st.st_dev, &target_st->st_dev);
+                    __put_user(st.st_ino, &target_st->st_ino);
 #ifdef TARGET_STAT64_HAS_BROKEN_ST_INO
-                    put_user(st.st_ino, &target_st->__st_ino);
+                    __put_user(st.st_ino, &target_st->__st_ino);
 #endif
-                    put_user(st.st_mode, &target_st->st_mode);
-                    put_user(st.st_nlink, &target_st->st_nlink);
-                    put_user(st.st_uid, &target_st->st_uid);
-                    put_user(st.st_gid, &target_st->st_gid);
-                    put_user(st.st_rdev, &target_st->st_rdev);
+                    __put_user(st.st_mode, &target_st->st_mode);
+                    __put_user(st.st_nlink, &target_st->st_nlink);
+                    __put_user(st.st_uid, &target_st->st_uid);
+                    __put_user(st.st_gid, &target_st->st_gid);
+                    __put_user(st.st_rdev, &target_st->st_rdev);
                     /* XXX: better use of kernel struct */
-                    put_user(st.st_size, &target_st->st_size);
-                    put_user(st.st_blksize, &target_st->st_blksize);
-                    put_user(st.st_blocks, &target_st->st_blocks);
-                    put_user(st.st_atime, &target_st->target_st_atime);
-                    put_user(st.st_mtime, &target_st->target_st_mtime);
-                    put_user(st.st_ctime, &target_st->target_st_ctime);
+                    __put_user(st.st_size, &target_st->st_size);
+                    __put_user(st.st_blksize, &target_st->st_blksize);
+                    __put_user(st.st_blocks, &target_st->st_blocks);
+                    __put_user(st.st_atime, &target_st->target_st_atime);
+                    __put_user(st.st_mtime, &target_st->target_st_mtime);
+                    __put_user(st.st_ctime, &target_st->target_st_ctime);
                     unlock_user_struct(target_st, arg2, 0);
                 }
             }

[Qemu-devel] Re: [PATCH] efault - add data type to put_user()/get_user()

[Thayne Harbaugh]

[-- Attachment #1: Type: text/plain, Size: 852 bytes --]


On Wed, 2007-10-31 at 16:44 -0600, Thayne Harbaugh wrote:
> This patch updates get_user() and put_user() to take a third argument of
> data type.  get_user() and put_user() use target address which are
> target_ulong and don't reflect the data type pointed to in target
> memory.
> 
> Simply casting the target_ulong to a type before passing to
> get/put_user() is poor because target_ulong isn't always a simple cast
> to a host type (consider 32 bit on 64 bit where address are either
> extended or truncate).  Also, simple casting of the argument to
> get/put_user() results in several warnings when target and long pointer
> sizes don't match.
> 
> This patch has additional updates to fix places where get/put_user() are
> already used.

This is an updated patch that doesn't conflict with the
abi_long/abi_ulong changes from a couple weeks ago.

[-- Attachment #2: 06_efault.patch.1.3 --]
[-- Type: text/x-patch, Size: 10443 bytes --]

Index: qemu/linux-user/qemu.h
===================================================================
--- qemu.orig/linux-user/qemu.h	2007-10-30 07:56:57.000000000 -0600
+++ qemu/linux-user/qemu.h	2007-10-30 17:55:57.000000000 -0600
@@ -230,26 +231,47 @@
     0;\
 })
 
-#define put_user(x,ptr)\
+/* put_user()/get_user() take a guest address and check access */
+#define put_user(x, gaddr, target_type)\
 ({\
     int __ret;\
-    if (access_ok(VERIFY_WRITE, ptr, sizeof(*ptr)))\
-        __ret = __put_user(x, ptr);\
+    if (access_ok(VERIFY_WRITE, gaddr, sizeof(target_type)))\
+        __ret = __put_user(x, (target_type)g2h(gaddr));	\
     else\
-        __ret = -EFAULT;\
+        __ret = -TARGET_EFAULT;\
     __ret;\
 })
 
-#define get_user(x,ptr)\
+#define get_user(x, gaddr, target_type)\
 ({\
     int __ret;\
-    if (access_ok(VERIFY_READ, ptr, sizeof(*ptr)))\
-        __ret = __get_user(x, ptr);\
+    if (access_ok(VERIFY_READ, gaddr, sizeof(target_type)))\
+        __ret = __get_user(x, (target_type)g2h(gaddr));	\
     else\
-        __ret = -EFAULT;\
+        __ret = -TARGET_EFAULT;\
     __ret;\
 })
 
+#define copy_from_user(hptr, gaddr, len)         \
+({                                              \
+    int __cfu_ret=0;                            \
+    if (access_ok(VERIFY_READ, gaddr, (len)))    \
+        memcpy(hptr, g2h(gaddr), (len));         \
+    else                                        \
+        __cfu_ret = -TARGET_EFAULT;             \
+    __cfu_ret;                                  \
+})
+
+#define copy_to_user(gaddr, hptr, len)           \
+({                                              \
+    int __ctu_ret=0;                            \
+    if (access_ok(VERIFY_WRITE, gaddr, (len)))   \
+        memcpy(g2h(gaddr), hptr, (len));         \
+    else                                        \
+        __ctu_ret = -TARGET_EFAULT;             \
+    __ctu_ret;                                  \
+})
+
 /* Functions for accessing guest memory.  The tget and tput functions
    read/write single values, byteswapping as neccessary.  The lock_user
    gets a pointer to a contiguous area of guest memory, but does not perform
Index: qemu/linux-user/syscall.c
===================================================================
--- qemu.orig/linux-user/syscall.c	2007-10-30 07:56:57.000000000 -0600
+++ qemu/linux-user/syscall.c	2007-10-30 17:56:06.000000000 -0600
@@ -1757,7 +1757,7 @@
                 break;
 	    }
 	}
-	if (put_user(raddr, (abi_ulong *)third))
+        if (put_user(raddr, third, abi_ulong *))
             return -TARGET_EFAULT;
         ret = 0;
 	break;
@@ -3689,17 +3689,16 @@
             struct target_statfs *target_stfs;
 
             lock_user_struct(target_stfs, arg2, 0);
-            /* ??? put_user is probably wrong.  */
-            put_user(stfs.f_type, &target_stfs->f_type);
-            put_user(stfs.f_bsize, &target_stfs->f_bsize);
-            put_user(stfs.f_blocks, &target_stfs->f_blocks);
-            put_user(stfs.f_bfree, &target_stfs->f_bfree);
-            put_user(stfs.f_bavail, &target_stfs->f_bavail);
-            put_user(stfs.f_files, &target_stfs->f_files);
-            put_user(stfs.f_ffree, &target_stfs->f_ffree);
-            put_user(stfs.f_fsid.__val[0], &target_stfs->f_fsid.val[0]);
-            put_user(stfs.f_fsid.__val[1], &target_stfs->f_fsid.val[1]);
-            put_user(stfs.f_namelen, &target_stfs->f_namelen);
+            __put_user(stfs.f_type, &target_stfs->f_type);
+            __put_user(stfs.f_bsize, &target_stfs->f_bsize);
+            __put_user(stfs.f_blocks, &target_stfs->f_blocks);
+            __put_user(stfs.f_bfree, &target_stfs->f_bfree);
+            __put_user(stfs.f_bavail, &target_stfs->f_bavail);
+            __put_user(stfs.f_files, &target_stfs->f_files);
+            __put_user(stfs.f_ffree, &target_stfs->f_ffree);
+            __put_user(stfs.f_fsid.__val[0], &target_stfs->f_fsid.val[0]);
+            __put_user(stfs.f_fsid.__val[1], &target_stfs->f_fsid.val[1]);
+            __put_user(stfs.f_namelen, &target_stfs->f_namelen);
             unlock_user_struct(target_stfs, arg2, 1);
         }
         break;
@@ -3716,17 +3715,16 @@
             struct target_statfs64 *target_stfs;
 
             lock_user_struct(target_stfs, arg3, 0);
-            /* ??? put_user is probably wrong.  */
-            put_user(stfs.f_type, &target_stfs->f_type);
-            put_user(stfs.f_bsize, &target_stfs->f_bsize);
-            put_user(stfs.f_blocks, &target_stfs->f_blocks);
-            put_user(stfs.f_bfree, &target_stfs->f_bfree);
-            put_user(stfs.f_bavail, &target_stfs->f_bavail);
-            put_user(stfs.f_files, &target_stfs->f_files);
-            put_user(stfs.f_ffree, &target_stfs->f_ffree);
-            put_user(stfs.f_fsid.__val[0], &target_stfs->f_fsid.val[0]);
-            put_user(stfs.f_fsid.__val[1], &target_stfs->f_fsid.val[1]);
-            put_user(stfs.f_namelen, &target_stfs->f_namelen);
+            __put_user(stfs.f_type, &target_stfs->f_type);
+            __put_user(stfs.f_bsize, &target_stfs->f_bsize);
+            __put_user(stfs.f_blocks, &target_stfs->f_blocks);
+            __put_user(stfs.f_bfree, &target_stfs->f_bfree);
+            __put_user(stfs.f_bavail, &target_stfs->f_bavail);
+            __put_user(stfs.f_files, &target_stfs->f_files);
+            __put_user(stfs.f_ffree, &target_stfs->f_ffree);
+            __put_user(stfs.f_fsid.__val[0], &target_stfs->f_fsid.val[0]);
+            __put_user(stfs.f_fsid.__val[1], &target_stfs->f_fsid.val[1]);
+            __put_user(stfs.f_namelen, &target_stfs->f_namelen);
             unlock_user_struct(target_stfs, arg3, 0);
         }
         break;
@@ -4465,24 +4463,22 @@
                     struct target_eabi_stat64 *target_st;
                     lock_user_struct(target_st, arg2, 1);
                     memset(target_st, 0, sizeof(struct target_eabi_stat64));
-                    /* put_user is probably wrong.  */
-                    put_user(st.st_dev, &target_st->st_dev);
-                    put_user(st.st_ino, &target_st->st_ino);
+                    __put_user(st.st_dev, &target_st->st_dev);
+                    __put_user(st.st_ino, &target_st->st_ino);
 #ifdef TARGET_STAT64_HAS_BROKEN_ST_INO
-                    put_user(st.st_ino, &target_st->__st_ino);
+                    __put_user(st.st_ino, &target_st->__st_ino);
 #endif
-                    put_user(st.st_mode, &target_st->st_mode);
-                    put_user(st.st_nlink, &target_st->st_nlink);
-                    put_user(st.st_uid, &target_st->st_uid);
-                    put_user(st.st_gid, &target_st->st_gid);
-                    put_user(st.st_rdev, &target_st->st_rdev);
-                    /* XXX: better use of kernel struct */
-                    put_user(st.st_size, &target_st->st_size);
-                    put_user(st.st_blksize, &target_st->st_blksize);
-                    put_user(st.st_blocks, &target_st->st_blocks);
-                    put_user(st.st_atime, &target_st->target_st_atime);
-                    put_user(st.st_mtime, &target_st->target_st_mtime);
-                    put_user(st.st_ctime, &target_st->target_st_ctime);
+                    __put_user(st.st_mode, &target_st->st_mode);
+                    __put_user(st.st_nlink, &target_st->st_nlink);
+                    __put_user(st.st_uid, &target_st->st_uid);
+                    __put_user(st.st_gid, &target_st->st_gid);
+                    __put_user(st.st_rdev, &target_st->st_rdev);
+                    __put_user(st.st_size, &target_st->st_size);
+                    __put_user(st.st_blksize, &target_st->st_blksize);
+                    __put_user(st.st_blocks, &target_st->st_blocks);
+                    __put_user(st.st_atime, &target_st->target_st_atime);
+                    __put_user(st.st_mtime, &target_st->target_st_mtime);
+                    __put_user(st.st_ctime, &target_st->target_st_ctime);
                     unlock_user_struct(target_st, arg2, 0);
                 } else
 #endif
@@ -4490,24 +4486,23 @@
                     struct target_stat64 *target_st;
                     lock_user_struct(target_st, arg2, 1);
                     memset(target_st, 0, sizeof(struct target_stat64));
-                    /* ??? put_user is probably wrong.  */
-                    put_user(st.st_dev, &target_st->st_dev);
-                    put_user(st.st_ino, &target_st->st_ino);
+                    __put_user(st.st_dev, &target_st->st_dev);
+                    __put_user(st.st_ino, &target_st->st_ino);
 #ifdef TARGET_STAT64_HAS_BROKEN_ST_INO
-                    put_user(st.st_ino, &target_st->__st_ino);
+                    __put_user(st.st_ino, &target_st->__st_ino);
 #endif
-                    put_user(st.st_mode, &target_st->st_mode);
-                    put_user(st.st_nlink, &target_st->st_nlink);
-                    put_user(st.st_uid, &target_st->st_uid);
-                    put_user(st.st_gid, &target_st->st_gid);
-                    put_user(st.st_rdev, &target_st->st_rdev);
+                    __put_user(st.st_mode, &target_st->st_mode);
+                    __put_user(st.st_nlink, &target_st->st_nlink);
+                    __put_user(st.st_uid, &target_st->st_uid);
+                    __put_user(st.st_gid, &target_st->st_gid);
+                    __put_user(st.st_rdev, &target_st->st_rdev);
                     /* XXX: better use of kernel struct */
-                    put_user(st.st_size, &target_st->st_size);
-                    put_user(st.st_blksize, &target_st->st_blksize);
-                    put_user(st.st_blocks, &target_st->st_blocks);
-                    put_user(st.st_atime, &target_st->target_st_atime);
-                    put_user(st.st_mtime, &target_st->target_st_mtime);
-                    put_user(st.st_ctime, &target_st->target_st_ctime);
+                    __put_user(st.st_size, &target_st->st_size);
+                    __put_user(st.st_blksize, &target_st->st_blksize);
+                    __put_user(st.st_blocks, &target_st->st_blocks);
+                    __put_user(st.st_atime, &target_st->target_st_atime);
+                    __put_user(st.st_mtime, &target_st->target_st_mtime);
+                    __put_user(st.st_ctime, &target_st->target_st_ctime);
                     unlock_user_struct(target_st, arg2, 0);
                 }
             }

Re: [Qemu-devel] Re: [PATCH] efault - add data type to put_user()/get_user()

[Thiemo Seufer]

Thayne Harbaugh wrote:
> 
> On Wed, 2007-10-31 at 16:44 -0600, Thayne Harbaugh wrote:
> > This patch updates get_user() and put_user() to take a third argument of
> > data type.  get_user() and put_user() use target address which are
> > target_ulong and don't reflect the data type pointed to in target
> > memory.
> > 
> > Simply casting the target_ulong to a type before passing to
> > get/put_user() is poor because target_ulong isn't always a simple cast
> > to a host type (consider 32 bit on 64 bit where address are either
> > extended or truncate).  Also, simple casting of the argument to
> > get/put_user() results in several warnings when target and long pointer
> > sizes don't match.
> > 
> > This patch has additional updates to fix places where get/put_user() are
> > already used.
> 
> This is an updated patch that doesn't conflict with the
> abi_long/abi_ulong changes from a couple weeks ago.

Still misses the sparc64 bits in linux-user/signal.c.


Thiemo

Re: [Qemu-devel] Re: [PATCH] efault - add data type to put_user()/get_user()

[Fabrice Bellard]

I think that using host addresses in __put_user and __get_user is not
logical. They should use target addresses as get_user and put_user. As
Paul said, It is not worth mixing get/put/copy and lock/unlock functions.

The ultimate goal of such cleanup is not only to generate -EFAULT
correctly but also to be able to have arbitrary address space changes.

In fact it would be good to be able to introduce an arbitrary address
space change (such as a translation as Paul did) so that we can verify
that all the Linux emulation stills works in this case.

Regards,

Fabrice.

Thayne Harbaugh wrote:
> On Wed, 2007-10-31 at 16:44 -0600, Thayne Harbaugh wrote:
>> This patch updates get_user() and put_user() to take a third argument of
>> data type.  get_user() and put_user() use target address which are
>> target_ulong and don't reflect the data type pointed to in target
>> memory.
>>
>> Simply casting the target_ulong to a type before passing to
>> get/put_user() is poor because target_ulong isn't always a simple cast
>> to a host type (consider 32 bit on 64 bit where address are either
>> extended or truncate).  Also, simple casting of the argument to
>> get/put_user() results in several warnings when target and long pointer
>> sizes don't match.
>>
>> This patch has additional updates to fix places where get/put_user() are
>> already used.
> 
> This is an updated patch that doesn't conflict with the
> abi_long/abi_ulong changes from a couple weeks ago.

Re: [Qemu-devel] Re: [PATCH] efault - add data type to put_user()/get_user()

[Thayne Harbaugh]

On Sat, 2007-11-03 at 20:05 +0100, Fabrice Bellard wrote:
> I think that using host addresses in __put_user and __get_user is not
> logical. They should use target addresses as get_user and put_user. As
> Paul said, It is not worth mixing get/put/copy and lock/unlock functions.

Please see the "RFC: x86_64 Best way to fix 'cast to pointer'" email for
some discussion of get/put/copy and lock/unlock.  {get,put}_user() is
used for individual ints or other atomically writable types that are
passed as pointers into a syscall.  copy_{to,from}_user_<struct>() are
used for structures that are passed to a syscall.  lock/unlock() will be
used internally in these because lock/unlock does address translation.
lock/unlock() are still needed and are independent.  __{get,put}_user()
will operate internally in these functions on structure data members
where lock/unlock() access_ok() have already been called.

> The ultimate goal of such cleanup is not only to generate -EFAULT
> correctly but also to be able to have arbitrary address space changes.

Yes.  This will be possible once all my clean-ups are pushed.

> In fact it would be good to be able to introduce an arbitrary address
> space change (such as a translation as Paul did) so that we can verify
> that all the Linux emulation stills works in this case.

I'll be testing this way.

> Regards,
> 
> Fabrice.
> 
> Thayne Harbaugh wrote:
> > On Wed, 2007-10-31 at 16:44 -0600, Thayne Harbaugh wrote:
> >> This patch updates get_user() and put_user() to take a third argument of
> >> data type.  get_user() and put_user() use target address which are
> >> target_ulong and don't reflect the data type pointed to in target
> >> memory.
> >>
> >> Simply casting the target_ulong to a type before passing to
> >> get/put_user() is poor because target_ulong isn't always a simple cast
> >> to a host type (consider 32 bit on 64 bit where address are either
> >> extended or truncate).  Also, simple casting of the argument to
> >> get/put_user() results in several warnings when target and long pointer
> >> sizes don't match.
> >>
> >> This patch has additional updates to fix places where get/put_user() are
> >> already used.
> > 
> > This is an updated patch that doesn't conflict with the
> > abi_long/abi_ulong changes from a couple weeks ago.

Re: [Qemu-devel] Re: [PATCH] efault - add data type to put_user()/get_user()

[Thayne Harbaugh]

Uhhh, I'm quite uncomfortable now.  After sending the emails describing
how everything should be done I realized that I had never reworked my
base patches.  All my higher-level patches are sound, but I never
reworked my {get,put}_user() and copy_{to,from}_user() patches to follow
the same pattern.

Fixes are short coming.

Thanks for your patience.

Re: [Qemu-devel] Re: [PATCH] efault - add data type to put_user()/get_user()

[Fabrice Bellard]

Thayne Harbaugh wrote:
> On Sat, 2007-11-03 at 20:05 +0100, Fabrice Bellard wrote:
>> I think that using host addresses in __put_user and __get_user is not
>> logical. They should use target addresses as get_user and put_user. As
>> Paul said, It is not worth mixing get/put/copy and lock/unlock functions.
> 
> Please see the "RFC: x86_64 Best way to fix 'cast to pointer'" email for
> some discussion of get/put/copy and lock/unlock.  {get,put}_user() is
> used for individual ints or other atomically writable types that are
> passed as pointers into a syscall.  copy_{to,from}_user_<struct>() are
> used for structures that are passed to a syscall.  lock/unlock() will be
> used internally in these because lock/unlock does address translation.
> lock/unlock() are still needed and are independent.  __{get,put}_user()
> will operate internally in these functions on structure data members
> where lock/unlock() access_ok() have already been called.

I believed I was clear : once you keep lock/unlock, there is no point in
modifying the code to use put_user/get_user and copy[to,from]_user.

So either you keep the code as it is and extend lock and unlock to
return an error code or you suppress all lock/unlock to use a Linux like
API (i.e. put_user/get_user , copy[to,from]_user, access_ok,
__put_user/__get_user).

So for gettimeofday, if we exclude the fact that the conversion of
struct timeval will be factorized, you have either:

        {
            struct timeval tv;
            ret = get_errno(gettimeofday(&tv, NULL));
            if (!is_error(ret)) {
		    struct target_timeval *target_tv;

		    lock_user_struct(target_tv, arg1, 0);
		    target_tv->tv_sec = tswapl(tv->tv_sec);
		    target_tv->tv_usec = tswapl(tv->tv_usec);
		    if (unlock_user_struct(target_tv, arg1, 1)) {
			ret = -TARGET_EFAULT;
			goto fail;
	            }
            }
        }

Or

        {
            struct timeval tv;
            ret = get_errno(gettimeofday(&tv, NULL));
            if (!is_error(ret)) {
		    struct target_timeval target_tv;

		    target_tv.tv_sec = tswapl(tv->tv_sec);
		    target_tv.tv_usec = tswapl(tv->tv_usec);
		    if (copy_to_user(arg1, &target_tv, sizeof(target_tv)) {
			ret = -TARGET_EFAULT;
			goto fail;
	            }
            }
        }

Personally I prefer the Linux API style, but Paul's lock/unlock is also
perfectly logical. The advantage of keeping the Paul's API is that you
can minimize the number of changes.

Another point is that if you use the Linux API style, it is not needed
to change the API as you want to do. The only useful addition I see is
to add an automatic tswap in get/put/__get/__put user as it is done now.

Moreover, it may be questionnable to do the page right check in
access_ok(). The Linux kernel does not do it at that point : access_ok()
only verifies that the address is in the user address space.

> [...]

Fabrice.

Re: [Qemu-devel] Re: [PATCH] efault - add data type to put_user()/get_user()

[Thayne Harbaugh]

[-- Attachment #1: Type: text/plain, Size: 6100 bytes --]


On Mon, 2007-11-05 at 22:42 +0100, Fabrice Bellard wrote:
> Thayne Harbaugh wrote:
> > On Sat, 2007-11-03 at 20:05 +0100, Fabrice Bellard wrote:
> >> I think that using host addresses in __put_user and __get_user is not
> >> logical. They should use target addresses as get_user and put_user. As
> >> Paul said, It is not worth mixing get/put/copy and lock/unlock functions.
> > 
> > Please see the "RFC: x86_64 Best way to fix 'cast to pointer'" email for
> > some discussion of get/put/copy and lock/unlock.  {get,put}_user() is
> > used for individual ints or other atomically writable types that are
> > passed as pointers into a syscall.  copy_{to,from}_user_<struct>() are
> > used for structures that are passed to a syscall.  lock/unlock() will be
> > used internally in these because lock/unlock does address translation.
> > lock/unlock() are still needed and are independent.  __{get,put}_user()
> > will operate internally in these functions on structure data members
> > where lock/unlock() access_ok() have already been called.
> 
> I believed I was clear : once you keep lock/unlock, there is no point in
> modifying the code to use put_user/get_user and copy[to,from]_user.

without lock/unlock how do you propose that target/host address
translation be performed?  Are you suggesting a g2h() inside of
copy_{to,from}_user()?

> So either you keep the code as it is and extend lock and unlock to
> return an error code or you suppress all lock/unlock to use a Linux like
> API (i.e. put_user/get_user , copy[to,from]_user, access_ok,
> __put_user/__get_user).

The error code because lock/unlock_user would then call access_ok()?

> So for gettimeofday, if we exclude the fact that the conversion of
> struct timeval will be factorized, you have either:
> 
>         {
>             struct timeval tv;
>             ret = get_errno(gettimeofday(&tv, NULL));
>             if (!is_error(ret)) {
> 		    struct target_timeval *target_tv;
> 
> 		    lock_user_struct(target_tv, arg1, 0);
> 		    target_tv->tv_sec = tswapl(tv->tv_sec);
> 		    target_tv->tv_usec = tswapl(tv->tv_usec);
> 		    if (unlock_user_struct(target_tv, arg1, 1)) {
> 			ret = -TARGET_EFAULT;
> 			goto fail;
> 	            }
>             }
>         }
> 
> Or
> 
>         {
>             struct timeval tv;
>             ret = get_errno(gettimeofday(&tv, NULL));
>             if (!is_error(ret)) {
> 		    struct target_timeval target_tv;
> 
> 		    target_tv.tv_sec = tswapl(tv->tv_sec);
> 		    target_tv.tv_usec = tswapl(tv->tv_usec);
> 		    if (copy_to_user(arg1, &target_tv, sizeof(target_tv)) {
> 			ret = -TARGET_EFAULT;
> 			goto fail;
> 	            }
>             }
>         }

I don't see where the second one is handling target/host address
translation.

A problem with both of the above examples is that they use tswapl().
Without the proper type casting tswapl() doesn't do proper sign
extension when dealing with 64bit/32bit host/target relationships.  I've
fixed more than one location where this has resulted in bugs.  What I'm
suggesting is the following:

static inline abi_long copy_to_user_timeval(abi_ulong target_timeval_addr,
                                            const struct timeval *tv)
{
    if (!access_ok(VERIFY_WRITE, target_tv_addr, sizeof(*target_tv)))
        return -TARGET_EFAULT;

    lock_user_struct(target_tv, target_tv_addr, 0);

    __put_user(tv->tv_sec, &target_tv->tv_sec);
    __put_user(tv->tv_usec, &target_tv->tv_usec);

    unlock_user_struct(target_tv, target_tv_addr, 1);

    return 0;
}

.
.
.
        {
            struct timeval tv;
            ret = get_errno(gettimeofday(&tv, NULL));
            if (!is_error(ret)) {
                if (copy_to_user_timeval(arg1, &tv)) {
                    ret = -TARGET_EFAULT;
                    goto fail;
	        }
            }
        }

(Ignoring the factorization of the timeval struct)
copy_to_user_timeval() here properly handles target/host address
translation.  It also uses __put_user() which properly handles any sign
extension.  The difference between the main syscall code and the linux
kernel is simply this:

copy_to_user(arg1, &tv, sizeof(tv)) -> copy_to_user_timeval(arg1, &tv)
                      ^^^^^^^^^^^^                 ^^^^^^^^

Allowing that minor difference (since qemu needs to do translation
between the structure types) is reasonable.  Furthermore the access_ok()
is kept inside the copy_to_user*() function just like in the linux
kernel.  The construct I've shown above is very comparable to kernel
code.


> Personally I prefer the Linux API style, but Paul's lock/unlock is also
> perfectly logical. The advantage of keeping the Paul's API is that you
> can minimize the number of changes.

Sure, there are advantages to minimal changes.  Personally I prefer the
Linux API style because that's what is being emulated.

> Another point is that if you use the Linux API style, it is not needed
> to change the API as you want to do. The only useful addition I see is
> to add an automatic tswap in get/put/__get/__put user as it is done now.

Why would a tswap() need to be added?  It's already inside of
__{get,put}_user()?  I'm a bit confused since the changes I'm making
don't deviate from the intentions of the kernel code and my changes will
address address translation and type casting as are necessary for qemu.

I think that the big problem is that the current patches in my tree are
different than what Stuart originally sent and the initial three patches
that I sent I hadn't gone back to add the address translation to
{get,put}_user() (yes, it's my fault, I should have paid more
attention).  I've attached a newer patch which reflects what I'm doing
at the higher level (it still doesn't address the sparc64 code, though).

> Moreover, it may be questionnable to do the page right check in

s/right/write/; ?

> access_ok(). The Linux kernel does not do it at that point : access_ok()
> only verifies that the address is in the user address space.

You're right - I don't see that any of the archs use the access type
argument.

> > [...]
> 
> Fabrice.

[-- Attachment #2: 06_efault.patch.1.3 --]
[-- Type: text/x-patch, Size: 11602 bytes --]

Index: qemu/linux-user/qemu.h
===================================================================
--- qemu.orig/linux-user/qemu.h	2007-11-03 12:36:40.000000000 -0600
+++ qemu/linux-user/qemu.h	2007-11-05 14:24:17.000000000 -0700
@@ -251,24 +251,63 @@
     0;\
 })
 
-#define put_user(x,ptr)\
-({\
-    int __ret;\
-    if (access_ok(VERIFY_WRITE, ptr, sizeof(*ptr)))\
-        __ret = __put_user(x, ptr);\
-    else\
-        __ret = -EFAULT;\
-    __ret;\
+/* put_user()/get_user() take a guest address and check access */
+#define put_user(x, gaddr, target_type)					\
+({									\
+    abi_ulong __gaddr = (gaddr);					\
+    target_type __hptr;							\
+    abi_long __ret;							\
+    if (access_ok(VERIFY_WRITE, __gaddr, sizeof(target_type))) {	\
+        __hptr = lock_user(__gaddr, sizeof(target_type), 0);		\
+        __ret = __put_user((x), __hptr);				\
+        unlock_user(__hptr, __gaddr, sizeof(target_type));		\
+    } else								\
+        __ret = -TARGET_EFAULT;						\
+    __ret;								\
 })
 
-#define get_user(x,ptr)\
-({\
-    int __ret;\
-    if (access_ok(VERIFY_READ, ptr, sizeof(*ptr)))\
-        __ret = __get_user(x, ptr);\
-    else\
-        __ret = -EFAULT;\
-    __ret;\
+#define get_user(x, gaddr, target_type)				\
+({								\
+    abi_ulong __gaddr = (gaddr);				\
+    target_type __hptr;						\
+    abi_long __ret;						\
+    if (access_ok(VERIFY_READ, __gaddr, sizeof(target_type))) {	\
+        __hptr = lock_user(__gaddr, sizeof(target_type), 1);	\
+        __ret = __get_user((x), __hptr);			\
+        unlock_user(__hptr, __gaddr, 0);			\
+    } else							\
+        __ret = -TARGET_EFAULT;					\
+    __ret;							\
+})
+
+#define copy_from_user(hptr, gaddr, len)	\
+({                                              \
+    abi_ulong __gaddr = (gaddr);		\
+    void *__hptr;				\
+    long __len = len;				\
+    abi_long __cfu_ret=0;			\
+    if (access_ok(VERIFY_READ, __gaddr, len)) {	\
+        __hptr = lock_user(__gaddr, len, 1);	\
+        memcpy((hptr), __hptr, len);		\
+        unlock_user(__hptr, __gaddr, 0);	\
+    } else					\
+        __cfu_ret = -TARGET_EFAULT;             \
+    __cfu_ret;                                  \
+})
+
+#define copy_to_user(gaddr, hptr, len)			\
+({							\
+    abi_ulong __gaddr = (gaddr);			\
+    void *__hptr;					\
+    long __len = len;					\
+    abi_long __ctu_ret=0;				\
+    if (access_ok(VERIFY_WRITE, __gaddr, len)) {	\
+        __hptr = lock_user(__gaddr, len, 0);		\
+        memcpy(__hptr, (hptr), len);			\
+        unlock_user(__hptr, __gaddr, len);		\
+    } else						\
+        __ctu_ret = -TARGET_EFAULT;			\
+    __ctu_ret;						\
 })
 
 /* Functions for accessing guest memory.  The tget and tput functions
Index: qemu/linux-user/syscall.c
===================================================================
--- qemu.orig/linux-user/syscall.c	2007-11-03 12:36:29.000000000 -0600
+++ qemu/linux-user/syscall.c	2007-11-05 14:36:54.000000000 -0700
@@ -1761,7 +1761,7 @@
                 break;
 	    }
 	}
-	if (put_user(raddr, (abi_ulong *)third))
+        if (put_user(raddr, third, abi_ulong *))
             return -TARGET_EFAULT;
         ret = 0;
 	break;
@@ -3697,18 +3697,20 @@
         if (!is_error(ret)) {
             struct target_statfs *target_stfs;
 
+            if (access_ok(VERIFY_WRITE, arg2, sizeof(*target_stfs)))
+                ret = -TARGET_EFAULT;
+
             lock_user_struct(target_stfs, arg2, 0);
-            /* ??? put_user is probably wrong.  */
-            put_user(stfs.f_type, &target_stfs->f_type);
-            put_user(stfs.f_bsize, &target_stfs->f_bsize);
-            put_user(stfs.f_blocks, &target_stfs->f_blocks);
-            put_user(stfs.f_bfree, &target_stfs->f_bfree);
-            put_user(stfs.f_bavail, &target_stfs->f_bavail);
-            put_user(stfs.f_files, &target_stfs->f_files);
-            put_user(stfs.f_ffree, &target_stfs->f_ffree);
-            put_user(stfs.f_fsid.__val[0], &target_stfs->f_fsid.val[0]);
-            put_user(stfs.f_fsid.__val[1], &target_stfs->f_fsid.val[1]);
-            put_user(stfs.f_namelen, &target_stfs->f_namelen);
+            __put_user(stfs.f_type, &target_stfs->f_type);
+            __put_user(stfs.f_bsize, &target_stfs->f_bsize);
+            __put_user(stfs.f_blocks, &target_stfs->f_blocks);
+            __put_user(stfs.f_bfree, &target_stfs->f_bfree);
+            __put_user(stfs.f_bavail, &target_stfs->f_bavail);
+            __put_user(stfs.f_files, &target_stfs->f_files);
+            __put_user(stfs.f_ffree, &target_stfs->f_ffree);
+            __put_user(stfs.f_fsid.__val[0], &target_stfs->f_fsid.val[0]);
+            __put_user(stfs.f_fsid.__val[1], &target_stfs->f_fsid.val[1]);
+            __put_user(stfs.f_namelen, &target_stfs->f_namelen);
             unlock_user_struct(target_stfs, arg2, 1);
         }
         break;
@@ -3724,18 +3726,20 @@
         if (!is_error(ret)) {
             struct target_statfs64 *target_stfs;
 
+            if (access_ok(VERIFY_WRITE, arg3, sizeof(*target_stfs)))
+                ret = -TARGET_EFAULT;
+
             lock_user_struct(target_stfs, arg3, 0);
-            /* ??? put_user is probably wrong.  */
-            put_user(stfs.f_type, &target_stfs->f_type);
-            put_user(stfs.f_bsize, &target_stfs->f_bsize);
-            put_user(stfs.f_blocks, &target_stfs->f_blocks);
-            put_user(stfs.f_bfree, &target_stfs->f_bfree);
-            put_user(stfs.f_bavail, &target_stfs->f_bavail);
-            put_user(stfs.f_files, &target_stfs->f_files);
-            put_user(stfs.f_ffree, &target_stfs->f_ffree);
-            put_user(stfs.f_fsid.__val[0], &target_stfs->f_fsid.val[0]);
-            put_user(stfs.f_fsid.__val[1], &target_stfs->f_fsid.val[1]);
-            put_user(stfs.f_namelen, &target_stfs->f_namelen);
+            __put_user(stfs.f_type, &target_stfs->f_type);
+            __put_user(stfs.f_bsize, &target_stfs->f_bsize);
+            __put_user(stfs.f_blocks, &target_stfs->f_blocks);
+            __put_user(stfs.f_bfree, &target_stfs->f_bfree);
+            __put_user(stfs.f_bavail, &target_stfs->f_bavail);
+            __put_user(stfs.f_files, &target_stfs->f_files);
+            __put_user(stfs.f_ffree, &target_stfs->f_ffree);
+            __put_user(stfs.f_fsid.__val[0], &target_stfs->f_fsid.val[0]);
+            __put_user(stfs.f_fsid.__val[1], &target_stfs->f_fsid.val[1]);
+            __put_user(stfs.f_namelen, &target_stfs->f_namelen);
             unlock_user_struct(target_stfs, arg3, 0);
         }
         break;
@@ -4472,51 +4476,56 @@
 #ifdef TARGET_ARM
                 if (((CPUARMState *)cpu_env)->eabi) {
                     struct target_eabi_stat64 *target_st;
+
+                    if (access_ok(VERIFY_WRITE, arg2, sizeof(*target_st)))
+                        ret = -TARGET_EFAULT;
+
                     lock_user_struct(target_st, arg2, 1);
                     memset(target_st, 0, sizeof(struct target_eabi_stat64));
-                    /* put_user is probably wrong.  */
-                    put_user(st.st_dev, &target_st->st_dev);
-                    put_user(st.st_ino, &target_st->st_ino);
+                    __put_user(st.st_dev, &target_st->st_dev);
+                    __put_user(st.st_ino, &target_st->st_ino);
 #ifdef TARGET_STAT64_HAS_BROKEN_ST_INO
-                    put_user(st.st_ino, &target_st->__st_ino);
+                    __put_user(st.st_ino, &target_st->__st_ino);
 #endif
-                    put_user(st.st_mode, &target_st->st_mode);
-                    put_user(st.st_nlink, &target_st->st_nlink);
-                    put_user(st.st_uid, &target_st->st_uid);
-                    put_user(st.st_gid, &target_st->st_gid);
-                    put_user(st.st_rdev, &target_st->st_rdev);
-                    /* XXX: better use of kernel struct */
-                    put_user(st.st_size, &target_st->st_size);
-                    put_user(st.st_blksize, &target_st->st_blksize);
-                    put_user(st.st_blocks, &target_st->st_blocks);
-                    put_user(st.st_atime, &target_st->target_st_atime);
-                    put_user(st.st_mtime, &target_st->target_st_mtime);
-                    put_user(st.st_ctime, &target_st->target_st_ctime);
+                    __put_user(st.st_mode, &target_st->st_mode);
+                    __put_user(st.st_nlink, &target_st->st_nlink);
+                    __put_user(st.st_uid, &target_st->st_uid);
+                    __put_user(st.st_gid, &target_st->st_gid);
+                    __put_user(st.st_rdev, &target_st->st_rdev);
+                    __put_user(st.st_size, &target_st->st_size);
+                    __put_user(st.st_blksize, &target_st->st_blksize);
+                    __put_user(st.st_blocks, &target_st->st_blocks);
+                    __put_user(st.st_atime, &target_st->target_st_atime);
+                    __put_user(st.st_mtime, &target_st->target_st_mtime);
+                    __put_user(st.st_ctime, &target_st->target_st_ctime);
                     unlock_user_struct(target_st, arg2, 0);
                 } else
 #endif
                 {
                     struct target_stat64 *target_st;
+
+                    if (access_ok(VERIFY_WRITE, arg2, sizeof(*target_st)))
+                        ret = -TARGET_EFAULT;
+
                     lock_user_struct(target_st, arg2, 1);
                     memset(target_st, 0, sizeof(struct target_stat64));
-                    /* ??? put_user is probably wrong.  */
-                    put_user(st.st_dev, &target_st->st_dev);
-                    put_user(st.st_ino, &target_st->st_ino);
+                    __put_user(st.st_dev, &target_st->st_dev);
+                    __put_user(st.st_ino, &target_st->st_ino);
 #ifdef TARGET_STAT64_HAS_BROKEN_ST_INO
-                    put_user(st.st_ino, &target_st->__st_ino);
+                    __put_user(st.st_ino, &target_st->__st_ino);
 #endif
-                    put_user(st.st_mode, &target_st->st_mode);
-                    put_user(st.st_nlink, &target_st->st_nlink);
-                    put_user(st.st_uid, &target_st->st_uid);
-                    put_user(st.st_gid, &target_st->st_gid);
-                    put_user(st.st_rdev, &target_st->st_rdev);
+                    __put_user(st.st_mode, &target_st->st_mode);
+                    __put_user(st.st_nlink, &target_st->st_nlink);
+                    __put_user(st.st_uid, &target_st->st_uid);
+                    __put_user(st.st_gid, &target_st->st_gid);
+                    __put_user(st.st_rdev, &target_st->st_rdev);
                     /* XXX: better use of kernel struct */
-                    put_user(st.st_size, &target_st->st_size);
-                    put_user(st.st_blksize, &target_st->st_blksize);
-                    put_user(st.st_blocks, &target_st->st_blocks);
-                    put_user(st.st_atime, &target_st->target_st_atime);
-                    put_user(st.st_mtime, &target_st->target_st_mtime);
-                    put_user(st.st_ctime, &target_st->target_st_ctime);
+                    __put_user(st.st_size, &target_st->st_size);
+                    __put_user(st.st_blksize, &target_st->st_blksize);
+                    __put_user(st.st_blocks, &target_st->st_blocks);
+                    __put_user(st.st_atime, &target_st->target_st_atime);
+                    __put_user(st.st_mtime, &target_st->target_st_mtime);
+                    __put_user(st.st_ctime, &target_st->target_st_ctime);
                     unlock_user_struct(target_st, arg2, 0);
                 }
             }

Re: [Qemu-devel] Re: [PATCH] efault - add data type to put_user()/get_user()

[Fabrice Bellard]

Thayne Harbaugh wrote:
> On Mon, 2007-11-05 at 22:42 +0100, Fabrice Bellard wrote:
>> Thayne Harbaugh wrote:
>>> On Sat, 2007-11-03 at 20:05 +0100, Fabrice Bellard wrote:
>>>> I think that using host addresses in __put_user and __get_user is not
>>>> logical. They should use target addresses as get_user and put_user. As
>>>> Paul said, It is not worth mixing get/put/copy and lock/unlock functions.
>>> Please see the "RFC: x86_64 Best way to fix 'cast to pointer'" email for
>>> some discussion of get/put/copy and lock/unlock.  {get,put}_user() is
>>> used for individual ints or other atomically writable types that are
>>> passed as pointers into a syscall.  copy_{to,from}_user_<struct>() are
>>> used for structures that are passed to a syscall.  lock/unlock() will be
>>> used internally in these because lock/unlock does address translation.
>>> lock/unlock() are still needed and are independent.  __{get,put}_user()
>>> will operate internally in these functions on structure data members
>>> where lock/unlock() access_ok() have already been called.
>> I believed I was clear : once you keep lock/unlock, there is no point in
>> modifying the code to use put_user/get_user and copy[to,from]_user.
> 
> without lock/unlock how do you propose that target/host address
> translation be performed?  Are you suggesting a g2h() inside of
> copy_{to,from}_user()?

Yes.

Keep in mind that g2h() is only the simple case in case the target
address space is directly addressable. I don't want that the API makes
this supposition, so in the general case copy_[to,from]user are the only
method you have to exchange data with the user space.

>> So either you keep the code as it is and extend lock and unlock to
>> return an error code or you suppress all lock/unlock to use a Linux like
>> API (i.e. put_user/get_user , copy[to,from]_user, access_ok,
>> __put_user/__get_user).
> 
> The error code because lock/unlock_user would then call access_ok()?

Of course.

>> So for gettimeofday, if we exclude the fact that the conversion of
>> struct timeval will be factorized, you have either:
>>
>>         {
>>             struct timeval tv;
>>             ret = get_errno(gettimeofday(&tv, NULL));
>>             if (!is_error(ret)) {
>> 		    struct target_timeval *target_tv;
>>
>> 		    lock_user_struct(target_tv, arg1, 0);
>> 		    target_tv->tv_sec = tswapl(tv->tv_sec);
>> 		    target_tv->tv_usec = tswapl(tv->tv_usec);
>> 		    if (unlock_user_struct(target_tv, arg1, 1)) {
>> 			ret = -TARGET_EFAULT;
>> 			goto fail;
>> 	            }
>>             }
>>         }
>>
>> Or
>>
>>         {
>>             struct timeval tv;
>>             ret = get_errno(gettimeofday(&tv, NULL));
>>             if (!is_error(ret)) {
>> 		    struct target_timeval target_tv;
>>
>> 		    target_tv.tv_sec = tswapl(tv->tv_sec);
>> 		    target_tv.tv_usec = tswapl(tv->tv_usec);
>> 		    if (copy_to_user(arg1, &target_tv, sizeof(target_tv)) {
>> 			ret = -TARGET_EFAULT;
>> 			goto fail;
>> 	            }
>>             }
>>         }
> 
> I don't see where the second one is handling target/host address
> translation.

copy_to_user() does it.

> A problem with both of the above examples is that they use tswapl().
> Without the proper type casting tswapl() doesn't do proper sign
> extension when dealing with 64bit/32bit host/target relationships.  I've
> fixed more than one location where this has resulted in bugs. 

This is an unrelated problem. If tswapl is not sufficient then another
function can be added.

> [...]

Regards,

Fabrice.

[Qemu-devel] Re: [PATCH] efault

[Thayne Harbaugh]

These three efault patches are the basis for another 30 patches which do
the following:

* Correct compiler warnings.
* Add coding consistency.
* Detect error cases and handle them properly.
* Divide syscall.c to closer resemble the Linux kernel for code
partitioning and organization.
* Add new features and functionality.
* Correct errors from previous patches that I've sent 8^)

I'd appreciate feed back and comments.  When I can make these patches
acceptable then I'll be able to send the additional patches and have
them meet the same design requirements.

Thanks.