From patchwork Wed Dec 14 22:28:08 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Zack Weinberg X-Patchwork-Id: 705802 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3tfB6w4T9Dz9sdn for ; Thu, 15 Dec 2016 09:28:24 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; secure) header.d=sourceware.org header.i=@sourceware.org header.b="ixf5a6ww"; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc:content-type; q=dns; s=default; b=rcQR P6fxY/1tlYfYAdxzrugkO43hiSjV6sKcsDP6ffGgBuQgIPoZsbjzJ3NdFS9S7e3l NGHAM0swoVM6YLf9p/3rEVgDqNfeAWTjZun6PegT0ldliSS5bonp+Xn4GyMQEBNW nFpIVUFC4jQI74MbU3ElbMBJ9QtLcvO+SXF+c0A= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc:content-type; s=default; bh=V+08/HSfaz H+DXRYlGtcCbekVqM=; b=ixf5a6ww8xa+rImFYky9JCJ2sym4QJQDn95EWjbTHp MHwadF7/hb2k6Gi21T0DQHTMiMzjKhwHvy6+TcuwcxaLoxPYBEkstPa2qMXr0EY6 HvmzNn6jEkiZ3YcTTAVWHKho+MDbdDUFvdjvzvys9+XxL7vuUDfOiwyyOgODLBCN 8= Received: (qmail 30498 invoked by alias); 14 Dec 2016 22:28:16 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 30485 invoked by uid 89); 14 Dec 2016 22:28:15 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-5.0 required=5.0 tests=AWL, BAYES_00, RP_MATCHES_RCVD, SPF_PASS autolearn=ham version=3.3.2 spammy=reluctant, veto, yours X-HELO: mailbackend.panix.com X-Gm-Message-State: AKaTC03SGo30u8RZ9JF3La4fdsr16i/CqOjt6pjTdthMGSfV8FXGvB0a08B26UqfiZ4P8cKLbDBr5Dt/nLACCg== X-Received: by 10.194.52.42 with SMTP id q10mr88715997wjo.50.1481754488814; Wed, 14 Dec 2016 14:28:08 -0800 (PST) MIME-Version: 1.0 In-Reply-To: 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> From: Zack Weinberg Date: Wed, 14 Dec 2016 17:28:08 -0500 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH] explicit_bzero final To: Florian Weimer Cc: Jeff Law , GNU C Library , Joseph Myers , Adhemerval Zanella , Wilco Dijkstra On Wed, Dec 14, 2016 at 12:05 PM, Florian Weimer wrote: > On 12/14/2016 02:15 PM, Florian Weimer wrote: >> On 12/14/2016 02:04 AM, Zack Weinberg wrote: >>> >>> We also have a nasty interaction between internal PLT bypass and >>> ifuncs which means that a hypothetical __explicit_bzero_chk is >>> ridiculously difficult to implement. I tried that once already and it >>> did not go well. >> >> I'm looking into this aspect right now. > > This patch on top of yours implements proper explict_bzero and > __explicit_bzero_chk symbols. I put it through build-many-glibcs, and it > results in the expected ABI everywhere. I appreciate your having tried this; the patch has a number of correctable problems (see below) but it does demonstrate that __explicit_bzero_chk is not a lost cause. 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. I am very reluctant to give it up, especially as I'm still not convinced it's a problem for the compiler (see reply to Jeff). 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?) What we _could_ do is #if IS_IN (libc) # define explicit_bzero(s, n) __internal_explicit_bzero (s, n) #else # define explicit_bzero(s, n) __explicit_bzero (s, n) #endif which would allow libcrypt's source code to use the unmangled names. I think that's something we're trying to do for other string functions, so perhaps it makes sense. --- a/crypt/crypt-entry.c +++ b/crypt/crypt-entry.c @@ -142,15 +142,13 @@ __crypt_r (const char *key, const char *salt, */ _ufc_output_conversion_r (res[0], res[1], salt, data); -#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. libcrypt really does need to refer to __explicit_bzero, not explicit_bzero. Joseph can explain better than I can, but the fundamental constraint is that the implementation of a standardized function ('crypt' is POSIX) is not allowed to refer to nonstandard user-namespace symbols. This change should have triggered linknamespace failures. +/* 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. + /* 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. --- a/include/string.h +++ b/include/string.h @@ -100,20 +100,15 @@ extern __typeof (memmem) __memmem; libc_hidden_proto (__memmem) libc_hidden_proto (__ffs) -/* explicit_bzero is used in libcrypt. */ -extern __typeof (explicit_bzero) __explicit_bzero; -extern __typeof (explicit_bzero) __internal_explicit_bzero; -libc_hidden_proto (__internal_explicit_bzero) -extern __typeof (__glibc_read_memory) __internal_glibc_read_memory; -libc_hidden_proto (__internal_glibc_read_memory) -/* Honor string.h inlines when present. */ -#if __GNUC_PREREQ (3,4) \ - && ((defined __extern_always_inline \ - && defined __OPTIMIZE__ && !defined __OPTIMIZE_SIZE__ \ - && !defined __NO_INLINE__ && !defined __NO_STRING_INLINES) \ - || (__USE_FORTIFY_LEVEL > 0 && defined __fortify_function)) -# define __explicit_bzero(s,n) explicit_bzero (s,n) -# define __internal_explicit_bzero(s,n) explicit_bzero (s,n) +#if IS_IN (libc) +/* Avoid hidden reference to IFUNC symbol __explicit_bzero_chk. */ +void __explicit_bzero_chk_internal (void *, size_t, size_t) + __THROW __nonnull ((1)) attribute_hidden; +# define explicit_bzero(buf, len) \ + __explicit_bzero_chk_internal (buf, len, __bos0 (buf)) +#elif !IS_IN (nonlib) +void __explicit_bzero_chk (void *, size_t, size_t) __THROW __nonnull ((1)); +# define explicit_bzero(buf, len) __explicit_bzero_chk (buf, len, __bos0 (buf)) #endif Oh, I see why you're not getting linknamespace failures from the