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 6C9A8385480A for ; Tue, 1 Dec 2020 03:37:32 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 6C9A8385480A Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=adacore.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=brobecker@adacore.com Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 048D0116E83; Mon, 30 Nov 2020 22:37:32 -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 Q1gyzkhq1gw6; Mon, 30 Nov 2020 22:37:31 -0500 (EST) Received: from float.home (localhost.localdomain [127.0.0.1]) (using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by rock.gnat.com (Postfix) with ESMTPS id A34EA116E71; Mon, 30 Nov 2020 22:37:31 -0500 (EST) Received: by float.home (Postfix, from userid 1000) id F1AECA6B54; Tue, 1 Dec 2020 07:37:25 +0400 (+04) Date: Tue, 1 Dec 2020 07:37:25 +0400 From: Joel Brobecker To: Simon Marchi Cc: gdb-patches@sourceware.org Subject: Re: [RFA 2/2] gmp-utils: protect gdb_mpz exports against out-of-range values Message-ID: <20201201033725.GA3501@adacore.com> References: <20201123042711.GA967337@adacore.com> <1606664757-144138-1-git-send-email-brobecker@adacore.com> <1606664757-144138-3-git-send-email-brobecker@adacore.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Spam-Status: No, score=-3.8 required=5.0 tests=BAYES_00, 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: Tue, 01 Dec 2020 03:37:34 -0000 Hi Simon, Thanks for the review! My preliminary answers below.. > > +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. I don't know for sure. What about a scenario where we want to export to a variable which is held in an area that's not a multiple number of bytes? The code isn't dramatically complexified because of it, so perhaps just keep it by default, and remove it later, if we find that we never used 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. It was on purpose, as I tend to write "32bit object" or "64bit object". But if there is a standard way to write these things. A quick google search indeed seems to indicate that there is large preference for having a dash (as in "32-bit computing"). So, if people agree, I will change the above to add a dash. > > > + 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. The problem with what you suggest is that, if we didn't do things right, by the time we realize it, we'll have already gone through a buffer overflow, with the associated random and uncontrolled damage. On Ubuntu, we already know that the overflow causes an abort, so we wouldn't even get to the point where we'd double-check. For me, this risk is bad enough that it's well worth this (small) extra allocation. I don't think this function is going to be called very frequently. > > + (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. Indeed. The above comes from my belief that mpz_export only writes the necessary number of *bytes*, but re-reading the documentation, I belive it's the necessary number of *words*. I don't know how things could possibly be working in the case where mpz_export does the memory allocation otherwise. Thanks for catching that. I'll remove the extra memset. > > @@ -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. I thought I had tried that and got an error, but I will try again. -- Joel