From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from rock.gnat.com (rock.gnat.com [205.232.38.15]) by sourceware.org (Postfix) with ESMTP id F264A385782B for ; Sat, 5 Dec 2020 08:10:56 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org F264A385782B Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=adacore.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=brobecke@adacore.com Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id D1489561AF; Sat, 5 Dec 2020 03:10:56 -0500 (EST) X-Virus-Scanned: Debian amavisd-new at gnat.com Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id yXu9OjZK1sAd; Sat, 5 Dec 2020 03:10:56 -0500 (EST) Received: from tron.gnat.com (tron.gnat.com [205.232.38.10]) by rock.gnat.com (Postfix) with ESMTP id C1172561AE; Sat, 5 Dec 2020 03:10:56 -0500 (EST) Received: by tron.gnat.com (Postfix, from userid 4233) id B6BE0112; Sat, 5 Dec 2020 03:10:56 -0500 (EST) From: Joel Brobecker To: gdb-patches@sourceware.org Cc: Simon Marchi , Joel Brobecker Subject: [RFAv2 2/2] gmp-utils: protect gdb_mpz exports against out-of-range values Date: Sat, 5 Dec 2020 03:10:55 -0500 Message-Id: <1607155855-98060-1-git-send-email-brobecker@adacore.com> X-Mailer: git-send-email 2.1.4 In-Reply-To: <1606664757-144138-3-git-send-email-brobecker@adacore.com> References: <1606664757-144138-3-git-send-email-brobecker@adacore.com> X-Spam-Status: No, score=-10.2 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 05 Dec 2020 08:10:59 -0000 Hi Simon, Here is v2 of this patch, taking into account your comments and suggestions. More precisely: - Parameter "nails" in gdb_mpz::safe_export has been removed; - I changed the error message to use "[nnn]-bits" instead of "[nnn]bits"; - I removed a useless memset in gdb_mpz::safe_export; - I used the {buf, size} idiom to construct a a gdb::array view instead of using gdb::make_array_view. The series was re-tested on x86_64-linux. OK to push? --------------------------------------------------------------------------- The gdb_mpz class currently provides a couple of methods which essentially export an mpz_t value into either a buffer, or an integral type. The export is based on using the mpz_export function which we discovered can be a bit treacherous if used without caution. In particular, the initial motivation for this patch was to catch situations where the mpz_t value was so large that it would not fit in the destination area. mpz_export does not know the size of the buffer, and therefore can happily write past the end of our buffer. While designing a solution to the above problem, I also discovered that we also needed to be careful when exporting signed numbers. In particular, numbers which are larger than the maximum value for a given signed type size, but no so large as to fit in the *unsigned* version with the same size, would end up being exported incorrectly. This is related to the fact that mpz_export ignores the sign of the value being exportd, and assumes an unsigned export. Thus, for such large values, the appears as if mpz_export is able to fit our value into our buffer, but in fact, it does not. Also, I noticed that gdb_mpz::write wasn't taking its unsigned_p parameter, which was a hole. For all these reasons, a new low-level private method called "safe_export" has been added to class gdb_mpz, whose goal is to perform all necessary checks and manipulations for a safe and correct export. As a bonus, this method allows us to factorize the handling of negative value exports. The gdb_mpz::as_integer and gdb_mpz::write methods are then simplified to take advantage of this new safe_export method. gdb/ChangeLog: * gmp-utils.h (gdb_mpz::safe_export): New private method. (gdb_mpz::as_integer): Reimplement using gdb_mpz::safe_export. * gmp-utils.c (gdb_mpz::write): Rewrite using gdb_mpz::safe_export. (gdb_mpz::safe_export): New method. * unittests/gmp-utils-selftests .c (gdb_mpz_as_integer): Update function description. (check_as_integer_raises_out_of_range_error): New function. (gdb_mpz_as_integer_out_of_range): New function. (_initialize_gmp_utils_selftests): Register gdb_mpz_as_integer_out_of_range as a selftest. --- gdb/gmp-utils.c | 79 +++++++++++++++++++++++++++++++++---- gdb/gmp-utils.h | 40 ++++++++++--------- gdb/unittests/gmp-utils-selftests.c | 71 +++++++++++++++++++++++++++++++-- 3 files changed, 162 insertions(+), 28 deletions(-) diff --git a/gdb/gmp-utils.c b/gdb/gmp-utils.c index e3a3333..6b5c820 100644 --- a/gdb/gmp-utils.c +++ b/gdb/gmp-utils.c @@ -68,9 +68,61 @@ void gdb_mpz::write (gdb::array_view buf, enum bfd_endian byte_order, bool unsigned_p) const { + this->safe_export + (buf, byte_order == BFD_ENDIAN_BIG ? 1 : -1 /* endian */, unsigned_p); +} + +/* See gmp-utils.h. */ + +void +gdb_mpz::safe_export (gdb::array_view buf, + int endian, bool unsigned_p) const +{ + gdb_assert (buf.size () > 0); + + if (mpz_sgn (val) == 0) + { + /* Our value is zero, so no need to call mpz_export to do the work, + especially since mpz_export's documentation explicitly says + that the function is a noop in this case. Just write zero to + BUF ourselves. */ + memset (buf.data (), 0, buf.size ()); + return; + } + + /* Determine the maximum range of values that our buffer can hold, + and verify that VAL is within that range. */ + + gdb_mpz lo, hi; + const size_t max_usable_bits = buf.size () * HOST_CHAR_BIT; + if (unsigned_p) + { + lo = 0; + + mpz_ui_pow_ui (hi.val, 2, max_usable_bits); + mpz_sub_ui (hi.val, hi.val, 1); + } + else + { + mpz_ui_pow_ui (lo.val, 2, max_usable_bits - 1); + mpz_neg (lo.val, lo.val); + + mpz_ui_pow_ui (hi.val, 2, max_usable_bits - 1); + mpz_sub_ui (hi.val, hi.val, 1); + } + + if (mpz_cmp (val, lo.val) < 0 || mpz_cmp (val, hi.val) > 0) + error (_("Cannot export value %s as %zu-bits %s integer" + " (must be between %s and %s)"), + this->str ().c_str (), + max_usable_bits, + unsigned_p ? _("unsigned") : _("signed"), + lo.str ().c_str (), + hi.str ().c_str ()); + gdb_mpz exported_val (val); - if (mpz_cmp_ui (val, 0) < 0) + if (mpz_cmp_ui (exported_val.val, 0) < 0) { /* mpz_export does not handle signed values, so create a positive value whose bit representation as an unsigned of the same length @@ -81,13 +133,26 @@ gdb_mpz::write (gdb::array_view buf, enum bfd_endian byte_order, mpz_add (exported_val.val, exported_val.val, neg_offset.val); } + /* Do the export into a buffer allocated by GMP itself; that way, + we can detect cases where BUF is not large enough to export + our value, and thus avoid a buffer overlow. Normally, this should + never happen, since we verified earlier that the buffer is large + enough to accomodate our value, but doing this allows us to be + extra safe with the export. + + After verification that the export behaved as expected, we will + copy the data over to BUF. */ + + size_t word_countp; + gdb::unique_xmalloc_ptr exported + (mpz_export (NULL, &word_countp, -1 /* order */, buf.size () /* size */, + endian, 0 /* nails */, exported_val.val)); + + gdb_assert (word_countp == 1); + /* Start by clearing the buffer, as mpz_export only writes as many - bytes as it needs (including none, if the value to export is zero. */ - memset (buf.data (), 0, buf.size ()); - mpz_export (buf.data (), NULL /* count */, -1 /* order */, - buf.size () /* size */, - byte_order == BFD_ENDIAN_BIG ? 1 : -1 /* endian */, - 0 /* nails */, exported_val.val); + bytes as it needs. */ + memcpy (buf.data (), exported.get (), buf.size ()); } /* See gmp-utils.h. */ diff --git a/gdb/gmp-utils.h b/gdb/gmp-utils.h index 12e4f8e..dae62cf 100644 --- a/gdb/gmp-utils.h +++ b/gdb/gmp-utils.h @@ -121,6 +121,24 @@ struct gdb_mpz /* Helper template for constructor and operator=. */ template void set (T src); + + /* Low-level function to export VAL into BUF as a number whose byte size + is the size of BUF. + + If UNSIGNED_P is true, then export VAL into BUF as an unsigned value. + Otherwise, export it as a signed value. + + The API is inspired from GMP's mpz_export, hence the naming and types + of the following parameter: + - ENDIAN should be: + . 1 for most significant byte first; or + . -1 for least significant byte first; or + . 0 for native endianness. + + An error is raised if BUF is not large enough to contain the value + being exported. */ + void safe_export (gdb::array_view buf, + int endian, bool unsigned_p) const; }; /* A class to make it easier to use GMP's mpq_t values within GDB. */ @@ -258,26 +276,12 @@ template T gdb_mpz::as_integer () const { - /* Initialize RESULT, because mpz_export only write the minimum - number of bytes, including none if our value is zero! */ - T result = 0; - - gdb_mpz exported_val (val); - if (std::is_signed::value && mpz_cmp_ui (val, 0) < 0) - { - /* We want to use mpz_export to set the return value, but - this function does not handle the sign. So give exported_val - a value which is at the same time positive, and has the same - bit representation as our negative value. */ - gdb_mpz neg_offset; + T result; - mpz_ui_pow_ui (neg_offset.val, 2, sizeof (T) * HOST_CHAR_BIT); - mpz_add (exported_val.val, exported_val.val, neg_offset.val); - } + this->safe_export ({(gdb_byte *) &result, sizeof (result)}, + 0 /* endian (0 = native) */, + !std::is_signed::value /* unsigned_p */); - mpz_export (&result, NULL /* count */, -1 /* order */, - sizeof (T) /* size */, 0 /* endian (0 = native) */, - 0 /* nails */, exported_val.val); return result; } diff --git a/gdb/unittests/gmp-utils-selftests.c b/gdb/unittests/gmp-utils-selftests.c index 1365905..30c1902 100644 --- a/gdb/unittests/gmp-utils-selftests.c +++ b/gdb/unittests/gmp-utils-selftests.c @@ -26,9 +26,10 @@ namespace selftests { /* Perform a series of general tests of gdb_mpz's as_integer method. - This function tries to be reasonably exhaustive, by testing the edges, - as well as a resonable set of values including negative ones, zero, - and positive values. */ + This function limits itself to values which are in range (out-of-range + values will be tested separately). In doing so, it tries to be reasonably + exhaustive, by testing the edges, as well as a resonable set of values + including negative ones, zero, and positive values. */ static void gdb_mpz_as_integer () @@ -80,6 +81,68 @@ gdb_mpz_as_integer () SELF_CHECK (v.as_integer () == ul_expected); } +/* A helper function which calls the given gdb_mpz object's as_integer + method with the given type T, and verifies that this triggers + an error due to VAL's value being out of range for type T. */ + +template>> +static void +check_as_integer_raises_out_of_range_error (const gdb_mpz &val) +{ + try + { + val.as_integer (); + } + catch (const gdb_exception_error &ex) + { + SELF_CHECK (ex.reason == RETURN_ERROR); + SELF_CHECK (ex.error == GENERIC_ERROR); + SELF_CHECK (strstr (ex.what (), "Cannot export value") != nullptr); + return; + } + /* The expected exception did not get raised. */ + SELF_CHECK (false); +} + +/* Perform out-of-range tests of gdb_mpz's as_integer method. + + The goal of this function is to verify that gdb_mpz::as_integer + handles out-of-range values correctly. */ + +static void +gdb_mpz_as_integer_out_of_range () +{ + gdb_mpz v; + + /* Try LONGEST_MIN minus 1. */ + mpz_ui_pow_ui (v.val, 2, sizeof (LONGEST) * 8 - 1); + mpz_neg (v.val, v.val); + mpz_sub_ui (v.val, v.val, 1); + + check_as_integer_raises_out_of_range_error (v); + check_as_integer_raises_out_of_range_error (v); + + /* Try negative one (-1). */ + v = -1; + + check_as_integer_raises_out_of_range_error (v); + SELF_CHECK (v.as_integer () == (LONGEST) -1); + + /* Try LONGEST_MAX plus 1. */ + v = LONGEST_MAX; + mpz_add_ui (v.val, v.val, 1); + + SELF_CHECK (v.as_integer () == (ULONGEST) LONGEST_MAX + 1); + check_as_integer_raises_out_of_range_error (v); + + /* Try ULONGEST_MAX plus 1. */ + v = ULONGEST_MAX; + mpz_add_ui (v.val, v.val, 1); + + check_as_integer_raises_out_of_range_error (v); + check_as_integer_raises_out_of_range_error (v); +} + /* A helper function to store the given integer value into a buffer, before reading it back into a gdb_mpz. Sets ACTUAL to the value read back, while at the same time setting EXPECTED as the value @@ -445,6 +508,8 @@ _initialize_gmp_utils_selftests () { selftests::register_test ("gdb_mpz_as_integer", selftests::gdb_mpz_as_integer); + selftests::register_test ("gdb_mpz_as_integer_out_of_range", + selftests::gdb_mpz_as_integer_out_of_range); selftests::register_test ("gdb_mpz_read_all_from_small", selftests::gdb_mpz_read_all_from_small); selftests::register_test ("gdb_mpz_read_min_max", -- 2.1.4