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 8F7BB1F5AE for ; Sat, 25 Jul 2020 10:39:47 +0000 (UTC) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 743D43857008; Sat, 25 Jul 2020 10:39:46 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 743D43857008 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1595673586; bh=OULo+5yRw4ANCBmJ8T2PqUv3nOx6XIkSHS7z88+Wlmw=; 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=OC2d55ECkzGR3Ccca2pAmI6q+wVTKjiZqOrqOXlbICume/GgUbFUC1hdjKrmXNvgJ 8hbA6uArzGq8RXk/gT5KDjx5gsy/g214GELHBNUSypTTf81iP8Y+CCraFbxl0trYzv CaH/dGBVLTc0ZAlyxA4mWsw8AwkwoXi/aDpjAEpg= Received: from mail-io1-xd41.google.com (mail-io1-xd41.google.com [IPv6:2607:f8b0:4864:20::d41]) by sourceware.org (Postfix) with ESMTPS id 03F2A3858D37 for ; Sat, 25 Jul 2020 10:39:44 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 03F2A3858D37 Received: by mail-io1-xd41.google.com with SMTP id v6so12332279iob.4 for ; Sat, 25 Jul 2020 03:39:43 -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=OULo+5yRw4ANCBmJ8T2PqUv3nOx6XIkSHS7z88+Wlmw=; b=tLoVpSGUzBty9UQMDZGLA/E6s5Oe8X3vv6KMqqugtmoyAnIaHVdkU+Fi9FzQehTkZ/ Bpd6NWZy9llqjO4RLAcMpK9dSuT6yhqC6OieXUPT83k1I8T+/9hUIzfzZZSOYTpeg87F MlhdUPFNBVofQeIJrVNt6A9SANsKe3sxwd7xTPrDZDeZlZC6XX8KL8Wvvh9lQmzKkIoJ XFr7XPq//WUAdOcYHRfWbw7LYU5z1YIiqbG/iJrBpVFKezws/Cc7FoxWJLBnVRqUjcBX /B9yC7B+rzXqKYllAvNwpKq7yN5525xm8MOoUbkDhYlmM+Lax3ccUJBPSZHLDOR8AQ+e +hHQ== X-Gm-Message-State: AOAM531NAKzVYG/VC5aTAKfs7Rz+QqZehlvV0ZSXCpRRjfZ2KEx+O5sw hGhg31NqfzcxkEA0VpjmwPw7tsJeTErlcXveguc= X-Google-Smtp-Source: ABdhPJzcGwiAe5qcDm6sbU1kfTrl1t0hHVmhx0EifieWjFi+xOby2z+IcWo7pZQfy7U1HvElZ5KSc1yY1ZLnD4EptDg= X-Received: by 2002:a05:6602:1581:: with SMTP id e1mr15013359iow.44.1595673583336; Sat, 25 Jul 2020 03:39:43 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: Date: Sat, 25 Jul 2020 13:39:31 +0300 Message-ID: Subject: Re: [PATCH] Update tcache double-free check To: "Carlos O'Donell" Content-Type: text/plain; charset="UTF-8" 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: GNU C Library Errors-To: libc-alpha-bounces@sourceware.org Sender: "Libc-alpha" 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. On Sat, Jul 25, 2020 at 12:05 AM Carlos O'Donell wrote: > > On 7/24/20 9:37 AM, Eyal Itkin via Libc-alpha wrote: > > Hello, > > > > As we discussed, I've attached here the patch for updating the > > double-free check in the tcache. The patch passes all of malloc's > > existing tests (including the double free tests), and it was tested to > > work as expected both with and without entropy. > > > > Once again, I apologize for sending the patch as an attachment instead > > of inlined in the body of the mail itself (same Gmail issues as > > before). > > > > I am aware that there might be whitespace issues with the patch, > > please feel free to fix them on your end if possible. > > Do we have any concerns having all threads call getrandom()? > > If even *one* thread succeeded at calling getrandom() could we > use that to make ~tcache more random? > > There is certainly a tradeoff here between the number of calls > to getrandom() and the values we use in the key. I haven't > measured the numbers to decide on this yet. > > Do your changes show up in benchtests/bench-malloc-{simple,thread}.c? > > -- > Cheers, > Carlos. >