From 19ac59af127b550f40a819cbd3c3f84fc0554b4e Mon Sep 17 00:00:00 2001 From: Andrew Boie Date: Mon, 9 Oct 2017 15:20:37 -0700 Subject: [PATCH 1/5] userspace: move _k_object_validate() definition This API only gets used inside system call handlers and a specific test case dedicated to it. Move definition to the private kernel header along with the rest of the defines for system call handlers. A non-userspace inline variant of this function is unnecessary and has been deleted. Signed-off-by: Andrew Boie --- include/kernel.h | 31 ------------------- kernel/include/syscall_handler.h | 22 +++++++++++++ .../mem_protect/obj_validation/src/main.c | 1 + 3 files changed, 23 insertions(+), 31 deletions(-) diff --git a/include/kernel.h b/include/kernel.h index dee8b6ee113c..d396acf04371 100644 --- a/include/kernel.h +++ b/include/kernel.h @@ -178,28 +178,6 @@ struct _k_object { } __packed; #define K_OBJ_FLAG_INITIALIZED BIT(0) -/** - * Ensure a system object is a valid object of the expected type - * - * Searches for the object and ensures that it is indeed an object - * of the expected type, that the caller has the right permissions on it, - * and that the object has been initialized. - * - * This function is intended to be called on the kernel-side system - * call handlers to validate kernel object pointers passed in from - * userspace. - * - * @param obj Address of the kernel object - * @param otype Expected type of the kernel object - * @param init If true, this is for an init function and we will not error - * out if the object is not initialized - * @return 0 If the object is valid - * -EBADF if not a valid object of the specified type - * -EPERM If the caller does not have permissions - * -EINVAL Object is not initialized - */ -int _k_object_validate(void *obj, enum k_objects otype, int init); - /** * Lookup a kernel object and init its metadata if it exists @@ -212,15 +190,6 @@ int _k_object_validate(void *obj, enum k_objects otype, int init); */ void _k_object_init(void *obj); #else -static inline int _k_object_validate(void *obj, enum k_objects otype, int init) -{ - ARG_UNUSED(obj); - ARG_UNUSED(otype); - ARG_UNUSED(init); - - return 0; -} - static inline void _k_object_init(void *obj) { ARG_UNUSED(obj); diff --git a/kernel/include/syscall_handler.h b/kernel/include/syscall_handler.h index d6f182e47b17..ef9eb5472a9f 100644 --- a/kernel/include/syscall_handler.h +++ b/kernel/include/syscall_handler.h @@ -17,6 +17,28 @@ extern const _k_syscall_handler_t _k_syscall_table[K_SYSCALL_LIMIT]; +/** + * Ensure a system object is a valid object of the expected type + * + * Searches for the object and ensures that it is indeed an object + * of the expected type, that the caller has the right permissions on it, + * and that the object has been initialized. + * + * This function is intended to be called on the kernel-side system + * call handlers to validate kernel object pointers passed in from + * userspace. + * + * @param obj Address of the kernel object + * @param otype Expected type of the kernel object + * @param init If true, this is for an init function and we will not error + * out if the object is not initialized + * @return 0 If the object is valid + * -EBADF if not a valid object of the specified type + * -EPERM If the caller does not have permissions + * -EINVAL Object is not initialized + */ +int _k_object_validate(void *obj, enum k_objects otype, int init); + /** * @brief Runtime expression check for system call arguments * diff --git a/tests/kernel/mem_protect/obj_validation/src/main.c b/tests/kernel/mem_protect/obj_validation/src/main.c index 1bea6bdd3d77..78e23369e200 100644 --- a/tests/kernel/mem_protect/obj_validation/src/main.c +++ b/tests/kernel/mem_protect/obj_validation/src/main.c @@ -8,6 +8,7 @@ #include #include #include +#include #define SEM_ARRAY_SIZE 16 From ce0cb4ded67828570c1a0c9aea0517f2305f36e0 Mon Sep 17 00:00:00 2001 From: Andrew Boie Date: Mon, 9 Oct 2017 15:09:29 -0700 Subject: [PATCH 2/5] syscalls: add _SYSCALL_VERIFY_MSG() Expecting stringified expressions to be completely comprehensible to end users is wishful thinking; we really need to express what a failed system call verification step means in human terms in most cases. Memory buffer and kernel object checks now are implemented in terms of _SYSCALL_VERIFY_MSG. Signed-off-by: Andrew Boie --- kernel/include/syscall_handler.h | 30 +++++++++++++++++++++++++----- 1 file changed, 25 insertions(+), 5 deletions(-) diff --git a/kernel/include/syscall_handler.h b/kernel/include/syscall_handler.h index ef9eb5472a9f..0223b33adf57 100644 --- a/kernel/include/syscall_handler.h +++ b/kernel/include/syscall_handler.h @@ -43,20 +43,37 @@ int _k_object_validate(void *obj, enum k_objects otype, int init); * @brief Runtime expression check for system call arguments * * Used in handler functions to perform various runtime checks on arguments, - * and generate a kernel oops if anything is not expected + * and generate a kernel oops if anything is not expected, printing a custom + * message. * * @param expr Boolean expression to verify, a false result will trigger an * oops * @param ssf Syscall stack frame argument passed to the handler function + * @param fmt Printf-style format string (followed by appropriate variadic + * arguments) to print on verification failure */ -#define _SYSCALL_VERIFY(expr, ssf) \ +#define _SYSCALL_VERIFY_MSG(expr, ssf, fmt, ...) \ do { \ if (!(expr)) { \ - printk("FATAL: syscall failed check: " #expr "\n"); \ + printk("FATAL: syscall %s failed check: " fmt "\n", \ + __func__, ##__VA_ARGS__); \ _arch_syscall_oops(ssf); \ } \ } while (0) +/** + * @brief Runtime expression check for system call arguments + * + * Used in handler functions to perform various runtime checks on arguments, + * and generate a kernel oops if anything is not expected. + * + * @param expr Boolean expression to verify, a false result will trigger an + * oops. A stringified version of this expression will be printed. + * @param ssf Syscall stack frame argument passed to the handler function + * arguments) to print on verification failure + */ +#define _SYSCALL_VERIFY(expr, ssf) _SYSCALL_VERIFY_MSG(expr, ssf, #expr) + /** * @brief Runtime check that a user thread has proper access to a memory area * @@ -73,7 +90,9 @@ int _k_object_validate(void *obj, enum k_objects otype, int init); * @param ssf Syscall stack frame argument passed to the handler function */ #define _SYSCALL_MEMORY(ptr, size, write, ssf) \ - _SYSCALL_VERIFY(!_arch_buffer_validate((void *)ptr, size, write), ssf) + _SYSCALL_VERIFY_MSG(!_arch_buffer_validate((void *)ptr, size, write), \ + ssf, "Memory region %p (size %u) has incorrect permissions", \ + (void *)(ptr), (u32_t)(size)) /** * @brief Runtime check that a pointer is a kernel object of expected type @@ -87,7 +106,8 @@ int _k_object_validate(void *obj, enum k_objects otype, int init); * @param ssf Syscall stack frame argument passed to the handler function */ #define _SYSCALL_IS_OBJ(ptr, type, init, ssf) \ - _SYSCALL_VERIFY(!_k_object_validate((void *)ptr, type, init), ssf) + _SYSCALL_VERIFY_MSG(!_k_object_validate((void *)ptr, type, init), ssf, \ + "object %p access denied", (void *)(ptr)) /* Convenience macros for handler implementations */ #define _SYSCALL_ARG0 ARG_UNUSED(arg1); ARG_UNUSED(arg2); ARG_UNUSED(arg3); \ From b69d981348e8d1d87228f7ec18518d5af66b88f9 Mon Sep 17 00:00:00 2001 From: Andrew Boie Date: Tue, 10 Oct 2017 12:25:55 -0700 Subject: [PATCH 3/5] syscall_handler: introduce new macros Instead of boolean arguments to indicate memory read/write permissions, or init/non-init APIs, new macros are introduced which bake the semantics directly into the name of the macro. Signed-off-by: Andrew Boie --- kernel/include/syscall_handler.h | 68 +++++++++++++++++++++++++------- 1 file changed, 54 insertions(+), 14 deletions(-) diff --git a/kernel/include/syscall_handler.h b/kernel/include/syscall_handler.h index 0223b33adf57..87b64554e125 100644 --- a/kernel/include/syscall_handler.h +++ b/kernel/include/syscall_handler.h @@ -74,14 +74,38 @@ int _k_object_validate(void *obj, enum k_objects otype, int init); */ #define _SYSCALL_VERIFY(expr, ssf) _SYSCALL_VERIFY_MSG(expr, ssf, #expr) +#define _SYSCALL_MEMORY(ptr, size, write, ssf) \ + _SYSCALL_VERIFY_MSG(!_arch_buffer_validate((void *)ptr, size, write), \ + ssf, "Memory region %p (size %u) %s access denied", \ + (void *)(ptr), (u32_t)(size), \ + write ? "write" : "read") + +/** + * @brief Runtime check that a user thread has read permission to a memory area + * + * Checks that the particular memory area is readable by the currently running + * thread if the CPU was in user mode, and generates a kernel oops if it + * wasn't. Prevents userspace from getting the kernel to read memory the thread + * does not have access to, or passing in garbage pointers that would + * crash/pagefault the kernel if dereferenced. + * + * @param ptr Memory area to examine + * @param size Size of the memory area + * @param write If the thread should be able to write to this memory, not just + * read it + * @param ssf Syscall stack frame argument passed to the handler function + */ +#define _SYSCALL_MEMORY_READ(ptr, size, ssf) \ + _SYSCALL_MEMORY(ptr, size, 0, ssf) + /** - * @brief Runtime check that a user thread has proper access to a memory area + * @brief Runtime check that a user thread has write permission to a memory area * - * Checks that the particular memory area is readable or writable by the + * Checks that the particular memory area is readable and writable by the * currently running thread if the CPU was in user mode, and generates a kernel * oops if it wasn't. Prevents userspace from getting the kernel to read or * modify memory the thread does not have access to, or passing in garbage - * pointers that would crash/pagefault the kernel if accessed. + * pointers that would crash/pagefault the kernel if dereferenced. * * @param ptr Memory area to examine * @param size Size of the memory area @@ -89,25 +113,41 @@ int _k_object_validate(void *obj, enum k_objects otype, int init); * read it * @param ssf Syscall stack frame argument passed to the handler function */ -#define _SYSCALL_MEMORY(ptr, size, write, ssf) \ - _SYSCALL_VERIFY_MSG(!_arch_buffer_validate((void *)ptr, size, write), \ - ssf, "Memory region %p (size %u) has incorrect permissions", \ - (void *)(ptr), (u32_t)(size)) +#define _SYSCALL_MEMORY_WRITE(ptr, size, ssf) \ + _SYSCALL_MEMORY(ptr, size, 1, ssf) + +#define _SYSCALL_IS_OBJ(ptr, type, init, ssf) \ + _SYSCALL_VERIFY_MSG(!_k_object_validate((void *)ptr, type, init), ssf, \ + "object %p access denied", (void *)(ptr)) /** - * @brief Runtime check that a pointer is a kernel object of expected type + * @brief Runtime check kernel object pointer for non-init functions * - * Passes along arguments to _k_object_validate() and triggers a kernel oops - * if the object wasn't valid or had incorrect permissions. + * Calls _k_object_validate and triggers a kernel oops if the check files. + * For use in system call handlers which are not init functions; a check + * enforcing that an object is initialized* will not occur. * * @param ptr Untrusted kernel object pointer * @param type Expected kernel object type - * @param init Whether this is an init function handler * @param ssf Syscall stack frame argument passed to the handler function */ -#define _SYSCALL_IS_OBJ(ptr, type, init, ssf) \ - _SYSCALL_VERIFY_MSG(!_k_object_validate((void *)ptr, type, init), ssf, \ - "object %p access denied", (void *)(ptr)) +#define _SYSCALL_OBJ(ptr, type, ssf) \ + _SYSCALL_IS_OBJ(ptr, type, 0, ssf) + +/** + * @brief Runtime check kernel object pointer for non-init functions + * + * See description of _SYSCALL_IS_OBJ. For use in system call handlers which + * are not init functions; a check enforcing that an object is initialized + * will not occur. + * + * @param ptr Untrusted kernel object pointer + * @param type Expected kernel object type + * @param ssf Syscall stack frame argument passed to the handler function + */ + +#define _SYSCALL_OBJ_INIT(ptr, type, ssf) \ + _SYSCALL_IS_OBJ(ptr, type, 1, ssf) /* Convenience macros for handler implementations */ #define _SYSCALL_ARG0 ARG_UNUSED(arg1); ARG_UNUSED(arg2); ARG_UNUSED(arg3); \ From 03d5179c78d2f442d6147da0683a2c2380b609dd Mon Sep 17 00:00:00 2001 From: Andrew Boie Date: Tue, 10 Oct 2017 12:30:23 -0700 Subject: [PATCH 4/5] kernel: system call handler cleanup Use new _SYSCALL_OBJ/_SYSCALL_OBJ_INIT macros. Use new _SYSCALL_MEMORY_READ/_SYSCALL_MEMORY_WRITE macros. Some non-obvious checks changed to use _SYSCALL_VERIFY_MSG. Signed-off-by: Andrew Boie --- kernel/alert.c | 4 ++-- kernel/msg_q.c | 18 +++++++++--------- kernel/mutex.c | 6 +++--- kernel/pipes.c | 16 ++++++++-------- kernel/sched.c | 12 +++++++----- kernel/sem.c | 10 +++++----- kernel/stack.c | 12 ++++++------ kernel/thread.c | 8 ++++---- kernel/thread_abort.c | 5 +++-- kernel/timer.c | 8 ++++---- kernel/userspace_handler.c | 2 +- subsys/driver_handlers/sensor.c | 12 ++++++------ 12 files changed, 58 insertions(+), 55 deletions(-) diff --git a/kernel/alert.c b/kernel/alert.c index 1b9f82c0ef1e..4dc3d24de02c 100644 --- a/kernel/alert.c +++ b/kernel/alert.c @@ -97,7 +97,7 @@ u32_t _handler_k_alert_send(u32_t alert, u32_t arg2, u32_t arg3, { _SYSCALL_ARG1; - _SYSCALL_IS_OBJ(alert, K_OBJ_ALERT, 0, ssf); + _SYSCALL_OBJ(alert, K_OBJ_ALERT, ssf); _impl_k_alert_send((struct k_alert *)alert); return 0; @@ -115,7 +115,7 @@ u32_t _handler_k_alert_recv(u32_t alert, u32_t timeout, u32_t arg3, { _SYSCALL_ARG2; - _SYSCALL_IS_OBJ(alert, K_OBJ_ALERT, 0, ssf); + _SYSCALL_OBJ(alert, K_OBJ_ALERT, ssf); return _impl_k_alert_recv((struct k_alert *)alert, timeout); } #endif diff --git a/kernel/msg_q.c b/kernel/msg_q.c index dda1c0f38d90..da324ec95081 100644 --- a/kernel/msg_q.c +++ b/kernel/msg_q.c @@ -69,8 +69,8 @@ u32_t _handler_k_msgq_init(u32_t q, u32_t buffer, u32_t msg_size, { _SYSCALL_ARG4; - _SYSCALL_IS_OBJ(q, K_OBJ_MSGQ, 1, ssf); - _SYSCALL_MEMORY(buffer, msg_size * max_msgs, 1, ssf); + _SYSCALL_OBJ_INIT(q, K_OBJ_MSGQ, ssf); + _SYSCALL_MEMORY_WRITE(buffer, msg_size * max_msgs, ssf); _impl_k_msgq_init((struct k_msgq *)q, (char *)buffer, msg_size, max_msgs); @@ -133,8 +133,8 @@ u32_t _handler_k_msgq_put(u32_t msgq_p, u32_t data, u32_t timeout, struct k_msgq *q = (struct k_msgq *)msgq_p; _SYSCALL_ARG3; - _SYSCALL_IS_OBJ(q, K_OBJ_MSGQ, 0, ssf); - _SYSCALL_MEMORY(data, q->msg_size, 0, ssf); + _SYSCALL_OBJ(q, K_OBJ_MSGQ, ssf); + _SYSCALL_MEMORY_READ(data, q->msg_size, ssf); return _impl_k_msgq_put(q, (void *)data, timeout); } @@ -201,8 +201,8 @@ u32_t _handler_k_msgq_get(u32_t msgq_p, u32_t data, u32_t timeout, struct k_msgq *q = (struct k_msgq *)msgq_p; _SYSCALL_ARG3; - _SYSCALL_IS_OBJ(q, K_OBJ_MSGQ, 0, ssf); - _SYSCALL_MEMORY(data, q->msg_size, 1, ssf); + _SYSCALL_OBJ(q, K_OBJ_MSGQ, ssf); + _SYSCALL_MEMORY_WRITE(data, q->msg_size, ssf); return _impl_k_msgq_get(q, (void *)data, timeout); } @@ -232,7 +232,7 @@ u32_t _handler_k_msgq_purge(u32_t q, u32_t arg2, u32_t arg3, { _SYSCALL_ARG1; - _SYSCALL_IS_OBJ(q, K_OBJ_MSGQ, 0, ssf); + _SYSCALL_OBJ(q, K_OBJ_MSGQ, ssf); _impl_k_msgq_purge((struct k_msgq *)q); return 0; @@ -243,7 +243,7 @@ u32_t _handler_k_msgq_num_free_get(u32_t q, u32_t arg2, u32_t arg3, u32_t arg4, { _SYSCALL_ARG1; - _SYSCALL_IS_OBJ(q, K_OBJ_MSGQ, 0, ssf); + _SYSCALL_OBJ(q, K_OBJ_MSGQ, ssf); return _impl_k_msgq_num_free_get((struct k_msgq *)q); } @@ -253,7 +253,7 @@ u32_t _handler_k_msgq_num_used_get(u32_t q, u32_t arg2, u32_t arg3, u32_t arg4, { _SYSCALL_ARG1; - _SYSCALL_IS_OBJ(q, K_OBJ_MSGQ, 0, ssf); + _SYSCALL_OBJ(q, K_OBJ_MSGQ, ssf); return _impl_k_msgq_num_used_get((struct k_msgq *)q); } diff --git a/kernel/mutex.c b/kernel/mutex.c index 88e1bfd793f5..45d25abfd9bb 100644 --- a/kernel/mutex.c +++ b/kernel/mutex.c @@ -87,7 +87,7 @@ u32_t _handler_k_mutex_init(u32_t mutex, u32_t arg2, u32_t arg3, { _SYSCALL_ARG1; - _SYSCALL_IS_OBJ(mutex, K_OBJ_MUTEX, 1, ssf); + _SYSCALL_OBJ_INIT(mutex, K_OBJ_MUTEX, ssf); _impl_k_mutex_init((struct k_mutex *)mutex); return 0; @@ -208,7 +208,7 @@ u32_t _handler_k_mutex_lock(u32_t mutex, u32_t timeout, u32_t arg3, { _SYSCALL_ARG2; - _SYSCALL_IS_OBJ(mutex, K_OBJ_MUTEX, 0, ssf); + _SYSCALL_OBJ(mutex, K_OBJ_MUTEX, ssf); return _impl_k_mutex_lock((struct k_mutex *)mutex, (s32_t)timeout); } #endif @@ -272,7 +272,7 @@ u32_t _handler_k_mutex_unlock(u32_t mutex, u32_t arg2, u32_t arg3, { _SYSCALL_ARG1; - _SYSCALL_IS_OBJ(mutex, K_OBJ_MUTEX, 0, ssf); + _SYSCALL_OBJ(mutex, K_OBJ_MUTEX, ssf); _impl_k_mutex_unlock((struct k_mutex *)mutex); return 0; } diff --git a/kernel/pipes.c b/kernel/pipes.c index 2fa44ad3b349..13da5f2575f3 100644 --- a/kernel/pipes.c +++ b/kernel/pipes.c @@ -146,8 +146,8 @@ u32_t _handler_k_pipe_init(u32_t pipe, u32_t buffer, u32_t size, { _SYSCALL_ARG3; - _SYSCALL_IS_OBJ(pipe, K_OBJ_PIPE, 1, ssf); - _SYSCALL_MEMORY(buffer, size, 1, ssf); + _SYSCALL_OBJ_INIT(pipe, K_OBJ_PIPE, ssf); + _SYSCALL_MEMORY_WRITE(buffer, size, ssf); _impl_k_pipe_init((struct k_pipe *)pipe, (unsigned char *)buffer, size); @@ -694,9 +694,9 @@ u32_t _handler_k_pipe_get(u32_t pipe, u32_t data, u32_t bytes_to_read, size_t *bytes_read = (size_t *)bytes_read_p; size_t min_xfer = (size_t)min_xfer_p; - _SYSCALL_IS_OBJ(pipe, K_OBJ_PIPE, 0, ssf); - _SYSCALL_MEMORY(bytes_read, sizeof(*bytes_read), 1, ssf); - _SYSCALL_MEMORY((void *)data, bytes_to_read, 1, ssf); + _SYSCALL_OBJ(pipe, K_OBJ_PIPE, ssf); + _SYSCALL_MEMORY_WRITE(bytes_read, sizeof(*bytes_read), ssf); + _SYSCALL_MEMORY_WRITE((void *)data, bytes_to_read, ssf); _SYSCALL_VERIFY(min_xfer <= bytes_to_read, ssf); return _impl_k_pipe_get((struct k_pipe *)pipe, (void *)data, @@ -724,9 +724,9 @@ u32_t _handler_k_pipe_put(u32_t pipe, u32_t data, u32_t bytes_to_write, size_t *bytes_written = (size_t *)bytes_written_p; size_t min_xfer = (size_t)min_xfer_p; - _SYSCALL_IS_OBJ(pipe, K_OBJ_PIPE, 0, ssf); - _SYSCALL_MEMORY(bytes_written, sizeof(*bytes_written), 1, ssf); - _SYSCALL_MEMORY((void *)data, bytes_to_write, 0, ssf); + _SYSCALL_OBJ(pipe, K_OBJ_PIPE, ssf); + _SYSCALL_MEMORY_WRITE(bytes_written, sizeof(*bytes_written), ssf); + _SYSCALL_MEMORY_READ((void *)data, bytes_to_write, ssf); _SYSCALL_VERIFY(min_xfer <= bytes_to_write, ssf); return _impl_k_pipe_put((struct k_pipe *)pipe, (void *)data, diff --git a/kernel/sched.c b/kernel/sched.c index fdb0cef74053..aed2e164e22d 100644 --- a/kernel/sched.c +++ b/kernel/sched.c @@ -273,7 +273,7 @@ u32_t _handler_k_thread_priority_get(u32_t arg1, u32_t arg2, u32_t arg3, _SYSCALL_ARG1; thread = (struct k_thread *)arg1; - _SYSCALL_IS_OBJ(thread, K_OBJ_THREAD, 0, ssf); + _SYSCALL_OBJ(thread, K_OBJ_THREAD, ssf); return (u32_t)_impl_k_thread_priority_get(thread); } #endif @@ -301,8 +301,9 @@ u32_t _handler_k_thread_priority_set(u32_t thread, u32_t prio, u32_t arg3, { _SYSCALL_ARG2; - _SYSCALL_IS_OBJ(thread, K_OBJ_THREAD, 0, ssf); - _SYSCALL_VERIFY(_VALID_PRIO(prio, NULL), ssf); + _SYSCALL_OBJ(thread, K_OBJ_THREAD, ssf); + _SYSCALL_VERIFY_MSG(_VALID_PRIO(prio, NULL), ssf, + "invalid thread priority %d", (int)prio); _impl_k_thread_priority_set((k_tid_t)thread, prio); return 0; } @@ -399,7 +400,8 @@ u32_t _handler_k_sleep(u32_t arg1, u32_t arg2, u32_t arg3, { _SYSCALL_ARG1; - _SYSCALL_VERIFY(arg1 != K_FOREVER, ssf); + _SYSCALL_VERIFY_MSG(arg1 != K_FOREVER, ssf, + "sleeping forever not allowed"); _impl_k_sleep(arg1); return 0; @@ -436,7 +438,7 @@ u32_t _handler_k_wakeup(u32_t thread, u32_t arg2, u32_t arg3, { _SYSCALL_ARG1; - _SYSCALL_IS_OBJ(thread, K_OBJ_THREAD, 0, ssf); + _SYSCALL_OBJ(thread, K_OBJ_THREAD, ssf); _impl_k_wakeup((k_tid_t)thread); return 0; } diff --git a/kernel/sem.c b/kernel/sem.c index d62b545cb511..1f9652e3d8f8 100644 --- a/kernel/sem.c +++ b/kernel/sem.c @@ -76,7 +76,7 @@ u32_t _handler_k_sem_init(u32_t sem_ptr, u32_t initial_count, u32_t limit, { _SYSCALL_ARG3; - _SYSCALL_IS_OBJ(sem_ptr, K_OBJ_SEM, 1, ssf); + _SYSCALL_OBJ_INIT(sem_ptr, K_OBJ_SEM, ssf); _SYSCALL_VERIFY(limit != 0, ssf); _impl_k_sem_init((struct k_sem *)sem_ptr, initial_count, limit); return 0; @@ -161,7 +161,7 @@ u32_t _handler_k_sem_give(u32_t sem_ptr, u32_t arg2, u32_t arg3, { _SYSCALL_ARG1; - _SYSCALL_IS_OBJ(sem_ptr, K_OBJ_SEM, 0, ssf); + _SYSCALL_OBJ(sem_ptr, K_OBJ_SEM, ssf); _impl_k_sem_give((struct k_sem *)sem_ptr); return 0; @@ -196,7 +196,7 @@ u32_t _handler_k_sem_take(u32_t sem_ptr, u32_t timeout, u32_t arg3, { _SYSCALL_ARG2; - _SYSCALL_IS_OBJ(sem_ptr, K_OBJ_SEM, 0, ssf); + _SYSCALL_OBJ(sem_ptr, K_OBJ_SEM, ssf); return _impl_k_sem_take((struct k_sem *)sem_ptr, timeout); } @@ -205,7 +205,7 @@ u32_t _handler_k_sem_reset(u32_t sem_ptr, u32_t arg2, u32_t arg3, { _SYSCALL_ARG1; - _SYSCALL_IS_OBJ(sem_ptr, K_OBJ_SEM, 0, ssf); + _SYSCALL_OBJ(sem_ptr, K_OBJ_SEM, ssf); _impl_k_sem_reset((struct k_sem *)sem_ptr); return 0; @@ -216,7 +216,7 @@ u32_t _handler_k_sem_count_get(u32_t sem_ptr, u32_t arg2, u32_t arg3, { _SYSCALL_ARG1; - _SYSCALL_IS_OBJ(sem_ptr, K_OBJ_SEM, 0, ssf); + _SYSCALL_OBJ(sem_ptr, K_OBJ_SEM, ssf); return _impl_k_sem_count_get((struct k_sem *)sem_ptr); } #endif /* CONFIG_USERSPACE */ diff --git a/kernel/stack.c b/kernel/stack.c index ea7c9373283e..99708fb7e26b 100644 --- a/kernel/stack.c +++ b/kernel/stack.c @@ -63,8 +63,8 @@ u32_t _handler_k_stack_init(u32_t stack, u32_t buffer, u32_t num_entries_p, /* FIXME why is 'num_entries' signed?? */ _SYSCALL_VERIFY(num_entries > 0, ssf); - _SYSCALL_IS_OBJ(stack, K_OBJ_STACK, 1, ssf); - _SYSCALL_MEMORY(buffer, num_entries * sizeof(u32_t), 1, ssf); + _SYSCALL_OBJ_INIT(stack, K_OBJ_STACK, ssf); + _SYSCALL_MEMORY_WRITE(buffer, num_entries * sizeof(u32_t), ssf); _impl_k_stack_init((struct k_stack *)stack, (u32_t *)buffer, num_entries); @@ -109,8 +109,8 @@ u32_t _handler_k_stack_push(u32_t stack_p, u32_t data, u32_t arg3, struct k_stack *stack = (struct k_stack *)stack_p; _SYSCALL_ARG2; - _SYSCALL_IS_OBJ(stack, K_OBJ_STACK, 0, ssf); - _SYSCALL_VERIFY(stack->next != stack->top, ssf); + _SYSCALL_OBJ(stack, K_OBJ_STACK, ssf); + _SYSCALL_VERIFY_MSG(stack->next != stack->top, ssf, "stack is full"); _impl_k_stack_push(stack, data); return 0; @@ -151,8 +151,8 @@ u32_t _handler_k_stack_pop(u32_t stack, u32_t data, u32_t timeout, { _SYSCALL_ARG3; - _SYSCALL_IS_OBJ(stack, K_OBJ_STACK, 0, ssf); - _SYSCALL_MEMORY(data, sizeof(u32_t), 1, ssf); + _SYSCALL_OBJ(stack, K_OBJ_STACK, ssf); + _SYSCALL_MEMORY_WRITE(data, sizeof(u32_t), ssf); return _impl_k_stack_pop((struct k_stack *)stack, (u32_t *)data, timeout); diff --git a/kernel/thread.c b/kernel/thread.c index a933fc7a8493..d804281fc6b1 100644 --- a/kernel/thread.c +++ b/kernel/thread.c @@ -264,7 +264,7 @@ u32_t _handler_k_thread_start(u32_t thread, u32_t arg2, u32_t arg3, { _SYSCALL_ARG1; - _SYSCALL_IS_OBJ(thread, K_OBJ_THREAD, 0, ssf); + _SYSCALL_OBJ(thread, K_OBJ_THREAD, ssf); _impl_k_thread_start((struct k_thread *)thread); return 0; } @@ -352,7 +352,7 @@ u32_t _handler_k_thread_cancel(u32_t thread, u32_t arg2, u32_t arg3, { _SYSCALL_ARG1; - _SYSCALL_IS_OBJ(thread, K_OBJ_THREAD, 0, ssf); + _SYSCALL_OBJ(thread, K_OBJ_THREAD, ssf); return _impl_k_thread_cancel((struct k_thread *)thread); } #endif @@ -433,7 +433,7 @@ u32_t _handler_k_thread_suspend(u32_t thread, u32_t arg2, u32_t arg3, { _SYSCALL_ARG1; - _SYSCALL_IS_OBJ(thread, K_OBJ_THREAD, 0, ssf); + _SYSCALL_OBJ(thread, K_OBJ_THREAD, ssf); _impl_k_thread_suspend((k_tid_t)thread); return 0; } @@ -463,7 +463,7 @@ u32_t _handler_k_thread_resume(u32_t thread, u32_t arg2, u32_t arg3, { _SYSCALL_ARG1; - _SYSCALL_IS_OBJ(thread, K_OBJ_THREAD, 0, ssf); + _SYSCALL_OBJ(thread, K_OBJ_THREAD, ssf); _impl_k_thread_resume((k_tid_t)thread); return 0; } diff --git a/kernel/thread_abort.c b/kernel/thread_abort.c index 97170e3c6aa0..8c61b6d106d0 100644 --- a/kernel/thread_abort.c +++ b/kernel/thread_abort.c @@ -51,8 +51,9 @@ u32_t _handler_k_thread_abort(u32_t thread_p, u32_t arg2, u32_t arg3, u32_t arg4, u32_t arg5, u32_t arg6, void *ssf) { struct k_thread *thread = (struct k_thread *)thread_p; - _SYSCALL_IS_OBJ(thread, K_OBJ_THREAD, 0, ssf); - _SYSCALL_VERIFY(!(thread->base.user_options & K_ESSENTIAL), ssf); + _SYSCALL_OBJ(thread, K_OBJ_THREAD, ssf); + _SYSCALL_VERIFY_MSG(!(thread->base.user_options & K_ESSENTIAL), ssf, + "aborting essential thread %p", thread); _impl_k_thread_abort((struct k_thread *)thread); return 0; diff --git a/kernel/timer.c b/kernel/timer.c index f04cce14c6ef..a5bf51b3cde2 100644 --- a/kernel/timer.c +++ b/kernel/timer.c @@ -143,7 +143,7 @@ u32_t _handler_k_timer_start(u32_t timer, u32_t duration_p, u32_t period_p, _SYSCALL_VERIFY(duration >= 0 && period >= 0 && (duration != 0 || period != 0), ssf); - _SYSCALL_IS_OBJ(timer, K_OBJ_TIMER, 0, ssf); + _SYSCALL_OBJ(timer, K_OBJ_TIMER, ssf); _impl_k_timer_start((struct k_timer *)timer, duration, period); return 0; } @@ -184,7 +184,7 @@ u32_t _handler_k_timer_stop(u32_t timer, u32_t arg2, u32_t arg3, { _SYSCALL_ARG1; - _SYSCALL_IS_OBJ(timer, K_OBJ_TIMER, 0, ssf); + _SYSCALL_OBJ(timer, K_OBJ_TIMER, ssf); _impl_k_timer_stop((struct k_timer *)timer); return 0; } @@ -208,7 +208,7 @@ u32_t _handler_k_timer_status_get(u32_t timer, u32_t arg2, u32_t arg3, { _SYSCALL_ARG1; - _SYSCALL_IS_OBJ(timer, K_OBJ_TIMER, 0, ssf); + _SYSCALL_OBJ(timer, K_OBJ_TIMER, ssf); return _impl_k_timer_status_get((struct k_timer *)timer); } #endif @@ -249,7 +249,7 @@ u32_t _handler_k_timer_status_sync(u32_t timer, u32_t arg2, u32_t arg3, { _SYSCALL_ARG1; - _SYSCALL_IS_OBJ(timer, K_OBJ_TIMER, 0, ssf); + _SYSCALL_OBJ(timer, K_OBJ_TIMER, ssf); return _impl_k_timer_status_sync((struct k_timer *)timer); } #endif diff --git a/kernel/userspace_handler.c b/kernel/userspace_handler.c index 8e4542b4c177..0dbd86013272 100644 --- a/kernel/userspace_handler.c +++ b/kernel/userspace_handler.c @@ -18,7 +18,7 @@ u32_t _handler_k_object_access_grant(u32_t object, u32_t thread, u32_t arg3, { _SYSCALL_ARG2; - _SYSCALL_IS_OBJ(thread, K_OBJ_THREAD, 0, ssf); + _SYSCALL_OBJ(thread, K_OBJ_THREAD, ssf); _impl_k_object_access_grant((void *)object, (struct k_thread *)thread); return 0; } diff --git a/subsys/driver_handlers/sensor.c b/subsys/driver_handlers/sensor.c index e6f4fc2e412d..97a7cea132f8 100644 --- a/subsys/driver_handlers/sensor.c +++ b/subsys/driver_handlers/sensor.c @@ -12,8 +12,8 @@ u32_t _handler_sensor_attr_set(u32_t dev, u32_t chan, u32_t attr, { _SYSCALL_ARG4; - _SYSCALL_IS_OBJ(dev, K_OBJ_DRIVER_SENSOR, 0, ssf); - _SYSCALL_MEMORY(val, sizeof(struct sensor_value), 0, ssf); + _SYSCALL_OBJ(dev, K_OBJ_DRIVER_SENSOR, ssf); + _SYSCALL_MEMORY_READ(val, sizeof(struct sensor_value), ssf); return _impl_sensor_attr_set((struct device *)dev, chan, attr, (const struct sensor_value *)val); } @@ -24,7 +24,7 @@ u32_t _handler_sensor_sample_fetch(u32_t dev, u32_t arg2, u32_t arg3, { _SYSCALL_ARG1; - _SYSCALL_IS_OBJ(dev, K_OBJ_DRIVER_SENSOR, 0, ssf); + _SYSCALL_OBJ(dev, K_OBJ_DRIVER_SENSOR, ssf); return _impl_sensor_sample_fetch((struct device *)dev); } @@ -35,7 +35,7 @@ u32_t _handler_sensor_sample_fetch_chan(u32_t dev, u32_t type, u32_t arg3, { _SYSCALL_ARG2; - _SYSCALL_IS_OBJ(dev, K_OBJ_DRIVER_SENSOR, 0, ssf); + _SYSCALL_OBJ(dev, K_OBJ_DRIVER_SENSOR, ssf); return _impl_sensor_sample_fetch_chan((struct device *)dev, type); } @@ -45,8 +45,8 @@ u32_t _handler_sensor_channel_get(u32_t dev, u32_t chan, u32_t val, { _SYSCALL_ARG3; - _SYSCALL_IS_OBJ(dev, K_OBJ_DRIVER_SENSOR, 0, ssf); - _SYSCALL_MEMORY(val, sizeof(struct sensor_value), 1, ssf); + _SYSCALL_OBJ(dev, K_OBJ_DRIVER_SENSOR, ssf); + _SYSCALL_MEMORY_WRITE(val, sizeof(struct sensor_value), ssf); return _impl_sensor_channel_get((struct device *)dev, chan, (struct sensor_value *)val); } From 3985f75caebc4ce586d71c45f2d2b70b1a1cd7c4 Mon Sep 17 00:00:00 2001 From: Andrew Boie Date: Tue, 10 Oct 2017 17:19:32 -0700 Subject: [PATCH 5/5] syscall_handler: handle multiplication overflow Computing the total size of the array need to handle the case where the product overflow a 32-bit unsigned integer. Signed-off-by: Andrew Boie --- kernel/include/syscall_handler.h | 41 ++++++++++++++++++++++++++++++++ kernel/msg_q.c | 2 +- kernel/stack.c | 2 +- 3 files changed, 43 insertions(+), 2 deletions(-) diff --git a/kernel/include/syscall_handler.h b/kernel/include/syscall_handler.h index 87b64554e125..a2cea20c6945 100644 --- a/kernel/include/syscall_handler.h +++ b/kernel/include/syscall_handler.h @@ -116,6 +116,47 @@ int _k_object_validate(void *obj, enum k_objects otype, int init); #define _SYSCALL_MEMORY_WRITE(ptr, size, ssf) \ _SYSCALL_MEMORY(ptr, size, 1, ssf) +#define _SYSCALL_MEMORY_ARRAY(ptr, nmemb, size, write, ssf) \ + do { \ + u32_t product; \ + _SYSCALL_VERIFY_MSG(!__builtin_umul_overflow((u32_t)(nmemb), \ + (u32_t)(size), \ + &product), ssf, \ + "%ux%u array is too large", \ + (u32_t)(nmemb), (u32_t)(size)); \ + _SYSCALL_MEMORY(ptr, product, write, ssf); \ + } while (0) + +/** + * @brief Validate user thread has read permission for sized array + * + * Used when the memory region is expressed in terms of number of elements and + * each element size, handles any overflow issues with computing the total + * array bounds. Otherwise see _SYSCALL_MEMORY_READ. + * + * @param ptr Memory area to examine + * @param nmemb Number of elements in the array + * @param size Size of each array element + * @param ssf Syscall stack frame argument passed to the handler function + */ +#define _SYSCALL_MEMORY_ARRAY_READ(ptr, nmemb, size, ssf) \ + _SYSCALL_MEMORY_ARRAY(ptr, nmemb, size, 0, ssf) + +/** + * @brief Validate user thread has read/write permission for sized array + * + * Used when the memory region is expressed in terms of number of elements and + * each element size, handles any overflow issues with computing the total + * array bounds. Otherwise see _SYSCALL_MEMORY_WRITE. + * + * @param ptr Memory area to examine + * @param nmemb Number of elements in the array + * @param size Size of each array element + * @param ssf Syscall stack frame argument passed to the handler function + */ +#define _SYSCALL_MEMORY_ARRAY_WRITE(ptr, nmemb, size, ssf) \ + _SYSCALL_MEMORY_ARRAY(ptr, nmemb, size, 1, ssf) + #define _SYSCALL_IS_OBJ(ptr, type, init, ssf) \ _SYSCALL_VERIFY_MSG(!_k_object_validate((void *)ptr, type, init), ssf, \ "object %p access denied", (void *)(ptr)) diff --git a/kernel/msg_q.c b/kernel/msg_q.c index da324ec95081..61179ecc7367 100644 --- a/kernel/msg_q.c +++ b/kernel/msg_q.c @@ -70,7 +70,7 @@ u32_t _handler_k_msgq_init(u32_t q, u32_t buffer, u32_t msg_size, _SYSCALL_ARG4; _SYSCALL_OBJ_INIT(q, K_OBJ_MSGQ, ssf); - _SYSCALL_MEMORY_WRITE(buffer, msg_size * max_msgs, ssf); + _SYSCALL_MEMORY_ARRAY_WRITE(buffer, max_msgs, msg_size, ssf); _impl_k_msgq_init((struct k_msgq *)q, (char *)buffer, msg_size, max_msgs); diff --git a/kernel/stack.c b/kernel/stack.c index 99708fb7e26b..63693dbe549b 100644 --- a/kernel/stack.c +++ b/kernel/stack.c @@ -64,7 +64,7 @@ u32_t _handler_k_stack_init(u32_t stack, u32_t buffer, u32_t num_entries_p, /* FIXME why is 'num_entries' signed?? */ _SYSCALL_VERIFY(num_entries > 0, ssf); _SYSCALL_OBJ_INIT(stack, K_OBJ_STACK, ssf); - _SYSCALL_MEMORY_WRITE(buffer, num_entries * sizeof(u32_t), ssf); + _SYSCALL_MEMORY_ARRAY_WRITE(buffer, num_entries, sizeof(u32_t), ssf); _impl_k_stack_init((struct k_stack *)stack, (u32_t *)buffer, num_entries);