From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id E520A385481E for ; Tue, 1 Dec 2020 04:02:21 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org E520A385481E 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) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 6C7E81E590; Mon, 30 Nov 2020 23:02:21 -0500 (EST) Subject: Re: [RFA 2/2] gmp-utils: protect gdb_mpz exports against out-of-range values To: Joel Brobecker Cc: 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> <20201201033725.GA3501@adacore.com> From: Simon Marchi Message-ID: <77fba482-26c1-f326-12ef-99c1b7ebe57c@simark.ca> Date: Mon, 30 Nov 2020 23:02:20 -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: <20201201033725.GA3501@adacore.com> Content-Type: text/plain; charset=utf-8 Content-Language: fr Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-3.8 required=5.0 tests=BAYES_00, KAM_DMARC_STATUS, NICE_REPLY_A, SPF_HELO_PASS, 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 04:02:23 -0000 On 2020-11-30 10:37 p.m., Joel Brobecker wrote: > 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? Well, I find it more logical to do the other way around, add the complexity when you need it. Because I'm pretty sure you won't go set an alarm clock for one year from now, to remember to come back and check whether or not we are using that feature :). Also, I'm usually not keen on checking in things that aren't used, because we don't even know if they work / do what they are intended to do. If you know things are coming that are going to use it, I'm fine with leaving it in, but otherwise I just don't see the point. I'll let you decide. >>> +{ >>> + 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. I googled a bit and it turns out that this is a kind of compound adjective, which requires a hyphen: https://www.gingersoftware.com/content/grammar-rules/adjectives/compound-adjectives/ Also, the name (bit) should be singular. So I think we should standardize on "%zu-bit". > >> >>> + 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. The way I see it is that if we check after the fact, it would be a gdb_assert. So if we get it wrong, it results in either in a crash or a failed assertion. Both equally result in a bug report. But you're right, using the heap allocation makes it so we can't get it wrong, so that's an advantage. I just thought that we'd get it right for good now :). >>> @@ -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. I tried it before making that suggestion and it worked, so if you have troubles let me know. Simon