Linux-Crypto Archive on lore.kernel.org
 help / color / Atom feed
From: Hans de Goede <hdegoede@redhat.com>
To: Ingo Molnar <mingo@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	"H . Peter Anvin" <hpa@zytor.com>,
	Herbert Xu <herbert@gondor.apana.org.au>,
	Ard Biesheuvel <ard.biesheuvel@linaro.org>,
	linux-crypto@vger.kernel.org, x86@kernel.org,
	linux-kernel@vger.kernel.org,
	Arvind Sankar <nivedita@alum.mit.edu>,
	Stephan Mueller <smueller@chronox.de>
Subject: Re: [PATCH v2 5.4 regression fix] x86/boot: Provide memzero_explicit
Date: Mon, 7 Oct 2019 16:11:51 +0200
Message-ID: <1dc3c53d-785e-f9a4-1b4c-3374c94ae0a7@redhat.com> (raw)
In-Reply-To: <20191007140022.GA29008@gmail.com>

Hi,

On 07-10-2019 16:00, Ingo Molnar wrote:
> 
> * Hans de Goede <hdegoede@redhat.com> wrote:
> 
>> The purgatory code now uses the shared lib/crypto/sha256.c sha256
>> implementation. This needs memzero_explicit, implement this.
>>
>> Reported-by: Arvind Sankar <nivedita@alum.mit.edu>
>> Fixes: 906a4bb97f5d ("crypto: sha256 - Use get/put_unaligned_be32 to get input, memzero_explicit")
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v2:
>> - Add barrier_data() call after the memset, making the function really
>>    explicit. Using barrier_data() works fine in the purgatory (build)
>>    environment.
>> ---
>>   arch/x86/boot/compressed/string.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/arch/x86/boot/compressed/string.c b/arch/x86/boot/compressed/string.c
>> index 81fc1eaa3229..654a7164a702 100644
>> --- a/arch/x86/boot/compressed/string.c
>> +++ b/arch/x86/boot/compressed/string.c
>> @@ -50,6 +50,12 @@ void *memset(void *s, int c, size_t n)
>>   	return s;
>>   }
>>   
>> +void memzero_explicit(void *s, size_t count)
>> +{
>> +	memset(s, 0, count);
>> +	barrier_data(s);
>> +}
> 
> So the barrier_data() is only there to keep LTO from optimizing out the
> seemingly unused function?

I believe that Stephan Mueller (who suggested adding the barrier)
was also worried about people using this as an example for other
"explicit" functions which actually might get inlined.

This is not so much about protecting against LTO as it is against
protecting against inlining, which in this case boils down to the
same thing. Also this change makes the arch/x86/boot/compressed/string.c
and lib/string.c versions identical which seems like a good thing to me
(except for the code duplication part of it).

But I agree a comment would be good, how about:

void memzero_explicit(void *s, size_t count)
{
	memset(s, 0, count);
	/* Avoid the memset getting optimized away if we ever get inlined */
	barrier_data(s);
}

?

Regards,

Hans

  reply index

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-07 13:47 Hans de Goede
2019-10-07 14:00 ` Ingo Molnar
2019-10-07 14:11   ` Hans de Goede [this message]
2019-10-07 14:22     ` Ingo Molnar
2019-10-07 14:29       ` Hans de Goede
2019-10-07 14:46         ` Ingo Molnar
2019-10-07 15:20           ` Arvind Sankar
2019-10-07 15:40             ` Ingo Molnar
2019-10-07 18:42               ` Arvind Sankar
2019-10-07 19:36                 ` Hans de Goede
2019-10-07 22:00                   ` [PATCH] lib/string: make memzero_explicit inline instead of external Arvind Sankar
2019-10-08 11:33                     ` [tip: x86/urgent] lib/string: Make memzero_explicit() " tip-bot2 for Arvind Sankar
2019-10-08 11:33                     ` tip-bot2 for Arvind Sankar
2019-10-10  2:52                     ` [PATCH] lib/string: make memzero_explicit " Dave Young
2019-10-10  6:56                       ` Dave Young
2019-10-07 14:49 ` [tip: x86/urgent] x86/boot: Provide memzero_explicit() tip-bot2 for Hans de Goede
2019-10-07 14:49 ` tip-bot2 for Hans de Goede

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1dc3c53d-785e-f9a4-1b4c-3374c94ae0a7@redhat.com \
    --to=hdegoede@redhat.com \
    --cc=ard.biesheuvel@linaro.org \
    --cc=bp@alien8.de \
    --cc=herbert@gondor.apana.org.au \
    --cc=hpa@zytor.com \
    --cc=linux-crypto@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@kernel.org \
    --cc=mingo@redhat.com \
    --cc=nivedita@alum.mit.edu \
    --cc=smueller@chronox.de \
    --cc=tglx@linutronix.de \
    --cc=x86@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Crypto Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-crypto/0 linux-crypto/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-crypto linux-crypto/ https://lore.kernel.org/linux-crypto \
		linux-crypto@vger.kernel.org
	public-inbox-index linux-crypto

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-crypto


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git