[Qemu-devel] [PATCH 0/2] linux-user: check clone flags for unsupported options

[Qemu-devel] [PATCH 0/2] linux-user: check clone flags for unsupported options

[Peter Maydell]

This patchset adds checks on the flags passed to the clone syscall.
Previously we weren't checking the flags at all for the clone case,
which meant that some tests in the LTP testsuite would behave bizarrely
because we let the clone syscall succeed but didn't provide the
semantics requested by the flags. The patches add sanity checking
so that we fail (EINVAL) any flags or flag-combinations which we
can't support.

(Sadly we can't just implement clone by passing directly through
to the host syscall, because that would badly confuse libc, breaking
mutexes, getpid(), etc. So we can only support things we can
emulate via either fork() or pthread_create().)

The first patch is a minor cleanup; the second has the meat.

This is the last of the linux-user fixes I have on my plate
for fixing up LTP issues. (There are a pile of other LTP failures
but I don't think they're interesting enough to tackle until/unless
we get bug reports about real world programs which have the same
problems. I'll resend a summary report of remaining LTP failures
when the last of the patches eventually hits master, ie after
the 2.7 release.)

I don't think this patchset really needs to go into 2.7.

Git branch with this and all the rest at:
https://git.linaro.org/people/peter.maydell/qemu-arm.git linux-fixes

thanks
-- PMM


Peter Maydell (2):
  linux-user: Remove unnecessary nptl_flags variable from do_fork()
  linux-user: Sanity check clone flags

 linux-user/syscall.c | 82 ++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 70 insertions(+), 12 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH 1/2] linux-user: Remove unnecessary nptl_flags variable from do_fork()

[Peter Maydell]

The 'nptl_flags' variable in do_fork() is set to a copy of
'flags', and then the CLONE_NPTL_FLAGS are cleared out of 'flags'.
However the only effect of this is that the later check on
"if (flags & CLONE_PARENT_SETTID)" is never true. Since we
will already have done the setting of parent_tidptr in clone_func()
in the child thread, we don't need to do it again.

