From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id RM/+IyEWxV+VKwAAWB0awg (envelope-from ) for ; Mon, 30 Nov 2020 10:56:17 -0500 Received: by simark.ca (Postfix, from userid 112) id 8A54A1F0AB; Mon, 30 Nov 2020 10:56:17 -0500 (EST) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-1.0 required=5.0 tests=MAILING_LIST_MULTI autolearn=ham autolearn_force=no version=3.4.2 Received: from sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id 695EE1E552 for ; Mon, 30 Nov 2020 10:56:16 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 95BFD3851C26; Mon, 30 Nov 2020 15:56:15 +0000 (GMT) Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id A3E24385802D for ; Mon, 30 Nov 2020 15:56:12 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org A3E24385802D Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark@simark.ca Received: from [10.0.0.11] (173-246-6-90.qc.cable.ebox.net [173.246.6.90]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 479471E552; Mon, 30 Nov 2020 10:56:12 -0500 (EST) Subject: Re: [RFA 2/2] gmp-utils: protect gdb_mpz exports against out-of-range values To: Joel Brobecker , gdb-patches@sourceware.org References: <20201123042711.GA967337@adacore.com> <1606664757-144138-1-git-send-email-brobecker@adacore.com> <1606664757-144138-3-git-send-email-brobecker@adacore.com> From: Simon Marchi Message-ID: Date: Mon, 30 Nov 2020 10:56:11 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.12.0 MIME-Version: 1.0 In-Reply-To: <1606664757-144138-3-git-send-email-brobecker@adacore.com> Content-Type: text/plain; charset=utf-8 Content-Language: fr Content-Transfer-Encoding: 7bit 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: , Errors-To: gdb-patches-bounces@sourceware.org Sender: "Gdb-patches" On 2020-11-29 10:45 a.m., Joel Brobecker wrote: > 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 | 44 ++++++++++++--------- > gdb/unittests/gmp-utils-selftests.c | 71 +++++++++++++++++++++++++++++++-- > 3 files changed, 167 insertions(+), 27 deletions(-) > > diff --git a/gdb/gmp-utils.c b/gdb/gmp-utils.c > index e3a3333..f94bdc5 100644 > --- a/gdb/gmp-utils.c > +++ b/gdb/gmp-utils.c > @@ -68,9 +68,62 @@ 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 */, 0 /* nails */, > + unsigned_p); > +} > + > +/* See gmp-utils.h. */ > + > +void > +gdb_mpz::safe_export (gdb::array_view buf, > + int endian, size_t nails, bool unsigned_p) const Just wondering if you'll ever need to pass a non-zero value for nails. If not, I think you could simplify the code by just removing it. > +{ > + 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 - nails; > + 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 %zubits %s integer" > + " (must be between %s and %s)"), You might be missing a space before "bits", or maybe it's on purpose. > + 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 +134,27 @@ 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 I'd prefer if we didn't use heap allocation unnecessarily. If we get things right, that isn't necessary. And if we can still assert after the call that we did indeed get it right, then we'll catch any case where we didn't. > + (mpz_export (NULL, &word_countp, -1 /* order */, buf.size () /* size */, > + endian, 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. */ > + bytes as it needs. */ > 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); > + memcpy (buf.data (), exported.get (), buf.size ()); The memset just before the memcpy of the same size is useless, as all the bytes will get overwritten by the memcpy. > @@ -258,26 +278,14 @@ 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; > + T result; > > - 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; > + this->safe_export (gdb::make_array_view ((gdb_byte *) &result, > + sizeof (result)), I'd suggest using {(gdb_byte *) &result, sizeof (result)} to make the array view, as suggested by Pedro earlier. Simon