From 301587065ab424c8696788cc015dd0deac081db2 Mon Sep 17 00:00:00 2001 From: Dmitrii Kuvaiskii Date: Wed, 29 Apr 2020 17:11:21 -0700 Subject: [PATCH] [LibOS,Pal] Make Graphene build with GCC 9.3 GCC 9.3 adds more static checks on C headers and sources. This commit fixes all detected issues (mainly possible NULL pointer dereferences and VLAs on stack). --- LibOS/shim/include/shim_flags_conv.h | 2 +- LibOS/shim/src/ipc/shim_ipc_pid.c | 2 +- Pal/regression/AttestationReport.c | 50 ++++++++++++++++++++-------- Pal/regression/Directory.c | 3 +- Pal/regression/File.c | 18 ++++++---- Pal/regression/Hex.c | 12 ++++--- Pal/regression/Tcp.c | 7 ++-- Pal/regression/Udp.c | 5 +-- Pal/regression/normalize_path.c | 4 +-- Pal/src/db_rtld.c | 10 +++++- Pal/src/host/Linux/db_sockets.c | 8 +++-- 11 files changed, 83 insertions(+), 38 deletions(-) diff --git a/LibOS/shim/include/shim_flags_conv.h b/LibOS/shim/include/shim_flags_conv.h index ed135ffe..ccab52ed 100644 --- a/LibOS/shim/include/shim_flags_conv.h +++ b/LibOS/shim/include/shim_flags_conv.h @@ -22,7 +22,7 @@ #define SHIM_FLAGS_CONV_H #include -#include +#include #include #include "assert.h" diff --git a/LibOS/shim/src/ipc/shim_ipc_pid.c b/LibOS/shim/src/ipc/shim_ipc_pid.c index 248d9bc9..19bb616c 100644 --- a/LibOS/shim/src/ipc/shim_ipc_pid.c +++ b/LibOS/shim/src/ipc/shim_ipc_pid.c @@ -536,7 +536,7 @@ int ipc_pid_retmeta_callback(IPC_CALLBACK_ARGS) { } int get_pid_port(IDTYPE pid, IDTYPE* dest, struct shim_ipc_port** port) { - IDTYPE owner; + IDTYPE owner = 0; int ret; if ((ret = connect_owner(pid, port, &owner)) < 0) diff --git a/Pal/regression/AttestationReport.c b/Pal/regression/AttestationReport.c index e27432bf..b7a79bd1 100644 --- a/Pal/regression/AttestationReport.c +++ b/Pal/regression/AttestationReport.c @@ -3,6 +3,10 @@ #include "pal_debug.h" #include "sgx_arch.h" +#define ALLOC_ALIGN_UP(addr) ALIGN_UP_POW2(addr, pal_control.alloc_align) + +char zerobuf[sizeof(sgx_report_t)] = {0}; + int main(int argc, char** argv) { bool ret; @@ -35,32 +39,46 @@ int main(int argc, char** argv) { pal_printf("user_report_data_size = %lu, target_info_size = %lu, report_size = %lu\n", user_report_data_size, target_info_size, report_size); - char user_report_data[user_report_data_size]; - char target_info[target_info_size]; - char report[report_size]; + char* user_report_data = DkVirtualMemoryAlloc(NULL, ALLOC_ALIGN_UP(user_report_data_size), 0, + PAL_PROT_READ | PAL_PROT_WRITE); + if (!user_report_data) { + pal_printf("ERROR: Cannot allocate memory for user_report_data\n"); + return -1; + } - memset(&user_report_data, 'A', sizeof(user_report_data)); - memset(&target_info, 0, sizeof(target_info)); - memset(&report, 0, sizeof(report)); + char* target_info = DkVirtualMemoryAlloc(NULL, ALLOC_ALIGN_UP(target_info_size), 0, + PAL_PROT_READ | PAL_PROT_WRITE); + if (!target_info) { + pal_printf("ERROR: Cannot allocate memory for target_info\n"); + return -1; + } - ret = DkAttestationReport(&user_report_data, &user_report_data_size, - &target_info, &target_info_size, - &report, &report_size); + char* report = DkVirtualMemoryAlloc(NULL, ALLOC_ALIGN_UP(report_size), 0, + PAL_PROT_READ | PAL_PROT_WRITE); + if (!report) { + pal_printf("ERROR: Cannot allocate memory for report\n"); + return -1; + } + + memset(user_report_data, 'A', user_report_data_size); + memset(target_info, 0, target_info_size); + memset(report, 0, report_size); + + ret = DkAttestationReport(user_report_data, &user_report_data_size, + target_info, &target_info_size, + report, &report_size); if (!ret) { pal_printf("ERROR: DkAttestationReport() to get SGX report failed\n"); return -1; } - sgx_report_t* sgx_report = (sgx_report_t*)&report; - if (memcmp(&sgx_report->body.report_data.d, &user_report_data, + sgx_report_t* sgx_report = (sgx_report_t*)report; + if (memcmp(&sgx_report->body.report_data.d, user_report_data, sizeof(sgx_report->body.report_data.d))) { pal_printf("ERROR: DkAttestationReport() returned SGX report with wrong report_data\n"); return -1; } - char zerobuf[report_size]; - memset(&zerobuf, 0, sizeof(zerobuf)); - if (memcmp(&sgx_report->body.reserved1, &zerobuf, sizeof(sgx_report->body.reserved1)) || memcmp(&sgx_report->body.reserved2, &zerobuf, sizeof(sgx_report->body.reserved2)) || memcmp(&sgx_report->body.reserved3, &zerobuf, sizeof(sgx_report->body.reserved3)) || @@ -71,6 +89,10 @@ int main(int argc, char** argv) { return -1; } + DkVirtualMemoryFree(user_report_data, ALLOC_ALIGN_UP(user_report_data_size)); + DkVirtualMemoryFree(target_info, ALLOC_ALIGN_UP(target_info_size)); + DkVirtualMemoryFree(report, ALLOC_ALIGN_UP(report_size)); + pal_printf("Success\n"); return 0; } diff --git a/Pal/regression/Directory.c b/Pal/regression/Directory.c index 2243a948..c26166e2 100644 --- a/Pal/regression/Directory.c +++ b/Pal/regression/Directory.c @@ -2,6 +2,8 @@ #include "pal.h" #include "pal_debug.h" +char buffer[80]; + int main(int argc, char** argv, char** envp) { /* test regular directory opening */ @@ -13,7 +15,6 @@ int main(int argc, char** argv, char** envp) { if (DkStreamAttributesQueryByHandle(dir1, &attr1)) pal_printf("Query by Handle: type = %d\n", attr1.handle_type); - char buffer[80]; int bytes = DkStreamRead(dir1, 0, 80, buffer, NULL, 0); if (bytes) { for (char* c = buffer; c < buffer + bytes; c += strlen(c) + 1) diff --git a/Pal/regression/File.c b/Pal/regression/File.c index b227def8..efe94f0d 100644 --- a/Pal/regression/File.c +++ b/Pal/regression/File.c @@ -3,20 +3,24 @@ #include "pal_debug.h" #define NUM_TO_HEX(num) ((num) >= 10 ? 'a' + ((num) - 10) : '0' + (num)) +#define BUF_SIZE 40 -static __attribute__((noinline)) void print_hex(char* fmt, const void* data, int len) { - char* buf = __alloca(len * 2 + 1); - buf[len * 2] = '\0'; +char buffer1[BUF_SIZE]; +char buffer2[BUF_SIZE]; +char buffer3[BUF_SIZE]; +char hex_buf[BUF_SIZE * 2 + 1]; + +static void print_hex(char* fmt, const void* data, int len) { + hex_buf[len * 2] = '\0'; for (int i = 0; i < len; i++) { unsigned char b = ((unsigned char*)data)[i]; - buf[i * 2] = NUM_TO_HEX(b >> 4); - buf[i * 2 + 1] = NUM_TO_HEX(b & 0xf); + hex_buf[i * 2] = NUM_TO_HEX(b >> 4); + hex_buf[i * 2 + 1] = NUM_TO_HEX(b & 0xf); } - pal_printf(fmt, buf); + pal_printf(fmt, hex_buf); } int main(int argc, char** argv, char** envp) { - char buffer1[40], buffer2[40], buffer3[40]; int ret; /* test regular file opening */ diff --git a/Pal/regression/Hex.c b/Pal/regression/Hex.c index d219c858..b88161fb 100644 --- a/Pal/regression/Hex.c +++ b/Pal/regression/Hex.c @@ -6,15 +6,19 @@ #include "api.h" #include "pal.h" +char x[] = {0xde, 0xad, 0xbe, 0xef}; +char y[] = {0xcd, 0xcd, 0xcd, 0xcd, 0xcd, 0xcd, 0xcd, 0xcd}; + +static_assert(sizeof(x) <= sizeof(y), "array x is longer than array y"); +char hex_buf[sizeof(y) * 2 + 1]; + noreturn void __abort(void) { // ENOTRECOVERABLE = 131 DkProcessExit(-131); } int main(void) { - char x[] = {0xde, 0xad, 0xbe, 0xef}; - char y[] = {0xcd, 0xcd, 0xcd, 0xcd, 0xcd, 0xcd, 0xcd, 0xcd}; - pal_printf("Hex test 1 is %s\n", ALLOCA_BYTES2HEXSTR(x)); - pal_printf("Hex test 2 is %s\n", ALLOCA_BYTES2HEXSTR(y)); + pal_printf("Hex test 1 is %s\n", BYTES2HEXSTR(x, hex_buf, sizeof(hex_buf))); + pal_printf("Hex test 2 is %s\n", BYTES2HEXSTR(y, hex_buf, sizeof(hex_buf))); return 0; } diff --git a/Pal/regression/Tcp.c b/Pal/regression/Tcp.c index 253ee4d8..4f948b36 100644 --- a/Pal/regression/Tcp.c +++ b/Pal/regression/Tcp.c @@ -5,15 +5,17 @@ #define PORT 8000 #define NTRIES 10 +char addr[40]; +char time_arg[24]; +char buffer[12]; + int main(int argc, char** argv) { - char addr[40]; int i; if (argc == 1) { unsigned long time = DkSystemTimeQuery(); pal_printf("start time = %lu\n", time); - char time_arg[24]; snprintf(time_arg, 24, "%ld", time); const char* newargs[4] = {"Tcp", time_arg, NULL}; @@ -67,7 +69,6 @@ int main(int argc, char** argv) { DkStreamGetName(cli, addr, 40); pal_printf("client connected on %s\n", addr); - char buffer[12]; int bytes = DkStreamRead(cli, 0, 12, buffer, NULL, 0); if (!bytes) { diff --git a/Pal/regression/Udp.c b/Pal/regression/Udp.c index 52c1c627..0fc4222e 100644 --- a/Pal/regression/Udp.c +++ b/Pal/regression/Udp.c @@ -4,8 +4,10 @@ #define NTRIES 10 +char addr[40]; +char buffer[20]; + int main(int argc, char** argv) { - char addr[40]; int i; if (argc == 1) { @@ -26,7 +28,6 @@ int main(int argc, char** argv) { PAL_HANDLE proc = DkProcessCreate("file:Udp", newargs); for (i = 0; i < NTRIES; i++) { - char buffer[20]; int bytes = DkStreamRead(srv, 0, 20, buffer, addr, 40); if (!bytes) { diff --git a/Pal/regression/normalize_path.c b/Pal/regression/normalize_path.c index cc773021..15b1e987 100644 --- a/Pal/regression/normalize_path.c +++ b/Pal/regression/normalize_path.c @@ -49,9 +49,9 @@ static size_t cases_len; static int (*func_to_test)(const char*, char*, size_t*); static const char* func_name; -static int run_test(void) { - char buf[URI_MAX] = {0}; +char buf[URI_MAX] = {0}; +static int run_test(void) { for (size_t i = 0; i < cases_len; i++) { size_t size = sizeof(buf); int ret = func_to_test(cases[i][0], buf, &size); diff --git a/Pal/src/db_rtld.c b/Pal/src/db_rtld.c index da70f10f..b77506cf 100644 --- a/Pal/src/db_rtld.c +++ b/Pal/src/db_rtld.c @@ -480,7 +480,13 @@ int load_elf_object (const char * uri, enum object_type type) int add_elf_object(void * addr, PAL_HANDLE handle, int type) { + if (!addr) + return -PAL_ERROR_INVAL; + struct link_map * map = new_elf_object(_DkStreamRealpath(handle), type); + if (!map) + return -PAL_ERROR_NOMEM; + const ElfW(Ehdr) * header = (void *) addr; const ElfW(Phdr) * ph, * phdr = (ElfW(Phdr) *) ((char *) addr + header->e_phoff); @@ -944,11 +950,13 @@ void DkDebugAttachBinary (PAL_STR uri, PAL_PTR start_addr) __UNUSED(uri); __UNUSED(start_addr); #else - if (!strstartswith_static(uri, URI_PREFIX_FILE)) + if (!strstartswith_static(uri, URI_PREFIX_FILE) || !start_addr) return; const char * realname = uri + URI_PREFIX_FILE_LEN; struct link_map * l = new_elf_object(realname, OBJECT_EXTERNAL); + if (!l) + return; /* This is the ELF header. We read it in `open_verify'. */ const ElfW(Ehdr) * header = (ElfW(Ehdr) *) start_addr; diff --git a/Pal/src/host/Linux/db_sockets.c b/Pal/src/host/Linux/db_sockets.c index ebe5a09a..e8745895 100644 --- a/Pal/src/host/Linux/db_sockets.c +++ b/Pal/src/host/Linux/db_sockets.c @@ -329,7 +329,9 @@ static int tcp_listen(PAL_HANDLE* handle, char* uri, int create, int options) { if ((ret = socket_parse_uri(uri, &bind_addr, &bind_addrlen, NULL, NULL)) < 0) return ret; - assert(bind_addr); + if (!bind_addr) + return -PAL_ERROR_DENIED; + assert(bind_addrlen == addr_size(bind_addr)); #if ALLOW_BIND_ANY == 0 @@ -624,7 +626,9 @@ static int udp_bind(PAL_HANDLE* handle, char* uri, int create, int options) { if ((ret = socket_parse_uri(uri, &bind_addr, &bind_addrlen, NULL, NULL)) < 0) return ret; - assert(bind_addr); + if (!bind_addr) + return -PAL_ERROR_DENIED; + assert(bind_addrlen == addr_size(bind_addr)); #if ALLOW_BIND_ANY == 0