[Qemu-devel] [PATCH v2 05/16] linux-user: allow NULL arguments to mount

[Paul Burton]

Calls to the mount syscall can legitimately provide NULL as the value
for the source of filesystemtype arguments, which QEMU would previously
reject & return -EFAULT to the target program. An example of this is
remounting an already mounted filesystem with different properties.

Instead of rejecting such syscalls with -EFAULT, pass NULL along to the
kernel as the target program expects.

Additionally this patch fixes a potential memory leak when DEBUG_REMAP
is enabled and lock_user_string fails on the target or filesystemtype
arguments but a prior argument was non-NULL and already locked.

Since the patch already touched most lines of the TARGET_NR_mount case,
it fixes the indentation for good measure.

Signed-off-by: Paul Burton <address@hidden>
---
Changes in v2:
  - None.
---
 linux-user/syscall.c | 68 +++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 46 insertions(+), 22 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 7da21c7..3861f0f 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -5563,29 +5563,53 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
arg1,
         break;
 #endif
     case TARGET_NR_mount:
-               {
-                       /* need to look at the data field */
-                       void *p2, *p3;
-                       p = lock_user_string(arg1);
-                       p2 = lock_user_string(arg2);
-                       p3 = lock_user_string(arg3);
-                        if (!p || !p2 || !p3)
-                            ret = -TARGET_EFAULT;
-                        else {
-                            /* FIXME - arg5 should be locked, but it isn't 
clear how to
-                             * do that since it's not guaranteed to be a 
NULL-terminated
-                             * string.
-                             */
-                            if ( ! arg5 )
-                                ret = get_errno(mount(p, p2, p3, (unsigned 
long)arg4, NULL));
-                            else
-                                ret = get_errno(mount(p, p2, p3, (unsigned 
long)arg4, g2h(arg5)));
-                        }
+        {
+            /* need to look at the data field */
+            void *p2, *p3;
+
+            if (arg1) {
+                p = lock_user_string(arg1);
+                if (!p)
+                    goto efault;
+            } else {
+                p = NULL;
+            }
+
+            p2 = lock_user_string(arg2);
+            if (!p2) {
+                if (arg1)
+                    unlock_user(p, arg1, 0);
+                    goto efault;
+            }
+
+            if (arg3) {
+                p3 = lock_user_string(arg3);
+                if (!p3) {
+                    if (arg1)
                         unlock_user(p, arg1, 0);
-                        unlock_user(p2, arg2, 0);
-                        unlock_user(p3, arg3, 0);
-                       break;
-               }
+                    unlock_user(p2, arg2, 0);
+                    goto efault;
+                }
+            } else {
+                p3 = NULL;
+            }
+
+            /* FIXME - arg5 should be locked, but it isn't clear how to
+             * do that since it's not guaranteed to be a NULL-terminated
+             * string.
+             */
+             if (!arg5)
+                 ret = get_errno(mount(p, p2, p3, (unsigned long)arg4, NULL));
+             else
+                 ret = get_errno(mount(p, p2, p3, (unsigned long)arg4, 
g2h(arg5)));
+
+             if (arg1)
+                 unlock_user(p, arg1, 0);
+             unlock_user(p2, arg2, 0);
+             if (arg3)
+                 unlock_user(p3, arg3, 0);
+        }
+        break;
 #ifdef TARGET_NR_umount
     case TARGET_NR_umount:
         if (!(p = lock_user_string(arg1)))
-- 
2.0.0