From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 116229 invoked by alias); 15 Dec 2016 23:31:28 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Received: (qmail 116219 invoked by uid 89); 15 Dec 2016 23:31:27 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.4 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,RCVD_IN_SORBS_SPAM,SPF_PASS autolearn=no version=3.3.2 spammy=dusty, Those, gonna, town X-HELO: mail-qk0-f193.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=li1YEeuB15IKtdtgdjJ21ROAkTcsvbgUf7lANizGMt8=; b=GDtWB4PG36OxrYVbG3c9/X7J5XoKve9KufYyrUW6tVSvugoQUFOS+LsmZ7nAMYE3av eaUiMDPEa/OaurOBN9tA2pzG2PpCV2/3bYZj2uVyYF4ZIZ6pPeQGsQh+hgFg1ckfVXiR auiTwZBInPpyL+3c71oOUWGqG2lurpuRKGcwDflLmhJv97ePsbltzwC3yTN0Qj0+WJlw y2VlrUByf0gRpqARy+XzvV2P8kZXkHfbqK1sYeLBMehLpNGAKbBErLRdHPkkR2F/ETtQ PzXSd0ogRVHUaBhUyvaQiPVS2yUxvzfCsxNT/u6eKOZZ4bZn5XwU5dEgwpAWmSh6FX8V sOGw== X-Gm-Message-State: AIkVDXKQ1xkLmsfcPeQZgYuHh+zHvCuzU8AjmxTft8IqfCvRt5ghdeRo1mMxoZMCPKhh2A== X-Received: by 10.55.18.30 with SMTP id c30mr13814qkh.213.1481844684384; Thu, 15 Dec 2016 15:31:24 -0800 (PST) Subject: Re: [PATCH] explicit_bzero final To: Florian Weimer References: <20161212230622.14045-1-zackw@panix.com> <96232743-345c-5126-e526-9a8aadc4fdb7@redhat.com> <2e51eedf-874e-cb81-79a6-0a0c667371db@redhat.com> <3ea46d8d-4ace-719d-a47e-917c000a0d26@redhat.com> <5e6bafce-8133-ad21-caf6-03a3b981ad3a@redhat.com> Cc: Jeff Law , GNU C Library , Joseph Myers , Adhemerval Zanella , Wilco Dijkstra From: Zack Weinberg Message-ID: <49cb875e-60de-260e-7995-dc75174a0c5f@panix.com> Date: Thu, 15 Dec 2016 23:31:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Icedove/45.5.1 MIME-Version: 1.0 In-Reply-To: <5e6bafce-8133-ad21-caf6-03a3b981ad3a@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-SW-Source: 2016-12/txt/msg00591.txt.bz2 On 12/15/2016 04:08 AM, Florian Weimer wrote: > On 12/14/2016 11:28 PM, Zack Weinberg wrote: > >> The remaining question in my mind is whether, in the case where a >> variable's address is only taken in a call to explicit_bzero, we >> should give up on the "hack to prevent the data being copied to >> memory" for the sake of hypothetical future GCC support. That hack, I >> remind you, is the inline expansion to memset+__glibc_read_memory. We >> made a huge fuss over that case in the manual, and a couple of people >> were prepared to veto explicit_bzero altogether if we didn't do >> something about it. > > Those people were mistaken. I can't see how you can prevent DSE on > scalars with current GCC. I gave that example to show that compiler > support was needed to implement memset_s (or a full implementation of > explicit_zero). It was never intended as something that is particularly > relevant to real-world code. Hmm, OK. I'm going to send a new patch which merges yours and mine and adjusts the manual accordingly. I really want to get this landed tomorrow, before I go out of town -- otherwise it's going to miss 2.25, and this has been dragging on _quite_ long enough. >> -#ifdef _LIBC >> /* >> * Erase key-dependent intermediate data. Data dependent only on >> * the salt is not considered sensitive. >> */ >> - __explicit_bzero (ktab, sizeof (ktab)); >> - __explicit_bzero (data->keysched, sizeof (data->keysched)); >> - __explicit_bzero (res, sizeof (res)); >> -#endif >> + explicit_bzero (ktab, sizeof (ktab)); >> + explicit_bzero (data->keysched, sizeof (data->keysched)); >> + explicit_bzero (res, sizeof (res)); >> >> The _LIBC ifdeffage is vestigial, but should probably be left alone in >> a patch that isn't about that. > > Huh? I think it's a new addition. Oh, huh, you're right. I don't remember why I did that. Certainly nobody's gonna be merging this back with UFC, so it can go. >> +/* This is the generic definition of __explicit_bzero_chk. The >> + __explicit_bzero_chk symbol is used as the implementation of >> + explicit_bzero throughout glibc. If this file is overriden by an >> + architecture, both __explicit_bzero_chk and >> + __explicit_bzero_chk_internal have to be defined (the latter not as >> + an IFUNC). */ >> >> This file is not in sysdeps/generic, so it cannot be overridden (or is >> that no longer the case? If so, why do we still have sysdeps/generic?) >> and I don't think we need the capability to override it. Better we >> should get libc-internal references to memset going to the proper ifunc >> for the architecture. > > It can be overridden, and it has to be, otherwise you end up with > multiple definitions of the same symbol when linking libc.so. ??? Your patch doesn't have any overrides of it. > Two symbols are needed because on some architectures, hidden references > to IFUNC symbols are not supported because calls to hidden functions are > always direct and do not use the GOT. Right, I remember that this was a problem before. >> + /* Compiler barrier. */ >> + asm volatile ("" ::: "memory"); >> +} >> >> I do not understand why you have reverted to an older, inferior >> compiler barrier. This was extensively hashed out quite some time ago. > > I did that because asm volatile ("") is essentially a NOP in this > context. It certainly does not prevent DSE of the preceding memset. In > contrast, the memory clobber ensures that if the thing-to-be-zeroed is > in memory. I meant as opposed to the out-of-line __glibc_read_memory. I suppose an all-memory clobber is not going to generate worse code than that as long as explicit_bzero.c remains separately compiled, and it's less likely to break (but _will_ generate worse code) if LTO-into-glibc happens before compiler support for explicit_bzero, so I can live with it. >> Oh, I see why you're not getting linknamespace failures from the >> libcrypt change: you've implicitly fortified all those calls, which >> has the side effect of making them use an impl-namespace symbol. It >> makes sense as a testing strategy, but it doesn't feel like the right >> move for the committed patch (better to leave that to an all-or-nothing >> "fortify libc internally" switch, ne?) > > I have no firm opinion on that. It avoids adding another GLIBC_PRIVATE > symbol, and I think the current set of symbols is also compatible with > future IFUNC-based optimizations. I think I'm going to leave it your way, both because it'll get the patch done faster, and because the crypt code is awfully dusty and not at all performance-critical. zw