From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id sMVtF7jsxV+JPwAAWB0awg (envelope-from ) for ; Tue, 01 Dec 2020 02:11:52 -0500 Received: by simark.ca (Postfix, from userid 112) id 5DCE51F0AB; Tue, 1 Dec 2020 02:11:52 -0500 (EST) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=0.3 required=5.0 tests=MAILING_LIST_MULTI,RDNS_NONE autolearn=no autolearn_force=no version=3.4.2 Received: from sourceware.org (unknown [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 12AA11E58E for ; Tue, 1 Dec 2020 02:11:52 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 434443854810; Tue, 1 Dec 2020 07:11:51 +0000 (GMT) Received: from rock.gnat.com (rock.gnat.com [IPv6:2620:20:4000:0:a9e:1ff:fe9b:1d1]) by sourceware.org (Postfix) with ESMTP id 8EA3F3854810 for ; Tue, 1 Dec 2020 07:11:48 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 8EA3F3854810 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 3E77456206; Tue, 1 Dec 2020 02:11:48 -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 oM99A1Xt-g3F; Tue, 1 Dec 2020 02:11:48 -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 D4C8C117A15; Tue, 1 Dec 2020 02:11:47 -0500 (EST) Received: by float.home (Postfix, from userid 1000) id 5728FA6B54; Tue, 1 Dec 2020 11:11:42 +0400 (+04) Date: Tue, 1 Dec 2020 11:11:42 +0400 From: Joel Brobecker To: Simon Marchi Subject: Re: [RFA 2/2] gmp-utils: protect gdb_mpz exports against out-of-range values Message-ID: <20201201071142.GB3501@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> <20201201033725.GA3501@adacore.com> <77fba482-26c1-f326-12ef-99c1b7ebe57c@simark.ca> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <77fba482-26c1-f326-12ef-99c1b7ebe57c@simark.ca> 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: , Cc: gdb-patches@sourceware.org Errors-To: gdb-patches-bounces@sourceware.org Sender: "Gdb-patches" > >>> +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. That's no problem. I will remove it. > Also, the name (bit) should be singular. So I think we should > standardize on "%zu-bit". Agreed. > >>> + /* 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 :). Me too. If GMP had a routine that gave us that information prior to calling mpz_export, we could use it and verify that we can confidently call mpz_export without risking a buffer overflow. Right now, we have a formula provided by the manual, but but given in a way that does not instill great confidence ("a calculation like the following"). I'm also concerned with future versions changing the formula and us not noticing. > >>> @@ -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. -- Joel