From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-Status: No, score=-4.1 required=3.0 tests=AWL,BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,MAILING_LIST_MULTI, SPF_HELO_PASS,SPF_PASS shortcircuit=no 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 (4096 bits) server-digest SHA256) (No client certificate requested) by dcvr.yhbt.net (Postfix) with ESMTPS id 1F1151F531 for ; Mon, 10 Aug 2020 13:07:44 +0000 (UTC) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id C12FB385703F; Mon, 10 Aug 2020 13:07:42 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org C12FB385703F DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1597064862; bh=VyvzAtXp4xBrNQ/q0jL1QmUKABZw2H4S0nRqvJ3qEZs=; h=References:In-Reply-To:Date:Subject:To:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=beHeZZBIvARQB3bwHIxWcgyg3e5IhmSxn3E3Auvkb0I58k5RC3ViC2BPl7sU4FT+j IfS/ItFKZte6S/rybfetsUNZmbkGLiAbocxttJEp7iVtCo2Kp64lB8xPqzPfQ2tUk7 kW8E/TSFHHSG4x5/ArXgXveTXhKY1YUuN+pdxuiA= Received: from mail-il1-x144.google.com (mail-il1-x144.google.com [IPv6:2607:f8b0:4864:20::144]) by sourceware.org (Postfix) with ESMTPS id 1C5AC3857C53 for ; Mon, 10 Aug 2020 13:07:41 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 1C5AC3857C53 Received: by mail-il1-x144.google.com with SMTP id z17so7439842ill.6 for ; Mon, 10 Aug 2020 06:07:41 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=VyvzAtXp4xBrNQ/q0jL1QmUKABZw2H4S0nRqvJ3qEZs=; b=Zc2ak/xzrA2J+V6PcGVFTypum865evdYvsan/ibvvOkt65NWeObM/g26gcpWQkL7KP EmjrDNcY4Re36smkiDzLl9UQhXPsfUB/O5PCJp7i2pMjM632lf9Nq1qi/S0oobJHoI46 zAMRlwN7z8NxCpOIk392MtgF+ZLh1s0Kh4s+9h5PR0xoZbN41vTrW87m8yUxV3a5fk5f KkXxnARvahg2wrw8gVyrw9IwCVCfMXXUnS32w/nQmXJFUdig5VR3oYxC/VtO68uTWPGA REJYcr0EdrZb/D/BXDisqOF13aguICsDKQkxoOXuUC8Upw8iU4tbARF4CdlqcwFz8D4e 0DRg== X-Gm-Message-State: AOAM5326tyVuz1FL8P1bwc0nMFHv6iThZaKLY4BhzqP6B+diJ/je1AdY MHBsEsKPCQEV9Ua6uccVe3FUPa5lNkO6gYBkJqs= X-Google-Smtp-Source: ABdhPJx9rRpm6Bmykblg9eZGQnZYiXxAiKxQcM98rnqMZE0iuoexgt2O+Ts378QogOyd7az90i3nor1ZilfZegtoz1g= X-Received: by 2002:a92:89da:: with SMTP id w87mr17856693ilk.236.1597064859140; Mon, 10 Aug 2020 06:07:39 -0700 (PDT) MIME-Version: 1.0 References: <22dd78e7-13a7-0bec-37ba-f3e32a9630ab@redhat.com> In-Reply-To: <22dd78e7-13a7-0bec-37ba-f3e32a9630ab@redhat.com> Date: Mon, 10 Aug 2020 16:07:28 +0300 Message-ID: Subject: Re: [PATCH] Update tcache double-free check To: "Carlos O'Donell" Content-Type: multipart/mixed; boundary="0000000000009d00a405ac85a3e6" X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Eyal Itkin via Libc-alpha Reply-To: Eyal Itkin Cc: Florian Weimer , GNU C Library Errors-To: libc-alpha-bounces@sourceware.org Sender: "Libc-alpha" --0000000000009d00a405ac85a3e6 Content-Type: text/plain; charset="UTF-8" Updated the patch to perform an atomic update operation on the global entropy state, so to avoid races when multiple threads are initialized simultaneously. The patch now accumulates entropy between threads, while still using the backup case of ~tcache to take care of cases in which no entropy was yet to be available. As Carlos mentioned earlier, I guess you will want to discuss this patch before integrating it. Also, feel free to update the patch if needed in case I missed some whitespace / long line coding style. Thanks again, Eyal On Sun, Jul 26, 2020 at 12:07 AM Carlos O'Donell wrote: > > On 7/25/20 6:39 AM, Eyal Itkin wrote: > > I've gone over the compiled binaries before/after the patch, and the > > change on my x64 binary is as follows: no change to tcache_get() or > > tcache_put(), and an addition of 2 asm instructions to _int_free(). If > > we would have only used ~tcache this would have been reduced to an > > addition of a single asm instruction in _int_free(). > > > > The patch's impact on benchtests/bench-malloc-(simple,thread) is > > negligible, and is less than the natural noise I get on my GCP > > instance between two consecutive benchmark executions. > > > > Since the call to getrandom() is only on tcache_init(), hence once per > > thread, I'm fine with it being called for all threads, but it is > > really for you to decide. > > > > There is a possible fix to make sure that the entropy we get from > > getrandom() will be accumulated, thus helping all future threads that > > might need it. We won't be able to update the keys of previous threads > > (if they lacked entropy), but we can enrich future threads with the > > entropy even if they failed to use getrandom(). This solution will > > also enable us to use all of the received entropy from getrandom() > > even when we get less than the wanted amount of bytes. > > > > The solution for such an entropy accumulation is quite simple: > > > > static __thread size_t tcache_key = 0; > > static size_t tcache_entropy = 0; > > > > void tcache_init() > > { > > ... > > /* Attempt to get a random key for the double-free checks. */ > > size_t local_entropy = 0; > > getrandom (&local_entropy, sizeof(local_entropy), GRND_NONBLOCK); > > /* always accumulate it with the past seen entropy */ > > tcache_entropy ^= local_entropy; > > /* always use the basic alternative of ~tcache in case we didn't > > have any entropy. */ > > tcache_key = tcache_entropy ^ ~((size_t) tcache); > > } > > > > As can be seen, in case that all calls to getrandom() failed, and we > > received nothing from it, we would still use ~tcache, just like > > before. In addition, this use of "~tcache" also adds the entropy from > > ASLR to the tcache_key, so we don't lose anything from using it. > > > > Please advise me of the best way to proceed. > > I like the accumulation of entropy. I'm a little worried about thread > startup and shutdown costs, but I haven't measured that yet. I'm also > busy getting the release out the door (I'm the release manager). > If you have thread startup/shutdown costs that would be interesting > for me to see as a reviewer. > > I'd like to see Florian's opinion on this. > > I fully expect that if you flesh this out you'll have to handle the > getrandom failure case, and the atomics required to avoid the data > race with multiple threads and global state. However, hold off on > that until we get a big more consensus if we really want to go to > this level of complexity. It would be good to keep the entropy we > have in the event the system is under some kind of entropy starvation > at a later point (I've seen at least one of these for TCP sequence > numbers, read and drop from /dev/random etc.) > > -- > Cheers, > Carlos. > --0000000000009d00a405ac85a3e6 Content-Type: application/octet-stream; name="0001-Update-tcache-double-free-check.patch" Content-Disposition: attachment; filename="0001-Update-tcache-double-free-check.patch" Content-Transfer-Encoding: base64 Content-ID: X-Attachment-Id: f_kdoj1y8x0 RnJvbSBiYWY2NjE5NmM0ODZmNmFiNjhiZTM0MGZkYmZmZDBlMGRmNzg0ZTc3IE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBFeWFsIEl0a2luIDxleWFsaXRAY2hlY2twb2ludC5jb20+CkRh dGU6IE1vbiwgMTAgQXVnIDIwMjAgMTU6NTg6MDggKzAzMDAKU3ViamVjdDogW1BBVENIXSBVcGRh dGUgdGNhY2hlIGRvdWJsZS1mcmVlIGNoZWNrCgpVcGRhdGUgdGhlIHZhbHVlIHVzZWQgZm9yIHRo ZSB0Y2FjaGUgZW50cnktPmtleSB3aGVuIGNoZWNraW5nIGZvcgpkb3VibGUgZnJlZSBvcGVyYXRp b25zLiBVc2UgYSByYW5kb20gdmFsdWUgYnkgZGVmYXVsdCwgYW5kIGFjY3VtdWxhdGUKdGhlIGdh dGhlcmVkIGVudHJvcHkgYWNyb3NzIHRpbWUgYW5kIGFjcm9zcyB0aHJlYWRzLiBJbiBhZGRpdGlv biwgdXNlCn50Y2FjaGUgYXMgYSBiYWNrdXAgdmFsdWUgaW5jYXNlIHRoZXJlIGlzbid0IGVub3Vn aCBlbnRyb3B5IC8gZW50cm9weQppc24ndCBhdmFpbGFibGUuCgpPcmlnaW5hbCBrZXkgdmFsdWUg d2FzICJ0Y2FjaGUiIHdoaWNoIG1heSBsZWFkIHRvIHNlY3VyaXR5IGlzc3VlcyBpbgpjb2RlIHdp dGggdXNlLWFmdGVyLWZyZWUgdnVsbmVyYWJpbGl0aWVzICgiSG91c2Ugb2YgSW8iIGV4cGxvaXQp LiBUaGUKbmV3IGtleSBpcyBubyBsb25nZXIgYSB2YWxpZCBwb2ludGVyIHRvIGEgY3JpdGljYWwg bWV0YS1kYXRhIHN0cnVjdC4KLS0tCiBtYWxsb2MvbWFsbG9jLmMgfCAyOSArKysrKysrKysrKysr KysrKysrKysrKystLS0tLQogMSBmaWxlIGNoYW5nZWQsIDI0IGluc2VydGlvbnMoKyksIDUgZGVs ZXRpb25zKC0pCgpkaWZmIC0tZ2l0IGEvbWFsbG9jL21hbGxvYy5jIGIvbWFsbG9jL21hbGxvYy5j CmluZGV4IGVlODdkZGJiZjkuLjRlOWE1ZTE3ZWYgMTAwNjQ0Ci0tLSBhL21hbGxvYy9tYWxsb2Mu YworKysgYi9tYWxsb2MvbWFsbG9jLmMKQEAgLTI0Nyw2ICsyNDcsOSBAQAogLyogRm9yIFNJTkdM RV9USFJFQURfUC4gICovCiAjaW5jbHVkZSA8c3lzZGVwLWNhbmNlbC5oPgogCisvKiBGb3IgdGNh Y2hlIGRvdWJsZS1mcmVlIGNoZWNrcy4gICovCisjaW5jbHVkZSA8c3lzL3JhbmRvbS5oPgorCiAv KgogICBEZWJ1Z2dpbmc6CiAKQEAgLTI5MTAsNyArMjkxMyw3IEBAIHR5cGVkZWYgc3RydWN0IHRj YWNoZV9lbnRyeQogewogICBzdHJ1Y3QgdGNhY2hlX2VudHJ5ICpuZXh0OwogICAvKiBUaGlzIGZp ZWxkIGV4aXN0cyB0byBkZXRlY3QgZG91YmxlIGZyZWVzLiAgKi8KLSAgc3RydWN0IHRjYWNoZV9w ZXJ0aHJlYWRfc3RydWN0ICprZXk7CisgIHNpemVfdCBrZXk7CiB9IHRjYWNoZV9lbnRyeTsKIAog LyogVGhlcmUgaXMgb25lIG9mIHRoZXNlIGZvciBlYWNoIHRocmVhZCwgd2hpY2ggY29udGFpbnMg dGhlCkBAIC0yOTI2LDYgKzI5MjksOCBAQCB0eXBlZGVmIHN0cnVjdCB0Y2FjaGVfcGVydGhyZWFk X3N0cnVjdAogCiBzdGF0aWMgX190aHJlYWQgYm9vbCB0Y2FjaGVfc2h1dHRpbmdfZG93biA9IGZh bHNlOwogc3RhdGljIF9fdGhyZWFkIHRjYWNoZV9wZXJ0aHJlYWRfc3RydWN0ICp0Y2FjaGUgPSBO VUxMOworc3RhdGljIF9fdGhyZWFkIHNpemVfdCB0Y2FjaGVfa2V5ID0gMDsKK3N0YXRpYyBzaXpl X3QgdGNhY2hlX2VudHJvcHkgPSAwOwogCiAvKiBDYWxsZXIgbXVzdCBlbnN1cmUgdGhhdCB3ZSBr bm93IHRjX2lkeCBpcyB2YWxpZCBhbmQgdGhlcmUncyByb29tCiAgICBmb3IgbW9yZSBjaHVua3Mu ICAqLwpAQCAtMjkzNiw3ICsyOTQxLDcgQEAgdGNhY2hlX3B1dCAobWNodW5rcHRyIGNodW5rLCBz aXplX3QgdGNfaWR4KQogCiAgIC8qIE1hcmsgdGhpcyBjaHVuayBhcyAiaW4gdGhlIHRjYWNoZSIg c28gdGhlIHRlc3QgaW4gX2ludF9mcmVlIHdpbGwKICAgICAgZGV0ZWN0IGEgZG91YmxlIGZyZWUu ICAqLwotICBlLT5rZXkgPSB0Y2FjaGU7CisgIGUtPmtleSA9IHRjYWNoZV9rZXk7CiAKICAgZS0+ bmV4dCA9IFBST1RFQ1RfUFRSICgmZS0+bmV4dCwgdGNhY2hlLT5lbnRyaWVzW3RjX2lkeF0pOwog ICB0Y2FjaGUtPmVudHJpZXNbdGNfaWR4XSA9IGU7CkBAIC0yOTUzLDcgKzI5NTgsNyBAQCB0Y2Fj aGVfZ2V0IChzaXplX3QgdGNfaWR4KQogICAgIG1hbGxvY19wcmludGVyciAoIm1hbGxvYygpOiB1 bmFsaWduZWQgdGNhY2hlIGNodW5rIGRldGVjdGVkIik7CiAgIHRjYWNoZS0+ZW50cmllc1t0Y19p ZHhdID0gUkVWRUFMX1BUUiAoZS0+bmV4dCk7CiAgIC0tKHRjYWNoZS0+Y291bnRzW3RjX2lkeF0p OwotICBlLT5rZXkgPSBOVUxMOworICBlLT5rZXkgPSAwOwogICByZXR1cm4gKHZvaWQgKikgZTsK IH0KIApAQCAtMzAxOSw4ICszMDI0LDIyIEBAIHRjYWNoZV9pbml0KHZvaWQpCiAgICAgewogICAg ICAgdGNhY2hlID0gKHRjYWNoZV9wZXJ0aHJlYWRfc3RydWN0ICopIHZpY3RpbTsKICAgICAgIG1l bXNldCAodGNhY2hlLCAwLCBzaXplb2YgKHRjYWNoZV9wZXJ0aHJlYWRfc3RydWN0KSk7Ci0gICAg fQogCisgICAgICAvKiBBdHRlbXB0IHRvIGdldCBhIHJhbmRvbSBrZXkgZm9yIHRoZSBkb3VibGUt ZnJlZSBjaGVja3MuICAqLworICAgICAgc2l6ZV90IGxvY2FsX2VudHJvcHkgPSAwLCB1cGRhdGVk X2VudHJvcHksIGVudHJvcHksIG9sZDsKKyAgICAgIGdldHJhbmRvbSAoJmxvY2FsX2VudHJvcHks IHNpemVvZihsb2NhbF9lbnRyb3B5KSwgR1JORF9OT05CTE9DSyk7CisgICAgICAvKiBBbHdheXMg YWNjdW11bGF0ZSBpdCB3aXRoIHBhc3Qgc2VlbiBlbnRyb3B5LiAgKi8KKyAgICAgIG9sZCA9IHRj YWNoZV9lbnRyb3B5OworICAgICAgZG8KKyAgICAgICAgeworICAgICAgICAgIGVudHJvcHkgPSBv bGQ7CisgICAgICAgICAgdXBkYXRlZF9lbnRyb3B5ID0gbG9jYWxfZW50cm9weSBeIGVudHJvcHk7 CisgICAgICAgIH0KKyAgICAgIHdoaWxlICgob2xkID0gY2F0b21pY19jb21wYXJlX2FuZF9leGNo YW5nZV92YWxfYWNxICgmdGNhY2hlX2VudHJvcHksCisgICAgICAgIHVwZGF0ZWRfZW50cm9weSwg ZW50cm9weSkpICE9IGVudHJvcHkpOworICAgICAgLyogQWx3YXlzIHVzZSB0aGUgZGVmYXVsdCBh Z3JlZWQgYWx0ZXJuYXRpdmU6IH50Y2FjaGUuICAqLworICAgICAgdGNhY2hlX2tleSA9IHVwZGF0 ZWRfZW50cm9weSBeIH4oKHNpemVfdCkgdGNhY2hlKTsKKyAgICB9CiB9CiAKICMgZGVmaW5lIE1B WUJFX0lOSVRfVENBQ0hFKCkgXApAQCAtNDIxOCw3ICs0MjM3LDcgQEAgX2ludF9mcmVlIChtc3Rh dGUgYXYsIG1jaHVua3B0ciBwLCBpbnQgaGF2ZV9sb2NrKQogCSAgIHRydXN0IGl0IChpdCBhbHNv IG1hdGNoZXMgcmFuZG9tIHBheWxvYWQgZGF0YSBhdCBhIDEgaW4KIAkgICAyXjxzaXplX3Q+IGNo YW5jZSksIHNvIHZlcmlmeSBpdCdzIG5vdCBhbiB1bmxpa2VseQogCSAgIGNvaW5jaWRlbmNlIGJl Zm9yZSBhYm9ydGluZy4gICovCi0JaWYgKF9fZ2xpYmNfdW5saWtlbHkgKGUtPmtleSA9PSB0Y2Fj aGUpKQorCWlmIChfX2dsaWJjX3VubGlrZWx5IChlLT5rZXkgPT0gdGNhY2hlX2tleSkpCiAJICB7 CiAJICAgIHRjYWNoZV9lbnRyeSAqdG1wOwogCSAgICBMSUJDX1BST0JFIChtZW1vcnlfdGNhY2hl X2RvdWJsZV9mcmVlLCAyLCBlLCB0Y19pZHgpOwotLSAKMi4xNy4xCgo= --0000000000009d00a405ac85a3e6--