From 8f7fa8fd0b0af9b963cc05a13ebe1dd913f966f4 Mon Sep 17 00:00:00 2001 From: Vijay Dhanraj Date: Mon, 20 Sep 2021 11:23:37 -0700 Subject: [PATCH] fixup! [Pal,LibOS] Refactor sysfs to improve sanitization --- LibOS/shim/include/shim_fs_pseudo.h | 20 +-- LibOS/shim/src/fs/sys/cache_info.c | 8 +- LibOS/shim/src/fs/sys/cpu_info.c | 16 +-- LibOS/shim/src/fs/sys/fs.c | 40 +++--- LibOS/shim/src/fs/sys/node_info.c | 13 +- Pal/include/arch/x86_64/pal-arch.h | 16 +-- Pal/include/host/Linux-common/topo_info.h | 4 +- Pal/src/host/Linux-SGX/db_main.c | 94 ++++++------- Pal/src/host/Linux-common/topo_info.c | 152 +++++++++++----------- 9 files changed, 176 insertions(+), 187 deletions(-) diff --git a/LibOS/shim/include/shim_fs_pseudo.h b/LibOS/shim/include/shim_fs_pseudo.h index 38a348a066..f78fec9fa1 100644 --- a/LibOS/shim/include/shim_fs_pseudo.h +++ b/LibOS/shim/include/shim_fs_pseudo.h @@ -209,18 +209,18 @@ int sys_cache_load(struct shim_dentry* dent, char** out_data, size_t* out_size); bool sys_cpu_online_name_exists(struct shim_dentry* parent, const char* name); int sys_cpu_online_list_names(struct shim_dentry* parent, readdir_callback_t callback, void* arg); -/* Converts integers to a string. If size multiplier like 'K', 'M' or 'G' is passed, it is appended +/* Converts integer to a string. If size multiplier like 'K', 'M' or 'G' is passed, it is appended * to the string. */ -int sys_convert_int_to_str(uint64_t val, uint64_t size_mult, char* str, int max_len); +int sys_convert_int_to_str(uint64_t val, uint64_t size_mult, char* str, size_t max_len); -/* Converts integer ranges to a string. For example if ranges.start and ranges.end were 0 and 63 - * respectively, then `0-63` string is generated. */ -int sys_convert_range_to_str(const PAL_RES_RANGE_INFO* res_range_info, char* str, int max_len, - const char* sep); +/* Converts array of integer range(s) to a string. For example if ranges[0].start and ranges[0].end + * were 0 and 63 respectively, then `0-63` string is generated. */ +int sys_convert_ranges_to_str(const PAL_RES_RANGE_INFO* res_range_info, char* str, size_t max_len, + const char* sep); -/* Converts integer ranges to a cpu bitmap string. For example, if ranges.start and ranges.end were - * 32 and 63, then `ffffffff,00000000` string is generated. */ -int sys_convert_range_to_cpu_bitmap_str(const PAL_RES_RANGE_INFO* res_range_info, char* str, - int max_len); +/* Converts array of integer range(s) to a cpu bitmap string. For example, if ranges[0].start and + * ranges[0].end were 0 and 31 respectively, then `00000000,ffffffff` string is generated. */ +int sys_convert_ranges_to_cpu_bitmap_str(const PAL_RES_RANGE_INFO* res_range_info, char* str, + size_t max_len); #endif /* SHIM_FS_PSEUDO_H_ */ diff --git a/LibOS/shim/src/fs/sys/cache_info.c b/LibOS/shim/src/fs/sys/cache_info.c index 60c26777a9..838a61be0f 100644 --- a/LibOS/shim/src/fs/sys/cache_info.c +++ b/LibOS/shim/src/fs/sys/cache_info.c @@ -29,18 +29,18 @@ int sys_cache_load(struct shim_dentry* dent, char** out_data, size_t* out_size) PAL_CORE_CACHE_INFO* cache = &g_pal_control->topo_info.core_topology[cpu_num].cache[cache_num]; char str[PAL_SYSFS_MAP_FILESZ] = {'\0'}; if (strcmp(name, "shared_cpu_map") == 0) { - ret = sys_convert_range_to_cpu_bitmap_str(&cache->shared_cpu_map, str, sizeof(str)); + ret = sys_convert_ranges_to_cpu_bitmap_str(&cache->shared_cpu_map, str, sizeof(str)); } else if (strcmp(name, "level") == 0) { ret = sys_convert_int_to_str(cache->level, MULTIPLIER_NONE, str, sizeof(str)); } else if (strcmp(name, "type") == 0) { switch (cache->type) { - case DATA: + case CACHE_TYPE_DATA: ret = snprintf(str, sizeof(str), "%s", "Data\n"); break; - case INSTRUCTION: + case CACHE_TYPE_INSTRUCTION: ret = snprintf(str, sizeof(str), "%s", "Instruction\n"); break; - case UNIFIED: + case CACHE_TYPE_UNIFIED: ret = snprintf(str, sizeof(str), "%s", "Unified\n"); break; default: diff --git a/LibOS/shim/src/fs/sys/cpu_info.c b/LibOS/shim/src/fs/sys/cpu_info.c index 18ba925cbf..4140374d1d 100644 --- a/LibOS/shim/src/fs/sys/cpu_info.c +++ b/LibOS/shim/src/fs/sys/cpu_info.c @@ -13,16 +13,16 @@ #include "shim_fs_pseudo.h" int sys_cpu_general_load(struct shim_dentry* dent, char** out_data, size_t* out_size) { - int ret = 0; + int ret; const char* name = dent->name; char str[PAL_SYSFS_BUF_FILESZ] = {'\0'}; if (strcmp(name, "online") == 0) { - ret = sys_convert_range_to_str(&g_pal_control->topo_info.online_logical_cores, str, - sizeof(str), ","); + ret = sys_convert_ranges_to_str(&g_pal_control->topo_info.online_logical_cores, str, + sizeof(str), ","); } else if (strcmp(name, "possible") == 0) { - ret = sys_convert_range_to_str(&g_pal_control->topo_info.possible_logical_cores, str, - sizeof(str), ","); + ret = sys_convert_ranges_to_str(&g_pal_control->topo_info.possible_logical_cores, str, + sizeof(str), ","); } else { log_debug("unrecognized file: %s", name); ret = -ENOENT; @@ -54,10 +54,10 @@ int sys_cpu_load(struct shim_dentry* dent, char** out_data, size_t* out_size) { } else if (strcmp(name, "physical_package_id") == 0) { ret = sys_convert_int_to_str(core_topology->cpu_socket, MULTIPLIER_NONE, str, sizeof(str)); } else if (strcmp(name, "core_siblings") == 0) { - ret = sys_convert_range_to_cpu_bitmap_str(&core_topology->core_siblings, str, sizeof(str)); + ret = sys_convert_ranges_to_cpu_bitmap_str(&core_topology->core_siblings, str, sizeof(str)); } else if (strcmp(name, "thread_siblings") == 0) { - ret = sys_convert_range_to_cpu_bitmap_str(&core_topology->thread_siblings, str, - sizeof(str)); + ret = sys_convert_ranges_to_cpu_bitmap_str(&core_topology->thread_siblings, str, + sizeof(str)); } else { log_debug("unrecognized file: %s", name); ret = -ENOENT; diff --git a/LibOS/shim/src/fs/sys/fs.c b/LibOS/shim/src/fs/sys/fs.c index 31c339ae36..ad514ec7e7 100644 --- a/LibOS/shim/src/fs/sys/fs.c +++ b/LibOS/shim/src/fs/sys/fs.c @@ -14,8 +14,10 @@ #include "shim_fs_pseudo.h" #include "stat.h" -int sys_convert_int_to_str(uint64_t val, uint64_t size_mult, char* str, int max_len) { +int sys_convert_int_to_str(uint64_t val, uint64_t size_mult, char* str, size_t max_len) { int ret = 0; + assert(max_len > 0); + switch (size_mult) { case MULTIPLIER_KB: ret = snprintf(str, max_len, "%luK", val); @@ -33,47 +35,45 @@ int sys_convert_int_to_str(uint64_t val, uint64_t size_mult, char* str, int max_ return ret; } -int sys_convert_range_to_str(const PAL_RES_RANGE_INFO* res_range_info, char* str, int max_len, - const char* sep) { +int sys_convert_ranges_to_str(const PAL_RES_RANGE_INFO* res_range_info, char* str, size_t max_len, + const char* sep) { if (res_range_info->range_count > INT64_MAX) return -EINVAL; int64_t range_cnt = (int64_t)res_range_info->range_count; - int offset = 0; + size_t offset = 0; for (int64_t i = 0; i < range_cnt; i++) { - if (max_len - offset < 0) + if (offset > max_len) return -ENOMEM; int ret; - char end_str[PAL_SYSFS_BUF_FILESZ] = {'\0'}; - if (res_range_info->ranges[i].end == UINT64_MAX) { - ret = snprintf(end_str, sizeof(end_str), "%s", ""); + if (res_range_info->ranges[i].end == res_range_info->ranges[i].start) { + ret = snprintf(str + offset, max_len - offset, "%lu%s", res_range_info->ranges[i].start, + (i + 1 == range_cnt) ? "\0" : sep); } else { - ret = snprintf(end_str, sizeof(end_str), "-%lu", res_range_info->ranges[i].end); + ret = snprintf(str + offset, max_len - offset, "%lu-%lu%s", + res_range_info->ranges[i].start, res_range_info->ranges[i].end, + (i + 1 == range_cnt) ? "\0" : sep); } if (ret < 0) return ret; - ret = snprintf(str + offset, max_len - offset, "%lu%s%s", res_range_info->ranges[i].start, - end_str, (i + 1 == range_cnt) ? "\0" : sep); - if (ret < 0) - return ret; offset += ret; } return 0; } -int sys_convert_range_to_cpu_bitmap_str(const PAL_RES_RANGE_INFO* res_range_info, char* str, - int max_len) { +int sys_convert_ranges_to_cpu_bitmap_str(const PAL_RES_RANGE_INFO* res_range_info, char* str, + size_t max_len) { if (g_pal_control->topo_info.possible_logical_cores.resource_count > INT64_MAX) return -1; - int ret = 0; + int ret; /* Extract cpumask from the ranges */ int64_t possible_cores = g_pal_control->topo_info.possible_logical_cores.resource_count; int64_t num_cpumask = BITS_TO_INTS(possible_cores); - unsigned int* bitmap = (unsigned int*)calloc(num_cpumask, sizeof(unsigned int)); + uint32_t* bitmap = (uint32_t*)calloc(num_cpumask, sizeof(uint32_t)); if (!bitmap) return -ENOMEM; @@ -83,8 +83,6 @@ int sys_convert_range_to_cpu_bitmap_str(const PAL_RES_RANGE_INFO* res_range_info for (int64_t i = 0; i < (int64_t)res_range_info->range_count; i++) { uint64_t start = res_range_info->ranges[i].start; uint64_t end = res_range_info->ranges[i].end; - if (end == UINT64_MAX) - end = start; if (start > INT64_MAX || end > INT64_MAX) return -EINVAL; for (int64_t j = (int64_t)start; j <= (int64_t)end; j++) { @@ -98,9 +96,9 @@ int sys_convert_range_to_cpu_bitmap_str(const PAL_RES_RANGE_INFO* res_range_info } /* Convert cpumask to strings */ - int offset = 0; + size_t offset = 0; for (int64_t j = num_cpumask - 1; j >= 0; j-- ) { - if (max_len - offset < 0) { + if (offset > max_len) { ret = -ENOMEM; goto out; } diff --git a/LibOS/shim/src/fs/sys/node_info.c b/LibOS/shim/src/fs/sys/node_info.c index f7885bcdc5..c18c20bedd 100644 --- a/LibOS/shim/src/fs/sys/node_info.c +++ b/LibOS/shim/src/fs/sys/node_info.c @@ -12,11 +12,12 @@ #include "shim_fs_pseudo.h" int sys_node_general_load(struct shim_dentry* dent, char** out_data, size_t* out_size) { + int ret; + const char* name = dent->name; - char str[PAL_SYSFS_BUF_FILESZ] = {'\0'}; - int ret = 0; + char str[PAL_SYSFS_BUF_FILESZ] = {'\0'};; if (strcmp(name, "online") == 0) { - ret = sys_convert_range_to_str(&g_pal_control->topo_info.nodes, str, sizeof(str), ","); + ret = sys_convert_ranges_to_str(&g_pal_control->topo_info.nodes, str, sizeof(str), ","); } else { log_debug("unrecognized file: %s", name); ret = -ENOENT; @@ -38,9 +39,9 @@ int sys_node_load(struct shim_dentry* dent, char** out_data, size_t* out_size) { PAL_NUMA_TOPO_INFO* numa_topology = &g_pal_control->topo_info.numa_topology[node_num]; char str[PAL_SYSFS_MAP_FILESZ] = {'\0'}; if (strcmp(name, "cpumap" ) == 0) { - ret = sys_convert_range_to_cpu_bitmap_str(&numa_topology->cpumap, str, sizeof(str)); + ret = sys_convert_ranges_to_cpu_bitmap_str(&numa_topology->cpumap, str, sizeof(str)); } else if (strcmp(name, "distance") == 0) { - ret = sys_convert_range_to_str(&numa_topology->distance, str, sizeof(str), " "); + ret = sys_convert_ranges_to_str(&numa_topology->distance, str, sizeof(str), " "); } else if (strcmp(name, "nr_hugepages") == 0) { const char* parent_name = dent->parent->name; if (strcmp(parent_name, "hugepages-2048kB") == 0) { @@ -49,6 +50,8 @@ int sys_node_load(struct shim_dentry* dent, char** out_data, size_t* out_size) { } else if (strcmp(parent_name, "hugepages-1048576kB") == 0) { ret = sys_convert_int_to_str(numa_topology->nr_hugepages[HUGEPAGES_1G], MULTIPLIER_NONE, str, sizeof(str)); + } else { + log_debug("unrecognized hugepage file: %s", parent_name); } } else { log_debug("unrecognized file: %s", name); diff --git a/Pal/include/arch/x86_64/pal-arch.h b/Pal/include/arch/x86_64/pal-arch.h index 4df8b743a6..6f74bcb986 100644 --- a/Pal/include/arch/x86_64/pal-arch.h +++ b/Pal/include/arch/x86_64/pal-arch.h @@ -31,12 +31,10 @@ typedef struct pal_tcb PAL_TCB; #define STACK_PROTECTOR_CANARY_DEFAULT 0xbadbadbadbadUL /* Used to represent buffers having numeric values with text. E.g "1024576K". - * Note: This MACRO is used to allocate on the stack, so increase the size if needed with caution or - * use malloc. */ + * NOTE: Used to allocate on stack; increase with caution or use malloc instead. */ #define PAL_SYSFS_BUF_FILESZ 64 /* Used to represent cpumaps like "00000000,ffffffff,00000000,ffffffff". - * Note: This MACRO is used to allocate on the stack, so increase the size if needed with caution or - * use malloc. */ + * NOTE: Used to allocate on stack; increase with caution or use malloc instead. */ #define PAL_SYSFS_MAP_FILESZ 256 typedef struct pal_tcb { @@ -288,9 +286,9 @@ enum { }; enum { - DATA = 0, - INSTRUCTION, - UNIFIED, + CACHE_TYPE_DATA = 0, + CACHE_TYPE_INSTRUCTION, + CACHE_TYPE_UNIFIED, }; /* PAL_CPU_INFO holds /proc/cpuinfo data */ @@ -306,13 +304,13 @@ typedef struct PAL_CPU_INFO_ { typedef struct PAL_RANGE_INFO_ { PAL_NUM start; - PAL_NUM end; /* If end is not present, set this to UINT64_MAX as end marker. */ + PAL_NUM end; } PAL_RANGE_INFO; typedef struct PAL_RES_RANGE_INFO_ { /* Count of total number of resources present. Eg. 0-63 will result in this count being 64 */ PAL_NUM resource_count; - /* Count of total number of ranges present. Eg. 0-31,32-63, will result in this count being 2 */ + /* Count of total number of ranges present. Eg. 0-31,32-63 will result in this count being 2 */ PAL_NUM range_count; PAL_RANGE_INFO* ranges; } PAL_RES_RANGE_INFO; diff --git a/Pal/include/host/Linux-common/topo_info.h b/Pal/include/host/Linux-common/topo_info.h index 7a930c306e..2feddccb9a 100644 --- a/Pal/include/host/Linux-common/topo_info.h +++ b/Pal/include/host/Linux-common/topo_info.h @@ -11,8 +11,8 @@ /* Opens a pseudo-file describing HW resources such as online CPUs and counts the number of * HW resources present in the file (if count == true) and stores the result in `PAL_RES_RANGE_INFO` * struct if provided or simply reads the integer stored in the file (if count == false). If - * `size_multiplier` is passed, then size qualifier like "K"/"M"/"G" are stored while reading the - * integer. For example on a single-core machine, calling this function on + * `size_mult` is passed, then numerical representation of size qualifier like "K"/"M"/"G" is + * stored while reading the integer. For example on a single-core machine, calling this function on * `/sys/devices/system/cpu/online` with count == true will return 1 and 0 with count == false. * Returns UNIX error code on failure. * N.B: Understands complex formats like "1,3-5,6" when called with count == true. diff --git a/Pal/src/host/Linux-SGX/db_main.c b/Pal/src/host/Linux-SGX/db_main.c index 06ea804f29..d3502d8cce 100644 --- a/Pal/src/host/Linux-SGX/db_main.c +++ b/Pal/src/host/Linux-SGX/db_main.c @@ -154,7 +154,9 @@ static int copy_hw_resource_range(PAL_RES_RANGE_INFO* src, PAL_RES_RANGE_INFO* d return 0; } -/* This fucntion does the following 3 sanitizations for a given resource range: +/* All sanitization functions below do not free memory. We simply exit on failure. */ + +/* This function does the following 3 sanitizations for a given resource range: * 1. Ensures the resource as well as range count doesn't exceed limits. * 2. Ensures that ranges don't overlap like "1-5, 3-4". * 3. Ensures the ranges aren't malformed like "1-5, 7-1". @@ -188,28 +190,24 @@ static int sanitize_hw_resource_range(PAL_RES_RANGE_INFO res_info, int64_t res_m return -1; int64_t start = (int64_t)res_info.ranges[i].start; - int64_t end; - if (res_info.ranges[i].end == UINT64_MAX) - end = start; - else if (res_info.ranges[i].end <= INT64_MAX) - end = (int64_t)res_info.ranges[i].end; - else + if (res_info.ranges[i].end > INT64_MAX) return -1; + int64_t end = (int64_t)res_info.ranges[i].end; /* Ensure start and end fall within the max_limit value */ if (!IS_IN_RANGE_INCL(start, range_min_limit, range_max_limit)) { - log_error("Invalid start range: %ld\n", start); + log_error("Invalid start range: %ld", start); return -1; } if ((start != end) && !IS_IN_RANGE_INCL(end, start + 1, range_max_limit)) { - log_error("Invalid end range: %ld\n", start); + log_error("Invalid end range: %ld", end); return -1; } /* check for malformed ranges */ if (previous_end >= end) { - log_error("Malformed range: previous_end =%ld, current end = %ld\n", previous_end, end); + log_error("Malformed range: previous_end = %ld, current end = %ld", previous_end, end); return -1; } previous_end = end; @@ -221,13 +219,13 @@ static int sanitize_hw_resource_range(PAL_RES_RANGE_INFO res_info, int64_t res_m static int sanitize_cache_topology_info(PAL_CORE_CACHE_INFO* cache, int64_t cache_lvls, int64_t num_cores) { for (int64_t lvl = 0; lvl < cache_lvls; lvl++) { - if (cache[lvl].type != DATA && cache[lvl].type != INSTRUCTION && - cache[lvl].type != UNIFIED) { - return -EINVAL; + if (cache[lvl].type != CACHE_TYPE_DATA && cache[lvl].type != CACHE_TYPE_INSTRUCTION && + cache[lvl].type != CACHE_TYPE_UNIFIED) { + return -1; } int64_t max_limit; - if (cache[lvl].type == DATA || cache[lvl].type == INSTRUCTION) { + if (cache[lvl].type == CACHE_TYPE_DATA || cache[lvl].type == CACHE_TYPE_INSTRUCTION) { max_limit = 2; /* Taking HT into account */ } else { /* if unified cache then it can range upto total number of cores. */ @@ -244,7 +242,7 @@ static int sanitize_cache_topology_info(PAL_CORE_CACHE_INFO* cache, int64_t cach return -1; int64_t level = (int64_t)cache[lvl].level; if (!IS_IN_RANGE_INCL(level, 1, 3)) /* x86 processors have max of 3 cache levels */ - return -EINVAL; + return -1; if (cache[lvl].size_multiplier != MULTIPLIER_KB && cache[lvl].size_multiplier != MULTIPLIER_MB && @@ -253,15 +251,13 @@ static int sanitize_cache_topology_info(PAL_CORE_CACHE_INFO* cache, int64_t cach return -1; } - int64_t multiplier; + int64_t multiplier =1; if (cache[lvl].size_multiplier == MULTIPLIER_KB) multiplier = 1024; else if (cache[lvl].size_multiplier == MULTIPLIER_MB) multiplier = 1024 * 1024; else if (cache[lvl].size_multiplier == MULTIPLIER_GB) multiplier = 1024 * 1024 * 1024; - else - multiplier = 1; if (cache[lvl].size > INT64_MAX) return -1; @@ -270,25 +266,25 @@ static int sanitize_cache_topology_info(PAL_CORE_CACHE_INFO* cache, int64_t cach return -1; if (!IS_IN_RANGE_INCL(cache_size, 1, 1 << 30)) - return -EINVAL; + return -1; if (cache[lvl].coherency_line_size > INT64_MAX) return -1; int64_t coherency_line_size = (int64_t)cache[lvl].coherency_line_size; if (!IS_IN_RANGE_INCL(coherency_line_size, 1, 1 << 16)) - return -EINVAL; + return -1; if (cache[lvl].number_of_sets > INT64_MAX) return -1; int64_t number_of_sets = (int64_t)cache[lvl].number_of_sets; if (!IS_IN_RANGE_INCL(number_of_sets, 1, 1 << 30)) - return -EINVAL; + return -1; if (cache[lvl].physical_line_partition > INT64_MAX) return -1; int64_t physical_line_partition = (int64_t)cache[lvl].physical_line_partition; if (!IS_IN_RANGE_INCL(physical_line_partition, 1, 1 << 16)) - return -EINVAL; + return -1; } return 0; } @@ -296,23 +292,23 @@ static int sanitize_cache_topology_info(PAL_CORE_CACHE_INFO* cache, int64_t cach static int sanitize_core_topology_info(PAL_CORE_TOPO_INFO* core_topology, int64_t num_cores, int64_t cache_lvls) { if (num_cores == 0 || cache_lvls == 0) - return -ENOENT; + return -1; for (int64_t idx = 0; idx < num_cores; idx++) { if (idx != 0) { /* core 0 is always online */ if (core_topology[idx].is_logical_core_online > INT64_MAX) - return -1; + return -1; int64_t is_core_online = core_topology[idx].is_logical_core_online; if (is_core_online != 0 && is_core_online != 1) - return -EINVAL; + return -1; } if (core_topology[idx].core_id > INT64_MAX) return -1; int64_t core_id = (int64_t)core_topology[idx].core_id; if (!IS_IN_RANGE_INCL(core_id, 0, num_cores - 1)) - return -EINVAL; + return -1; int64_t core_siblings = sanitize_hw_resource_range(core_topology[idx].core_siblings, 1, num_cores, 0, num_cores); @@ -330,24 +326,20 @@ static int sanitize_core_topology_info(PAL_CORE_TOPO_INFO* core_topology, int64_ } if (sanitize_cache_topology_info(core_topology[idx].cache, cache_lvls, num_cores) < 0) - return -EINVAL; + return -1; } return 0; } -typedef struct PAL_SOCKET_INFO_ { - PAL_NUM range_count; - PAL_RANGE_INFO* ranges; -} PAL_SOCKET_INFO; - static int sanitize_socket_info(PAL_TOPO_INFO topo_info) { int ret = 0; int64_t prev_socket = -1; int64_t num_sockets = (int64_t)topo_info.num_sockets; - PAL_SOCKET_INFO* socket_info = (PAL_SOCKET_INFO*)calloc(num_sockets, sizeof(PAL_SOCKET_INFO)); + PAL_RES_RANGE_INFO * socket_info = + (PAL_RES_RANGE_INFO*)calloc(num_sockets, sizeof(PAL_RES_RANGE_INFO )); if (!socket_info) - return -ENOMEM; + return -1; int64_t num_cores = topo_info.online_logical_cores.resource_count; for (int64_t idx = 0; idx < num_cores; idx++) { @@ -356,38 +348,38 @@ static int sanitize_socket_info(PAL_TOPO_INFO topo_info) { int64_t socket = (int64_t)topo_info.core_topology[idx].cpu_socket; if (!IS_IN_RANGE_INCL(socket, 0, num_sockets - 1)) { - ret = -EINVAL; + ret = -1; goto out_socket; } /* Extract cores that are part of each socket to validate against core_siblings. * Note: Although a clever attacker might modify both of these values, idea here is to * provide a consistent view of the topology. */ - int64_t range_count; if (socket != prev_socket) { socket_info[socket].range_count++; size_t new_sz = sizeof(PAL_RANGE_INFO) * socket_info[socket].range_count; size_t old_sz = new_sz - sizeof(PAL_RANGE_INFO); socket_info[socket].ranges = realloc_size(socket_info[socket].ranges, old_sz, new_sz); if (!socket_info->ranges) { - ret = -ENOMEM; + ret = -1; goto out_socket; } - range_count = socket_info[socket].range_count - 1; - socket_info[socket].ranges[range_count].start = idx; - socket_info[socket].ranges[range_count].end = UINT64_MAX; + int64_t range_idx = socket_info[socket].range_count - 1; + socket_info[socket].ranges[range_idx].start = idx; + socket_info[socket].ranges[range_idx].end = UINT64_MAX; prev_socket = socket; } else { - range_count = socket_info[socket].range_count - 1; - socket_info[socket].ranges[range_count].end = idx; + int64_t range_idx = socket_info[socket].range_count - 1; + socket_info[socket].ranges[range_idx].end = idx; } } - /* Verify against core-siblings */ + /* core-siblings represent all the cores that are part of a socket. We cross-verify the + * socket info against this. */ for (int64_t idx = 0; idx < num_sockets; idx++) { if (!socket_info[idx].range_count || !socket_info[idx].ranges) { - ret = -EINVAL; + ret = -1; goto out_socket; } @@ -395,7 +387,7 @@ static int sanitize_socket_info(PAL_TOPO_INFO topo_info) { uint64_t core_sibling_cnt = topo_info.core_topology[core_in_socket].core_siblings.range_count; if (core_sibling_cnt != socket_info[idx].range_count) { - ret = -EINVAL; + ret = -1; goto out_socket; } @@ -404,14 +396,14 @@ static int sanitize_socket_info(PAL_TOPO_INFO topo_info) { for (uint64_t j = 0; j < core_sibling_cnt; j++) { if (socket_info[idx].ranges[j].start != core_sibling_ranges[j].start || socket_info[idx].ranges[j].end != core_sibling_ranges[j].end) { - ret = -EINVAL; + ret = -1; goto out_socket; } } } out_socket: - for (int64_t i =0 ; i < num_sockets; i++) { + for (int64_t i = 0 ; i < num_sockets; i++) { if (socket_info[i].ranges) free(socket_info[i].ranges); } @@ -425,7 +417,7 @@ static int sanitize_numa_topology_info(PAL_NUMA_TOPO_INFO* numa_topology, int64_ int64_t num_cpumask = BITS_TO_INTS(possible_cores); unsigned int* bitmap = (unsigned int*)calloc(num_cpumask, sizeof(unsigned int)); if (!bitmap) - return -ENOMEM; + return -1; int64_t total_cores_in_numa = 0; for (int64_t idx = 0; idx < num_nodes; idx++) { @@ -439,8 +431,6 @@ static int sanitize_numa_topology_info(PAL_NUMA_TOPO_INFO* numa_topology, int64_ for (int64_t i = 0; i < (int64_t)numa_topology[idx].cpumap.range_count; i++) { uint64_t start = numa_topology[idx].cpumap.ranges[i].start; uint64_t end = numa_topology[idx].cpumap.ranges[i].end; - if (end == UINT64_MAX) - end = start; for (int64_t j = (int64_t)start; j <= (int64_t)end; j++) { int64_t index = j / (sizeof(int) * BITS_IN_BYTE); if (index >= num_cpumask) { @@ -448,7 +438,7 @@ static int sanitize_numa_topology_info(PAL_NUMA_TOPO_INFO* numa_topology, int64_ goto out_numa; } if (bitmap[index] >> (j % (sizeof(int) * BITS_IN_BYTE)) == 1) { - log_error("Invalid numa_topology! Core: %ld found in multiple numa nodes", j); + log_error("Invalid numa_topology: Core %ld found in multiple numa nodes", j); ret = -1; goto out_numa; } @@ -466,7 +456,7 @@ static int sanitize_numa_topology_info(PAL_NUMA_TOPO_INFO* numa_topology, int64_ } if (total_cores_in_numa != num_cores) { - log_error("Invalid numa_topology! More cores in NUMA than online"); + log_error("Invalid numa_topology: more cores in NUMA than online"); ret = -1; goto out_numa; } diff --git a/Pal/src/host/Linux-common/topo_info.c b/Pal/src/host/Linux-common/topo_info.c index e6bac6a305..b17822457a 100644 --- a/Pal/src/host/Linux-common/topo_info.c +++ b/Pal/src/host/Linux-common/topo_info.c @@ -24,7 +24,7 @@ int get_hw_resource(const char* filename, bool count, PAL_RES_RANGE_INFO* res_in res_info->ranges = NULL; } - if(size_mult) + if (size_mult) *size_mult = MULTIPLIER_NONE; int fd = DO_SYSCALL(open, filename, O_RDONLY | O_CLOEXEC, 0); @@ -62,9 +62,9 @@ int get_hw_resource(const char* filename, bool count, PAL_RES_RANGE_INFO* res_in if (*end == 'K') { *size_mult = MULTIPLIER_KB; } else if (*end == 'M') { - *size_mult = MULTIPLIER_KB; + *size_mult = MULTIPLIER_MB; } else if (*end == 'G') { - *size_mult = MULTIPLIER_KB; + *size_mult = MULTIPLIER_GB; } else { *size_mult = MULTIPLIER_NONE; } @@ -73,28 +73,15 @@ int get_hw_resource(const char* filename, bool count, PAL_RES_RANGE_INFO* res_in return retval; } - /* caller wants to count the number of HW resources */ + uint64_t range_start; + uint64_t range_end; + /* caller wants to count the number of HW resources. If `res_info` struct is passed, range + * information is stored in it as we parse the string. */ if (*end == '\0' || *end == ',' || *end == '\n' || *end == ' ') { /* single HW resource index, count as one more */ resource_cnt++; - if (res_info) { - res_info->range_count++; - - /* Realloc as we identify new range when parsing */ - size_t new_sz = sizeof(PAL_RANGE_INFO) * res_info->range_count; - size_t old_sz = new_sz - sizeof(PAL_RANGE_INFO); - void* tmp = malloc(new_sz); - if (!tmp) - return -ENOMEM; - - if (res_info->ranges) { - memcpy(tmp, res_info->ranges, old_sz); - free(res_info->ranges); - } - res_info->ranges = tmp; - res_info->ranges[res_info->range_count - 1].start = firstint; - res_info->ranges[res_info->range_count - 1].end = UINT64_MAX; - } + range_start = firstint; + range_end = firstint; } else if (*end == '-') { /* HW resource range, count how many HW resources are in range */ ptr = end + 1; @@ -108,25 +95,31 @@ int get_hw_resource(const char* filename, bool count, PAL_RES_RANGE_INFO* res_in if (__builtin_add_overflow(resource_cnt, diff, &total_cnt) || total_cnt >= INT_MAX) return -EINVAL; resource_cnt += (int)secondint - (int)firstint + 1; //inclusive (e.g., 0-7, or 8-16) - if (res_info) { - res_info->range_count++; - - /* Realloc as we identify new range when parsing */ - size_t new_sz = sizeof(PAL_RANGE_INFO) * res_info->range_count; - size_t old_sz = new_sz - sizeof(PAL_RANGE_INFO); - void* tmp = malloc(new_sz); - if (!tmp) - return -ENOMEM; - - if (res_info->ranges) { - memcpy(tmp, res_info->ranges, old_sz); - free(res_info->ranges); - } - res_info->ranges = tmp; - res_info->ranges[res_info->range_count - 1].start = firstint; - res_info->ranges[res_info->range_count - 1].end = secondint; - } } + range_start = firstint; + range_end = secondint; + } else { + /* Illegal character found */ + return -EINVAL; + } + + if (res_info) { + res_info->range_count++; + + /* Realloc as we identify new range when parsing */ + size_t new_sz = sizeof(PAL_RANGE_INFO) * res_info->range_count; + size_t old_sz = new_sz - sizeof(PAL_RANGE_INFO); + void* tmp = malloc(new_sz); + if (!tmp) + return -ENOMEM; + + if (res_info->ranges) { + memcpy(tmp, res_info->ranges, old_sz); + free(res_info->ranges); + } + res_info->ranges = tmp; + res_info->ranges[res_info->range_count - 1].start = range_start; + res_info->ranges[res_info->range_count - 1].end = range_end; } ptr = end; } @@ -153,14 +146,6 @@ int read_file_buffer(const char* filename, char* buf, size_t count) { return ret; } -#define READ_FILE_BUFFER(filepath, buf, failure_label) \ - ({ \ - ret = read_file_buffer(filepath, buf, ARRAY_SIZE(buf)-1); \ - if (ret < 0) \ - goto failure_label; \ - buf[ret] = '\0'; \ - }) - /* Returns number of cache levels present on this system by counting "indexX" dir entries under * `/sys/devices/system/cpu/cpuX/cache` on success and negative UNIX error code on failure. */ static int get_num_cache_level(const char* path) { @@ -206,13 +191,14 @@ static int get_cache_topo_info(int num_cache_lvl, int core_idx, PAL_CORE_CACHE_I for (int lvl = 0; lvl < num_cache_lvl; lvl++) { snprintf(filename, sizeof(filename), "/sys/devices/system/cpu/cpu%d/cache/index%d/shared_cpu_list", core_idx, lvl); - ret = get_hw_resource(filename, /*count=*/true, &core_cache[lvl].shared_cpu_map, NULL); + ret = get_hw_resource(filename, /*count=*/true, &core_cache[lvl].shared_cpu_map, + /*size_mult=*/NULL); if (ret < 0) goto out_cache; snprintf(filename, sizeof(filename), "/sys/devices/system/cpu/cpu%d/cache/index%d/level", core_idx, lvl); - int level = get_hw_resource(filename, /*count=*/false, NULL, NULL); + int level = get_hw_resource(filename, /*count=*/false, NULL, /*size_mult=*/NULL); if (level < 0) goto out_cache; core_cache[lvl].level = level; @@ -220,13 +206,18 @@ static int get_cache_topo_info(int num_cache_lvl, int core_idx, PAL_CORE_CACHE_I char type[PAL_SYSFS_BUF_FILESZ] = {'\0'}; snprintf(filename, sizeof(filename), "/sys/devices/system/cpu/cpu%d/cache/index%d/type", core_idx, lvl); - READ_FILE_BUFFER(filename, type, /*failure_label=*/out_cache); + + ret = read_file_buffer(filename, type, ARRAY_SIZE(type)-1); + if (ret < 0) + goto out_cache; + type[ret] = '\0'; + if (!strcmp(type, "Unified\n")) { - core_cache[lvl].type = UNIFIED; + core_cache[lvl].type = CACHE_TYPE_UNIFIED; } else if (!strcmp(type, "Instruction\n")) { - core_cache[lvl].type = INSTRUCTION; + core_cache[lvl].type = CACHE_TYPE_INSTRUCTION; } else if (!strcmp(type, "Data\n")) { - core_cache[lvl].type = DATA; + core_cache[lvl].type = CACHE_TYPE_DATA; } else { ret = -EINVAL; goto out_cache; @@ -243,21 +234,23 @@ static int get_cache_topo_info(int num_cache_lvl, int core_idx, PAL_CORE_CACHE_I snprintf(filename, sizeof(filename), "/sys/devices/system/cpu/cpu%d/cache/index%d/coherency_line_size", core_idx, lvl); - int coherency_line_size = get_hw_resource(filename, /*count=*/false, NULL, NULL); + int coherency_line_size = get_hw_resource(filename, /*count=*/false, NULL, + /*size_mult=*/NULL); if (coherency_line_size < 0) goto out_cache; core_cache[lvl].coherency_line_size = coherency_line_size; snprintf(filename, sizeof(filename), "/sys/devices/system/cpu/cpu%d/cache/index%d/number_of_sets", core_idx, lvl); - int num_sets = get_hw_resource(filename, /*count=*/false, NULL, NULL); + int num_sets = get_hw_resource(filename, /*count=*/false, NULL, /*size_mult=*/NULL); if (num_sets < 0) goto out_cache; core_cache[lvl].number_of_sets = num_sets; snprintf(filename, sizeof(filename), "/sys/devices/system/cpu/cpu%d/cache/index%d/physical_line_partition", core_idx, lvl); - int physical_line_partition = get_hw_resource(filename, /*count=*/false, NULL, NULL); + int physical_line_partition = get_hw_resource(filename, /*count=*/false, NULL, + /*size_mult=*/NULL); if (physical_line_partition < 0) goto out_cache; core_cache[lvl].physical_line_partition = physical_line_partition; @@ -274,20 +267,26 @@ static int get_cache_topo_info(int num_cache_lvl, int core_idx, PAL_CORE_CACHE_I /* Get core topology-related info */ static int get_core_topo_info(PAL_TOPO_INFO* topo_info) { int ret = get_hw_resource("/sys/devices/system/cpu/online", /*count=*/true, - &topo_info->online_logical_cores, NULL); + &topo_info->online_logical_cores, /*size_mult=*/NULL); if (ret < 0) return ret; ret = get_hw_resource("/sys/devices/system/cpu/possible", /*count=*/true, - &topo_info->possible_logical_cores, NULL); + &topo_info->possible_logical_cores, /*size_mult=*/NULL); if (ret < 0) return ret; + if (topo_info->online_logical_cores.resource_count > INT32_MAX) + return -EINVAL; int online_logical_cores = topo_info->online_logical_cores.resource_count; + + if (topo_info->possible_logical_cores.resource_count > INT32_MAX) + return -EINVAL; int possible_logical_cores = topo_info->possible_logical_cores.resource_count; + /* TODO: correctly support offline cores */ - if (possible_logical_cores > 0 && possible_logical_cores > online_logical_cores) { - log_warning("some CPUs seem to be offline; Graphene doesn't take this into account which " + if (possible_logical_cores > online_logical_cores) { + log_warning("Some CPUs seem to be offline; Gramine doesn't take this into account which " "may lead to subpar performance"); } @@ -301,54 +300,53 @@ static int get_core_topo_info(PAL_TOPO_INFO* topo_info) { if (!core_topology) return -ENOMEM; - int num_sockets = 0; int current_max_socket = -1; char filename[128]; for (int idx = 0; idx < online_logical_cores; idx++) { /* cpu0 is always online and thus the "online" file is not present. */ if (idx != 0) { snprintf(filename, sizeof(filename), "/sys/devices/system/cpu/cpu%d/online", idx); - ret = get_hw_resource(filename, /*count=*/false, NULL, NULL); + ret = get_hw_resource(filename, /*count=*/false, NULL, /*size_mult=*/NULL); if (ret < 0) goto out_topology; core_topology[idx].is_logical_core_online = ret; } snprintf(filename, sizeof(filename), "/sys/devices/system/cpu/cpu%d/topology/core_id", idx); - ret = get_hw_resource(filename, /*count=*/false, NULL, NULL); + ret = get_hw_resource(filename, /*count=*/false, NULL, /*size_mult=*/NULL); if (ret < 0) goto out_topology; - core_topology[idx].core_id = ret; + core_topology[idx].core_id = ret; snprintf(filename, sizeof(filename), "/sys/devices/system/cpu/cpu%d/topology/core_siblings_list", idx); - ret = get_hw_resource(filename, /*count=*/true, &core_topology[idx].core_siblings, NULL); + ret = get_hw_resource(filename, /*count=*/true, &core_topology[idx].core_siblings, + /*size_mult=*/NULL); if (ret < 0) goto out_topology; snprintf(filename, sizeof(filename), "/sys/devices/system/cpu/cpu%d/topology/thread_siblings_list", idx); - ret = get_hw_resource(filename, /*count=*/true, &core_topology[idx].thread_siblings, NULL); + ret = get_hw_resource(filename, /*count=*/true, &core_topology[idx].thread_siblings, + /*size_mult=*/NULL); if (ret < 0) goto out_topology; snprintf(filename, sizeof(filename), "/sys/devices/system/cpu/cpu%d/topology/physical_package_id", idx); - ret = get_hw_resource(filename, /*count=*/false, NULL, NULL); + ret = get_hw_resource(filename, /*count=*/false, NULL, /*size_mult=*/NULL); if (ret < 0) goto out_topology; - core_topology[idx].cpu_socket = ret; - if (ret > current_max_socket) { + core_topology[idx].cpu_socket = ret; + if (ret > current_max_socket) current_max_socket = ret; - num_sockets++; - } ret = get_cache_topo_info(num_cache_lvl, idx, &core_topology[idx].cache); if (ret < 0) goto out_topology; } topo_info->core_topology = core_topology; - topo_info->num_sockets = num_sockets; + topo_info->num_sockets = current_max_socket + 1; topo_info->physical_cores_per_socket = core_topology[0].core_siblings.resource_count / core_topology[0].thread_siblings.resource_count; return 0; @@ -361,7 +359,7 @@ static int get_core_topo_info(PAL_TOPO_INFO* topo_info) { /* Get NUMA topology-related info */ static int get_numa_topo_info(PAL_TOPO_INFO* topo_info) { int ret = get_hw_resource("/sys/devices/system/node/online", /*count=*/true, - &topo_info->nodes, NULL); + &topo_info->nodes, /*size_mult=*/NULL); if (ret < 0) return ret; @@ -374,12 +372,14 @@ static int get_numa_topo_info(PAL_TOPO_INFO* topo_info) { char filename[128]; for (int idx = 0; idx < num_nodes; idx++) { snprintf(filename, sizeof(filename), "/sys/devices/system/node/node%d/cpulist", idx); - ret = get_hw_resource(filename, /*count=*/true, &numa_topology[idx].cpumap, NULL); + ret = get_hw_resource(filename, /*count=*/true, &numa_topology[idx].cpumap, + /*size_mult=*/NULL); if (ret < 0) goto out_topology; snprintf(filename, sizeof(filename), "/sys/devices/system/node/node%d/distance", idx); - ret = get_hw_resource(filename, /*count=*/true, &numa_topology[idx].distance, NULL); + ret = get_hw_resource(filename, /*count=*/true, &numa_topology[idx].distance, + /*size_mult=*/NULL); if (ret < 0) goto out_topology;