Delete the dead if() and the clearing of CLONE_NPTL_FLAGS from
'flags', and then use 'flags' where we were previously using
'nptl_flags', so we can delete the unnecessary variable.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/syscall.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 17ecd10..1bde131 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -5856,7 +5856,6 @@ static int do_fork(CPUArchState *env, unsigned int flags, abi_ulong newsp,
     TaskState *ts;
     CPUState *new_cpu;
     CPUArchState *new_env;
-    unsigned int nptl_flags;
     sigset_t sigmask;
 
     /* Emulate vfork() with fork() */
@@ -5879,15 +5878,14 @@ static int do_fork(CPUArchState *env, unsigned int flags, abi_ulong newsp,
         ts->bprm = parent_ts->bprm;
         ts->info = parent_ts->info;
         ts->signal_mask = parent_ts->signal_mask;
-        nptl_flags = flags;
-        flags &= ~CLONE_NPTL_FLAGS2;
 
-        if (nptl_flags & CLONE_CHILD_CLEARTID) {
+        if (flags & CLONE_CHILD_CLEARTID) {
             ts->child_tidptr = child_tidptr;
         }
 
-        if (nptl_flags & CLONE_SETTLS)
+        if (flags & CLONE_SETTLS) {
             cpu_set_tls (new_env, newtls);
+        }
 
         /* Grab a mutex so that thread setup appears atomic.  */
         pthread_mutex_lock(&clone_lock);
@@ -5897,10 +5895,12 @@ static int do_fork(CPUArchState *env, unsigned int flags, abi_ulong newsp,
         pthread_mutex_lock(&info.mutex);
         pthread_cond_init(&info.cond, NULL);
         info.env = new_env;
-        if (nptl_flags & CLONE_CHILD_SETTID)
+        if (flags & CLONE_CHILD_SETTID) {
             info.child_tidptr = child_tidptr;
-        if (nptl_flags & CLONE_PARENT_SETTID)
+        }
+        if (flags & CLONE_PARENT_SETTID) {
             info.parent_tidptr = parent_tidptr;
+        }
 
         ret = pthread_attr_init(&attr);
         ret = pthread_attr_setstacksize(&attr, NEW_STACK_SIZE);
@@ -5919,8 +5919,6 @@ static int do_fork(CPUArchState *env, unsigned int flags, abi_ulong newsp,
             /* Wait for the child to initialize.  */
             pthread_cond_wait(&info.cond, &info.mutex);
             ret = info.tid;
-            if (flags & CLONE_PARENT_SETTID)
-                put_user_u32(ret, parent_tidptr);
         } else {
             ret = -1;
         }
-- 
1.9.1

[Qemu-devel] [PATCH 2/2] linux-user: Sanity check clone flags

[Peter Maydell]

We currently make no checks on the flags passed to the clone syscall,
which means we will not fail clone attempts which ask for features
that we can't implement. Add sanity checking of the flags to clone
(which we were already doing in the "this is a fork" path, but not
for the "this is a new thread" path), tidy up the checking in
the fork path to match it, and check that the fork case isn't trying
to specify a custom termination signal.

This is helpful in causing some LTP test cases to fail cleanly
rather than behaving bizarrely when we let the clone succeed
but didn't provide the semantics requested by the flags.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/syscall.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 63 insertions(+), 3 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 1bde131..ebdb753 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -112,8 +112,56 @@ int __clone2(int (*fn)(void *), void *child_stack_base,
 
 #include "qemu.h"
 
-#define CLONE_NPTL_FLAGS2 (CLONE_SETTLS | \
-    CLONE_PARENT_SETTID | CLONE_CHILD_SETTID | CLONE_CHILD_CLEARTID)
+#ifndef CLONE_IO
+#define CLONE_IO                0x80000000      /* Clone io context */
+#endif
+
+/* We can't directly call the host clone syscall, because this will
+ * badly confuse libc (breaking mutexes, for example). So we must
+ * divide clone flags into:
+ *  * flag combinations that look like pthread_create()
+ *  * flag combinations that look like fork()
+ *  * flags we can implement within QEMU itself
+ *  * flags we can't support and will return an error for
+ */
+/* For thread creation, all these flags must be present; for
+ * fork, none must be present.
+ */
+#define CLONE_THREAD_FLAGS                              \
+    (CLONE_VM | CLONE_FS | CLONE_FILES |                \
+     CLONE_SIGHAND | CLONE_THREAD | CLONE_SYSVSEM)
+
+/* These flags are ignored:
+ * CLONE_DETACHED is now ignored by the kernel;
+ * CLONE_IO is just an optimisation hint to the I/O scheduler
+ */
+#define CLONE_IGNORED_FLAGS                     \
+    (CLONE_DETACHED | CLONE_IO)
+
+/* Flags for fork which we can implement within QEMU itself */
+#define CLONE_OPTIONAL_FORK_FLAGS               \
+    (CLONE_SETTLS | CLONE_PARENT_SETTID |       \
+     CLONE_CHILD_CLEARTID | CLONE_CHILD_SETTID)
+
+/* Flags for thread creation which we can implement within QEMU itself */
+#define CLONE_OPTIONAL_THREAD_FLAGS                             \
+    (CLONE_SETTLS | CLONE_PARENT_SETTID |                       \
+     CLONE_CHILD_CLEARTID | CLONE_CHILD_SETTID | CLONE_PARENT)
+
+#define CLONE_INVALID_FORK_FLAGS                                        \
+    (~(CSIGNAL | CLONE_OPTIONAL_FORK_FLAGS | CLONE_IGNORED_FLAGS))
+
+#define CLONE_INVALID_THREAD_FLAGS                                      \
+    (~(CSIGNAL | CLONE_THREAD_FLAGS | CLONE_OPTIONAL_THREAD_FLAGS |     \
+       CLONE_IGNORED_FLAGS))
+
+/* CLONE_VFORK is special cased early in do_fork(). The other flag bits
+ * have almost all been allocated. We cannot support any of
+ * CLONE_NEWNS, CLONE_NEWCGROUP, CLONE_NEWUTS, CLONE_NEWIPC,
+ * CLONE_NEWUSER, CLONE_NEWPID, CLONE_NEWNET, CLONE_PTRACE, CLONE_UNTRACED.
+ * The checks against the invalid thread masks above will catch these.
+ * (The one remaining unallocated bit is 0x1000 which used to be CLONE_PID.)
+ */
 
 //#define DEBUG
 /* Define DEBUG_ERESTARTSYS to force every syscall to be restarted
@@ -5858,6 +5906,8 @@ static int do_fork(CPUArchState *env, unsigned int flags, abi_ulong newsp,
     CPUArchState *new_env;
     sigset_t sigmask;
 
+    flags &= ~CLONE_IGNORED_FLAGS;
+
     /* Emulate vfork() with fork() */
     if (flags & CLONE_VFORK)
         flags &= ~(CLONE_VFORK | CLONE_VM);
@@ -5867,6 +5917,11 @@ static int do_fork(CPUArchState *env, unsigned int flags, abi_ulong newsp,
         new_thread_info info;
         pthread_attr_t attr;
 
+        if (((flags & CLONE_THREAD_FLAGS) != CLONE_THREAD_FLAGS) ||
+            (flags & CLONE_INVALID_THREAD_FLAGS)) {
+            return -TARGET_EINVAL;
+        }
+
         ts = g_new0(TaskState, 1);
         init_task_state(ts);
         /* we create a new CPU instance. */
@@ -5928,7 +5983,12 @@ static int do_fork(CPUArchState *env, unsigned int flags, abi_ulong newsp,
         pthread_mutex_unlock(&clone_lock);
     } else {
         /* if no CLONE_VM, we consider it is a fork */
-        if ((flags & ~(CSIGNAL | CLONE_NPTL_FLAGS2)) != 0) {
+        if (flags & CLONE_INVALID_FORK_FLAGS) {
+            return -TARGET_EINVAL;
+        }
+
+        /* We can't support custom termination signals */
+        if ((flags & CSIGNAL) != TARGET_SIGCHLD) {
             return -TARGET_EINVAL;
         }
 
-- 
1.9.1