Skip to content
Permalink
Browse files

userspace: fix copy from user locking

We don't actually need spinlocks here.

For user_copy(), we are checking that the pointer/size passed in
from user mode represents an area that the thread can read or
write to. Then we do a memcpy into the kernel-side buffer,
which is used from then on. It's OK if another thread scribbles
on the buffer contents during the copy, as we have not yet
begun any examination of its contents yet.

For the z_user_string*_copy() functions, it's also possible
that another thread could scribble on the string contents,
but we do no analysis of the string other than to establish
a length. We just need to ensure that when these functions
exit, the copied string is NULL terminated.

For SMP, the spinlocks are removed as they will not prevent a
thread running on another CPU from changing the buffer/string
contents, we just need to safely deal with that possibility.

For UP, the locks do prevent another thread from stepping
in, but it's better to just safely deal with it rather than
affect the interrupt latency of the system.

Signed-off-by: Andrew Boie <andrew.p.boie@intel.com>
  • Loading branch information...
andrewboie authored and nashif committed Apr 12, 2019
1 parent 4097a5b commit 09dc929d41a9733819fce1aee9a45393b954d11d
Showing with 11 additions and 10 deletions.
  1. +11 −10 kernel/userspace.c
@@ -50,8 +50,6 @@ static struct k_spinlock lists_lock; /* kobj rbtree/dlist */
static struct k_spinlock objfree_lock; /* k_object_free */ static struct k_spinlock objfree_lock; /* k_object_free */
#endif #endif
static struct k_spinlock obj_lock; /* kobj struct data */ static struct k_spinlock obj_lock; /* kobj struct data */
static struct k_spinlock ucopy_lock; /* copy to/from userspace */
static struct k_spinlock ucopy_outer_lock; /* code that calls copies */


#define MAX_THREAD_BITS (CONFIG_MAX_THREAD_BYTES * 8) #define MAX_THREAD_BITS (CONFIG_MAX_THREAD_BYTES * 8)


@@ -629,7 +627,6 @@ void z_object_uninit(void *obj)
void *z_user_alloc_from_copy(const void *src, size_t size) void *z_user_alloc_from_copy(const void *src, size_t size)
{ {
void *dst = NULL; void *dst = NULL;
k_spinlock_key_t key = k_spin_lock(&ucopy_lock);


/* Does the caller in user mode have access to read this memory? */ /* Does the caller in user mode have access to read this memory? */
if (Z_SYSCALL_MEMORY_READ(src, size)) { if (Z_SYSCALL_MEMORY_READ(src, size)) {
@@ -644,14 +641,12 @@ void *z_user_alloc_from_copy(const void *src, size_t size)


(void)memcpy(dst, src, size); (void)memcpy(dst, src, size);
out_err: out_err:
k_spin_unlock(&ucopy_lock, key);
return dst; return dst;
} }


static int user_copy(void *dst, const void *src, size_t size, bool to_user) static int user_copy(void *dst, const void *src, size_t size, bool to_user)
{ {
int ret = EFAULT; int ret = EFAULT;
k_spinlock_key_t key = k_spin_lock(&ucopy_lock);


/* Does the caller in user mode have access to this memory? */ /* Does the caller in user mode have access to this memory? */
if (to_user ? Z_SYSCALL_MEMORY_WRITE(dst, size) : if (to_user ? Z_SYSCALL_MEMORY_WRITE(dst, size) :
@@ -662,7 +657,6 @@ static int user_copy(void *dst, const void *src, size_t size, bool to_user)
(void)memcpy(dst, src, size); (void)memcpy(dst, src, size);
ret = 0; ret = 0;
out_err: out_err:
k_spin_unlock(&ucopy_lock, key);
return ret; return ret;
} }


@@ -681,7 +675,6 @@ char *z_user_string_alloc_copy(const char *src, size_t maxlen)
unsigned long actual_len; unsigned long actual_len;
int err; int err;
char *ret = NULL; char *ret = NULL;
k_spinlock_key_t key = k_spin_lock(&ucopy_outer_lock);


actual_len = z_user_string_nlen(src, maxlen, &err); actual_len = z_user_string_nlen(src, maxlen, &err);
if (err != 0) { if (err != 0) {
@@ -698,16 +691,22 @@ char *z_user_string_alloc_copy(const char *src, size_t maxlen)
} }


ret = z_user_alloc_from_copy(src, actual_len); ret = z_user_alloc_from_copy(src, actual_len);

/* Someone may have modified the source string during the above
* checks. Ensure what we actually copied is still terminated
* properly.
*/
if (ret != NULL) {
ret[actual_len - 1] = '\0';
}
out: out:
k_spin_unlock(&ucopy_outer_lock, key);
return ret; return ret;
} }


int z_user_string_copy(char *dst, const char *src, size_t maxlen) int z_user_string_copy(char *dst, const char *src, size_t maxlen)
{ {
unsigned long actual_len; unsigned long actual_len;
int ret, err; int ret, err;
k_spinlock_key_t key = k_spin_lock(&ucopy_outer_lock);


actual_len = z_user_string_nlen(src, maxlen, &err); actual_len = z_user_string_nlen(src, maxlen, &err);
if (err != 0) { if (err != 0) {
@@ -727,8 +726,10 @@ int z_user_string_copy(char *dst, const char *src, size_t maxlen)
} }


ret = z_user_from_copy(dst, src, actual_len); ret = z_user_from_copy(dst, src, actual_len);

/* See comment above in z_user_string_alloc_copy() */
dst[actual_len - 1] = '\0';
out: out:
k_spin_unlock(&ucopy_outer_lock, key);
return ret; return ret;
} }


0 comments on commit 09dc929

Please sign in to comment.
You can’t perform that action at this time.