Skip to content

Commit

Permalink
[LibOS] Fix a few user-pointer checking bugs in syscalls
Browse files Browse the repository at this point in the history
Additionally fixes a bug in gettimeofday() implementation which left
timezone struct uninitialized.

Signed-off-by: Michał Kowalczyk <mkow@invisiblethingslab.com>
  • Loading branch information
mkow authored and dimakuv committed Sep 11, 2023
1 parent 4ef6abb commit 3475e38
Show file tree
Hide file tree
Showing 9 changed files with 39 additions and 40 deletions.
5 changes: 3 additions & 2 deletions libos/include/libos_thread.h
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,8 @@ struct libos_thread {

/* child tid */
int* set_child_tid;
int* clear_child_tid; /* LibOS zeroes it to notify parent that thread exited */
int* clear_child_tid; /* LibOS zeroes it to notify parent that thread exited.
* Be careful - unverified user pointer! */
int clear_child_tid_pal; /* PAL zeroes it to notify LibOS that thread exited */

/* Thread signal mask. Should be accessed with `this_thread->lock` held. Must not be modified
Expand Down Expand Up @@ -114,7 +115,7 @@ struct libos_thread {
stack_t signal_altstack;

/* futex robust list */
struct robust_list_head* robust_list;
struct robust_list_head* robust_list; // careful - unverified user pointer!

PAL_HANDLE scheduler_event;

Expand Down
3 changes: 0 additions & 3 deletions libos/src/sys/libos_access.c
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,6 @@ long libos_syscall_access(const char* file, mode_t mode) {
}

long libos_syscall_faccessat(int dfd, const char* filename, mode_t mode) {
if (!filename)
return -EINVAL;

if (!is_user_string_readable(filename))
return -EFAULT;

Expand Down
7 changes: 3 additions & 4 deletions libos/src/sys/libos_alarm.c
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,9 @@ long libos_syscall_setitimer(int which, struct __kernel_itimerval* value,
if (which != ITIMER_REAL)
return -ENOSYS;

if (!value)
return -EFAULT;
/* Technically, as of v6.5.2, Linux supports value==NULL with a special meaning (setting timer
* to 0), but it throws a warning that it will be removed in the future versions, so we probably
* don't have to implement it. */
if (!is_user_memory_readable(value, sizeof(*value)))
return -EFAULT;
if (ovalue && !is_user_memory_writable(ovalue, sizeof(*ovalue)))
Expand Down Expand Up @@ -131,8 +132,6 @@ long libos_syscall_getitimer(int which, struct __kernel_itimerval* value) {
if (which != ITIMER_REAL)
return -ENOSYS;

if (!value)
return -EFAULT;
if (!is_user_memory_writable(value, sizeof(*value)))
return -EFAULT;

Expand Down
14 changes: 6 additions & 8 deletions libos/src/sys/libos_clone.c
Original file line number Diff line number Diff line change
Expand Up @@ -297,8 +297,8 @@ long libos_syscall_clone(unsigned long flags, unsigned long user_stack_addr, int

int* set_parent_tid = NULL;
if (flags & CLONE_PARENT_SETTID) {
if (!parent_tidptr)
return -EINVAL;
if (!is_user_memory_writable(parent_tidptr, sizeof(*parent_tidptr)))
return -EFAULT;
set_parent_tid = parent_tidptr;
}

Expand Down Expand Up @@ -333,13 +333,12 @@ long libos_syscall_clone(unsigned long flags, unsigned long user_stack_addr, int

struct libos_thread* thread = get_new_thread();
if (!thread) {
ret = -ENOMEM;
goto failed;
return -ENOMEM;
}

if (flags & CLONE_CHILD_SETTID) {
if (!child_tidptr) {
ret = -EINVAL;
if (!is_user_memory_writable(child_tidptr, sizeof(*child_tidptr))) {
ret = -EFAULT;
goto failed;
}
thread->set_child_tid = child_tidptr;
Expand Down Expand Up @@ -489,7 +488,6 @@ long libos_syscall_clone(unsigned long flags, unsigned long user_stack_addr, int
if (new_args.initialize_event)
PalObjectClose(new_args.initialize_event);
failed:
if (thread)
put_thread(thread);
put_thread(thread);
return ret;
}
3 changes: 0 additions & 3 deletions libos/src/sys/libos_getcwd.c
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,6 @@
#endif

long libos_syscall_getcwd(char* buf, size_t buf_size) {
if (!buf || !buf_size)
return -EINVAL;

if (!is_user_memory_writable(buf, buf_size))
return -EFAULT;

Expand Down
2 changes: 1 addition & 1 deletion libos/src/sys/libos_getpid.c
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ long libos_syscall_getppid(void) {
long libos_syscall_set_tid_address(int* tidptr) {
struct libos_thread* cur = get_cur_thread();
lock(&cur->lock);
cur->clear_child_tid = tidptr;
cur->clear_child_tid = tidptr; // will be lazy-verified (before writing to it)
unlock(&cur->lock);
return cur->tid;
}
Expand Down
6 changes: 3 additions & 3 deletions libos/src/sys/libos_mmap.c
Original file line number Diff line number Diff line change
Expand Up @@ -371,9 +371,9 @@ long libos_syscall_munmap(void* _addr, size_t length) {
return 0;
}

/* This emulation of mincore() always tells that pages are _NOT_ in RAM
* pessimistically due to lack of a good way to know it.
* Possibly it may cause performance(or other) issue due to this lying.
/* This emulation of mincore() always pessimistically tells that pages are _NOT_ in RAM due to lack
* of a good way to know it.
* This lying may possibly cause performance (or other) issues.
*/
long libos_syscall_mincore(void* addr, size_t len, unsigned char* vec) {
if (!IS_ALLOC_ALIGNED_PTR(addr))
Expand Down
6 changes: 5 additions & 1 deletion libos/src/sys/libos_socket.c
Original file line number Diff line number Diff line change
Expand Up @@ -1349,7 +1349,7 @@ long libos_syscall_getsockopt(int fd, int level, int optname, char* optval, int*
goto out;
}

if (!is_user_memory_writable(optlen, sizeof(*optlen))) {
if (!is_user_memory_readable(optlen, sizeof(*optlen))) {
ret = -EFAULT;
goto out;
}
Expand Down Expand Up @@ -1385,6 +1385,10 @@ long libos_syscall_getsockopt(int fd, int level, int optname, char* optval, int*
unlock(&sock->lock);

if (ret == 0) {
if (!is_user_memory_writable(optlen, sizeof(*optlen))) {
ret = -EFAULT;
goto out;
}
*optlen = len;
}

Expand Down
33 changes: 18 additions & 15 deletions libos/src/sys/libos_time.c
Original file line number Diff line number Diff line change
Expand Up @@ -11,23 +11,29 @@
#include "pal.h"

long libos_syscall_gettimeofday(struct __kernel_timeval* tv, struct __kernel_timezone* tz) {
if (!tv)
return -EINVAL;
if (tv) {
if (!is_user_memory_writable(tv, sizeof(*tv)))
return -EFAULT;

if (!is_user_memory_writable(tv, sizeof(*tv)))
return -EFAULT;
uint64_t time = 0;
int ret = PalSystemTimeQuery(&time);
if (ret < 0) {
return pal_to_unix_errno(ret);
}

if (tz && !is_user_memory_writable(tz, sizeof(*tz)))
return -EFAULT;
tv->tv_sec = time / 1000000;
tv->tv_usec = time % 1000000;
}

uint64_t time = 0;
int ret = PalSystemTimeQuery(&time);
if (ret < 0) {
return pal_to_unix_errno(ret);
if (tz) {
if (!is_user_memory_writable(tz, sizeof(*tz)))
return -EFAULT;

/* Not implemented, return zeros. */
tz->tz_minuteswest = 0;
tz->tz_dsttime = 0;
}

tv->tv_sec = time / 1000000;
tv->tv_usec = time % 1000000;
return 0;
}

Expand All @@ -54,9 +60,6 @@ long libos_syscall_clock_gettime(clockid_t which_clock, struct timespec* tp) {
if (!(0 <= which_clock && which_clock < MAX_CLOCKS))
return -EINVAL;

if (!tp)
return -EINVAL;

if (!is_user_memory_writable(tp, sizeof(*tp)))
return -EFAULT;

Expand Down

0 comments on commit 3475e38

Please sign in to comment.