From 4c5479d21e4d8d0bd1ca6abc04f6800a92fd34e5 Mon Sep 17 00:00:00 2001 From: Christian Brauner Date: Sat, 27 Mar 2021 21:14:02 +0100 Subject: [PATCH] string_utils: handle overflow correct in parse_byte_size_string() This takes the overflow handling code from the kernel. Link: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=32549 Signed-off-by: Christian Brauner --- src/lxc/compiler.h | 196 ++++++++++++++++++++++++++++++++++++- src/lxc/confile.c | 8 +- src/lxc/string_utils.c | 11 +-- src/lxc/string_utils.h | 2 +- src/tests/lxc-test-utils.c | 2 +- 5 files changed, 203 insertions(+), 16 deletions(-) diff --git a/src/lxc/compiler.h b/src/lxc/compiler.h index 81cab5c43..5d45955d0 100644 --- a/src/lxc/compiler.h +++ b/src/lxc/compiler.h @@ -7,6 +7,9 @@ #define _GNU_SOURCE 1 #endif +#include +#include + #include "config.h" #ifndef thread_local @@ -25,6 +28,196 @@ #define __fallthrough /* fall through */ #endif +#if defined(__GNUC__) && !defined(__clang__) + #if GCC_VERSION >= 50100 + #define COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW 1 + #endif +#endif + +#define likely(x) __builtin_expect(!!(x), 1) +#define unlikely(x) __builtin_expect(!!(x), 0) +#define __must_check __attribute__((__warn_unused_result__)) + +static inline bool __must_check __must_check_overflow(bool overflow) +{ + return unlikely(overflow); +} + +#define is_signed_type(type) (((type)(-1)) < (type)1) +#define __type_half_max(type) ((type)1 << (8*sizeof(type) - 1 - is_signed_type(type))) +#define type_max(T) ((T)((__type_half_max(T) - 1) + __type_half_max(T))) +#define type_min(T) ((T)((T)-type_max(T)-(T)1)) + +/* + * Avoids triggering -Wtype-limits compilation warning, + * while using unsigned data types to check a < 0. + */ +#define is_non_negative(a) ((a) > 0 || (a) == 0) +#define is_negative(a) (!(is_non_negative(a))) + +#ifdef COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW +/* + * For simplicity and code hygiene, the fallback code below insists on + * a, b and *d having the same type (similar to the min() and max() + * macros), whereas gcc's type-generic overflow checkers accept + * different types. Hence we don't just make check_add_overflow an + * alias for __builtin_add_overflow, but add type checks similar to + * below. + */ +#define check_add_overflow(a, b, d) __must_check_overflow(({ \ + typeof(a) __a = (a); \ + typeof(b) __b = (b); \ + typeof(d) __d = (d); \ + (void) (&__a == &__b); \ + (void) (&__a == __d); \ + __builtin_add_overflow(__a, __b, __d); \ +})) + +#define check_sub_overflow(a, b, d) __must_check_overflow(({ \ + typeof(a) __a = (a); \ + typeof(b) __b = (b); \ + typeof(d) __d = (d); \ + (void) (&__a == &__b); \ + (void) (&__a == __d); \ + __builtin_sub_overflow(__a, __b, __d); \ +})) + +#define check_mul_overflow(a, b, d) __must_check_overflow(({ \ + typeof(a) __a = (a); \ + typeof(b) __b = (b); \ + typeof(d) __d = (d); \ + (void) (&__a == &__b); \ + (void) (&__a == __d); \ + __builtin_mul_overflow(__a, __b, __d); \ +})) +#else /* !COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW */ + +/* Checking for unsigned overflow is relatively easy without causing UB. */ +#define __unsigned_add_overflow(a, b, d) ({ \ + typeof(a) __a = (a); \ + typeof(b) __b = (b); \ + typeof(d) __d = (d); \ + (void) (&__a == &__b); \ + (void) (&__a == __d); \ + *__d = __a + __b; \ + *__d < __a; \ +}) +#define __unsigned_sub_overflow(a, b, d) ({ \ + typeof(a) __a = (a); \ + typeof(b) __b = (b); \ + typeof(d) __d = (d); \ + (void) (&__a == &__b); \ + (void) (&__a == __d); \ + *__d = __a - __b; \ + __a < __b; \ +}) + +/* + * If one of a or b is a compile-time constant, this avoids a division. + */ +#define __unsigned_mul_overflow(a, b, d) ({ \ + typeof(a) __a = (a); \ + typeof(b) __b = (b); \ + typeof(d) __d = (d); \ + (void) (&__a == &__b); \ + (void) (&__a == __d); \ + *__d = __a * __b; \ + __builtin_constant_p(__b) ? \ + __b > 0 && __a > type_max(typeof(__a)) / __b : \ + __a > 0 && __b > type_max(typeof(__b)) / __a; \ +}) + +/* + * For signed types, detecting overflow is much harder, especially if + * we want to avoid UB. But the interface of these macros is such that + * we must provide a result in *d, and in fact we must produce the + * result promised by gcc's builtins, which is simply the possibly + * wrapped-around value. Fortunately, we can just formally do the + * operations in the widest relevant unsigned type (u64) and then + * truncate the result - gcc is smart enough to generate the same code + * with and without the (u64) casts. + */ + +/* + * Adding two signed integers can overflow only if they have the same + * sign, and overflow has happened iff the result has the opposite + * sign. + */ +#define __signed_add_overflow(a, b, d) ({ \ + typeof(a) __a = (a); \ + typeof(b) __b = (b); \ + typeof(d) __d = (d); \ + (void) (&__a == &__b); \ + (void) (&__a == __d); \ + *__d = (__u64)__a + (__u64)__b; \ + (((~(__a ^ __b)) & (*__d ^ __a)) \ + & type_min(typeof(__a))) != 0; \ +}) + +/* + * Subtraction is similar, except that overflow can now happen only + * when the signs are opposite. In this case, overflow has happened if + * the result has the opposite sign of a. + */ +#define __signed_sub_overflow(a, b, d) ({ \ + typeof(a) __a = (a); \ + typeof(b) __b = (b); \ + typeof(d) __d = (d); \ + (void) (&__a == &__b); \ + (void) (&__a == __d); \ + *__d = (__u64)__a - (__u64)__b; \ + ((((__a ^ __b)) & (*__d ^ __a)) \ + & type_min(typeof(__a))) != 0; \ +}) + +/* + * Signed multiplication is rather hard. gcc always follows C99, so + * division is truncated towards 0. This means that we can write the + * overflow check like this: + * + * (a > 0 && (b > MAX/a || b < MIN/a)) || + * (a < -1 && (b > MIN/a || b < MAX/a) || + * (a == -1 && b == MIN) + * + * The redundant casts of -1 are to silence an annoying -Wtype-limits + * (included in -Wextra) warning: When the type is u8 or u16, the + * __b_c_e in check_mul_overflow obviously selects + * __unsigned_mul_overflow, but unfortunately gcc still parses this + * code and warns about the limited range of __b. + */ + +#define __signed_mul_overflow(a, b, d) ({ \ + typeof(a) __a = (a); \ + typeof(b) __b = (b); \ + typeof(d) __d = (d); \ + typeof(a) __tmax = type_max(typeof(a)); \ + typeof(a) __tmin = type_min(typeof(a)); \ + (void) (&__a == &__b); \ + (void) (&__a == __d); \ + *__d = (__u64)__a * (__u64)__b; \ + (__b > 0 && (__a > __tmax/__b || __a < __tmin/__b)) || \ + (__b < (typeof(__b))-1 && (__a > __tmin/__b || __a < __tmax/__b)) || \ + (__b == (typeof(__b))-1 && __a == __tmin); \ +}) + + +#define check_add_overflow(a, b, d) __must_check_overflow( \ + __builtin_choose_expr(is_signed_type(typeof(a)), \ + __signed_add_overflow(a, b, d), \ + __unsigned_add_overflow(a, b, d))) + +#define check_sub_overflow(a, b, d) __must_check_overflow( \ + __builtin_choose_expr(is_signed_type(typeof(a)), \ + __signed_sub_overflow(a, b, d), \ + __unsigned_sub_overflow(a, b, d))) + +#define check_mul_overflow(a, b, d) __must_check_overflow( \ + __builtin_choose_expr(is_signed_type(typeof(a)), \ + __signed_mul_overflow(a, b, d), \ + __unsigned_mul_overflow(a, b, d))) + +#endif /* COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW */ + #ifndef __noreturn # if __STDC_VERSION__ >= 201112L # if !IS_BIONIC @@ -89,7 +282,4 @@ #define __public __attribute__((visibility("default"))) #endif -#define likely(x) __builtin_expect(!!(x), 1) -#define unlikely(x) __builtin_expect(!!(x), 0) - #endif /* __LXC_COMPILER_H */ diff --git a/src/lxc/confile.c b/src/lxc/confile.c index 1decb0bc5..59fc5704d 100644 --- a/src/lxc/confile.c +++ b/src/lxc/confile.c @@ -2421,7 +2421,7 @@ static int set_config_console_buffer_size(const char *key, const char *value, struct lxc_conf *lxc_conf, void *data) { int ret; - int64_t size; + long long int size; uint64_t buffer_size, pgsz; if (lxc_config_value_empty(value)) { @@ -2445,7 +2445,7 @@ static int set_config_console_buffer_size(const char *key, const char *value, /* must be at least a page size */ pgsz = lxc_getpagesize(); if ((uint64_t)size < pgsz) { - NOTICE("Requested ringbuffer size for the console is %" PRId64 " but must be at least %" PRId64 " bytes. Setting ringbuffer size to %" PRId64 " bytes", + NOTICE("Requested ringbuffer size for the console is %lld but must be at least %" PRId64 " bytes. Setting ringbuffer size to %" PRId64 " bytes", size, pgsz, pgsz); size = pgsz; } @@ -2466,7 +2466,7 @@ static int set_config_console_size(const char *key, const char *value, struct lxc_conf *lxc_conf, void *data) { int ret; - int64_t size; + long long int size; uint64_t log_size, pgsz; if (lxc_config_value_empty(value)) { @@ -2490,7 +2490,7 @@ static int set_config_console_size(const char *key, const char *value, /* must be at least a page size */ pgsz = lxc_getpagesize(); if ((uint64_t)size < pgsz) { - NOTICE("Requested ringbuffer size for the console is %" PRId64 " but must be at least %" PRId64 " bytes. Setting ringbuffer size to %" PRId64 " bytes", + NOTICE("Requested ringbuffer size for the console is %lld but must be at least %" PRId64 " bytes. Setting ringbuffer size to %" PRId64 " bytes", size, pgsz, pgsz); size = pgsz; } diff --git a/src/lxc/string_utils.c b/src/lxc/string_utils.c index 1d408b977..933fb434d 100644 --- a/src/lxc/string_utils.c +++ b/src/lxc/string_utils.c @@ -897,13 +897,12 @@ void *must_realloc(void *orig, size_t sz) return ret; } -int parse_byte_size_string(const char *s, int64_t *converted) +int parse_byte_size_string(const char *s, long long int *converted) { int ret, suffix_len; - long long int conv; - int64_t mltpl, overflow; + long long int conv, mltpl; char *end; - char dup[INTTYPE_TO_STRLEN(int64_t)]; + char dup[INTTYPE_TO_STRLEN(long long int)]; char suffix[3] = {0}; if (is_empty_string(s)) @@ -960,11 +959,9 @@ int parse_byte_size_string(const char *s, int64_t *converted) else return ret_errno(EINVAL); - overflow = conv * mltpl; - if (conv != 0 && (overflow / conv) != mltpl) + if (check_mul_overflow(conv, mltpl, converted)) return ret_errno(ERANGE); - *converted = overflow; return 0; } diff --git a/src/lxc/string_utils.h b/src/lxc/string_utils.h index 688587de1..76c765d3c 100644 --- a/src/lxc/string_utils.h +++ b/src/lxc/string_utils.h @@ -81,7 +81,7 @@ __hidden extern int lxc_safe_uint64(const char *numstr, uint64_t *converted, int __hidden extern int lxc_safe_int64_residual(const char *numstr, int64_t *converted, int base, char *residual, size_t residual_len); /* Handles B, kb, MB, GB. Detects overflows and reports -ERANGE. */ -__hidden extern int parse_byte_size_string(const char *s, int64_t *converted); +__hidden extern int parse_byte_size_string(const char *s, long long int *converted); /* * Concatenate all passed-in strings into one path. Do not fail. If any piece diff --git a/src/tests/lxc-test-utils.c b/src/tests/lxc-test-utils.c index 841198d9e..9b3e8cd19 100644 --- a/src/tests/lxc-test-utils.c +++ b/src/tests/lxc-test-utils.c @@ -403,7 +403,7 @@ void test_lxc_string_in_array(void) void test_parse_byte_size_string(void) { int ret; - int64_t n; + long long int n; ret = parse_byte_size_string("0", &n); if (ret < 0) { -- 2.47.2