| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Sat, 17 Aug 2019 14:18:19 +0200
From: Hans de Goede <hdegoede@...hat.com>
To: Herbert Xu <herbert@...dor.apana.org.au>,
Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
"H . Peter Anvin" <hpa@...or.com>,
Heiko Carstens <heiko.carstens@...ibm.com>,
Vasily Gorbik <gor@...ux.ibm.com>,
Christian Borntraeger <borntraeger@...ibm.com>,
Ard Biesheuvel <ard.biesheuvel@...aro.org>,
linux-crypto@...r.kernel.org, x86@...nel.org,
linux-s390@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 6/6] crypto: sha256_generic - Use sha256_transform from
generic sha256 lib
Hi Eric,
Thank you for the review and for the quick turn around time
on the review.
On 17-08-19 07:35, Eric Biggers wrote:
> On Fri, Aug 16, 2019 at 10:13:18PM -0700, Eric Biggers wrote:
>> On Fri, Aug 16, 2019 at 11:16:11PM +0200, Hans de Goede wrote:
>>> Drop the duplicate sha256_transform function from crypto/sha256_generic.c
>>> and use the implementation from lib/crypto/sha256.c instead.
>>> "diff -u lib/crypto/sha256.c sha256_generic.c"
>>> shows that both implementations are identical.
>>>
>>> Signed-off-by: Hans de Goede <hdegoede@...hat.com>
>>
>> Hi Hans, thanks for doing this!
>>
>> I'm a little concerned that the only sha256 lib function which sha256_generic.c
>> calls is sha256_transform(). This means that sha256_init(), sha256_update(),
>> and sha256_final() are not tested by the crypto self-tests. They could be
>> broken and we wouldn't know.
>>
>> IMO, it would be better to make sha256_generic.c use sha256_init(),
>> sha256_update(), and sha256_final() rather than using sha256_base.h.
>> Then we'd get test coverage of both the sha256 lib, and of sha256_base.h
>> via the architecture-specific implementations.
>>
>> To do this you'll also need to add sha224_init(), sha224_update(), and
>> sha224_final(). But that's straightforward.
>>
>
> This is basically what I'm suggesting:
>
> diff --git a/crypto/sha256_generic.c b/crypto/sha256_generic.c
> index 51b3afcb5407..94bb23e33804 100644
> --- a/crypto/sha256_generic.c
> +++ b/crypto/sha256_generic.c
> @@ -39,39 +39,42 @@ const u8 sha256_zero_message_hash[SHA256_DIGEST_SIZE] = {
> };
> EXPORT_SYMBOL_GPL(sha256_zero_message_hash);
>
> -static void sha256_generic_block_fn(struct sha256_state *sst, u8 const *src,
> - int blocks)
> +static int crypto_sha256_init(struct shash_desc *desc)
> {
> - while (blocks--) {
> - sha256_transform(sst->state, src);
> - src += SHA256_BLOCK_SIZE;
> - }
> + return sha256_init(shash_desc_ctx(desc));
> +}
> +
> +static int crypto_sha224_init(struct shash_desc *desc)
> +{
> + return sha224_init(shash_desc_ctx(desc));
> }
>
> int crypto_sha256_update(struct shash_desc *desc, const u8 *data,
> unsigned int len)
> {
> - return sha256_base_do_update(desc, data, len, sha256_generic_block_fn);
> + return sha256_update(shash_desc_ctx(desc), data, len);
> }
> EXPORT_SYMBOL(crypto_sha256_update);
>
> static int crypto_sha256_final(struct shash_desc *desc, u8 *out)
> {
> - sha256_base_do_finalize(desc, sha256_generic_block_fn);
> - return sha256_base_finish(desc, out);
> + if (crypto_shash_digestsize(desc->tfm) == SHA224_DIGEST_SIZE)
> + return sha224_final(shash_desc_ctx(desc), out);
> + else
> + return sha256_final(shash_desc_ctx(desc), out);
> }
>
> int crypto_sha256_finup(struct shash_desc *desc, const u8 *data,
> unsigned int len, u8 *hash)
> {
> - sha256_base_do_update(desc, data, len, sha256_generic_block_fn);
> + sha256_update(shash_desc_ctx(desc), data, len);
> return crypto_sha256_final(desc, hash);
> }
> EXPORT_SYMBOL(crypto_sha256_finup);
>
> static struct shash_alg sha256_algs[2] = { {
> .digestsize = SHA256_DIGEST_SIZE,
> - .init = sha256_base_init,
> + .init = crypto_sha256_init,
> .update = crypto_sha256_update,
> .final = crypto_sha256_final,
> .finup = crypto_sha256_finup,
> @@ -85,7 +88,7 @@ static struct shash_alg sha256_algs[2] = { {
> }
> }, {
> .digestsize = SHA224_DIGEST_SIZE,
> - .init = sha224_base_init,
> + .init = crypto_sha224_init,
> .update = crypto_sha256_update,
> .final = crypto_sha256_final,
> .finup = crypto_sha256_finup,
> diff --git a/include/crypto/sha256.h b/include/crypto/sha256.h
> index f596202ad85f..44e207fb13ad 100644
> --- a/include/crypto/sha256.h
> +++ b/include/crypto/sha256.h
> @@ -21,9 +21,13 @@
> */
>
> extern int sha256_init(struct sha256_state *sctx);
> -extern void sha256_transform(u32 *state, const u8 *input);
> extern int sha256_update(struct sha256_state *sctx, const u8 *input,
> unsigned int length);
> extern int sha256_final(struct sha256_state *sctx, u8 *hash);
>
> +extern int sha224_init(struct sha256_state *sctx);
> +extern int sha224_update(struct sha256_state *sctx, const u8 *input,
> + unsigned int length);
> +extern int sha224_final(struct sha256_state *sctx, u8 *hash);
> +
> #endif /* SHA256_H */
> diff --git a/lib/crypto/sha256.c b/lib/crypto/sha256.c
> index 3e9cc54f7e1c..d808543b3784 100644
> --- a/lib/crypto/sha256.c
> +++ b/lib/crypto/sha256.c
> @@ -42,7 +42,7 @@ static inline void BLEND_OP(int I, u32 *W)
> W[I] = s1(W[I-2]) + W[I-7] + s0(W[I-15]) + W[I-16];
> }
>
> -void sha256_transform(u32 *state, const u8 *input)
> +static void sha256_transform(u32 *state, const u8 *input)
> {
> u32 a, b, c, d, e, f, g, h, t1, t2;
> u32 W[64];
> @@ -204,7 +204,6 @@ void sha256_transform(u32 *state, const u8 *input)
> a = b = c = d = e = f = g = h = t1 = t2 = 0;
> memzero_explicit(W, 64 * sizeof(u32));
> }
> -EXPORT_SYMBOL(sha256_transform);
>
> int sha256_init(struct sha256_state *sctx)
> {
> @@ -222,6 +221,22 @@ int sha256_init(struct sha256_state *sctx)
> }
> EXPORT_SYMBOL(sha256_init);
>
> +int sha224_init(struct sha256_state *sctx)
> +{
> + sctx->state[0] = SHA224_H0;
> + sctx->state[1] = SHA224_H1;
> + sctx->state[2] = SHA224_H2;
> + sctx->state[3] = SHA224_H3;
> + sctx->state[4] = SHA224_H4;
> + sctx->state[5] = SHA224_H5;
> + sctx->state[6] = SHA224_H6;
> + sctx->state[7] = SHA224_H7;
> + sctx->count = 0;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(sha224_init);
> +
> int sha256_update(struct sha256_state *sctx, const u8 *data, unsigned int len)
> {
> unsigned int partial, done;
> @@ -253,7 +268,13 @@ int sha256_update(struct sha256_state *sctx, const u8 *data, unsigned int len)
> }
> EXPORT_SYMBOL(sha256_update);
>
> -int sha256_final(struct sha256_state *sctx, u8 *out)
> +int sha224_update(struct sha256_state *sctx, const u8 *data, unsigned int len)
> +{
> + return sha256_update(sctx, data, len);
> +}
> +EXPORT_SYMBOL(sha224_update);
> +
> +static int __sha256_final(struct sha256_state *sctx, u8 *out, int digest_words)
> {
> __be32 *dst = (__be32 *)out;
> __be64 bits;
> @@ -273,7 +294,7 @@ int sha256_final(struct sha256_state *sctx, u8 *out)
> sha256_update(sctx, (const u8 *)&bits, sizeof(bits));
>
> /* Store state in digest */
> - for (i = 0; i < 8; i++)
> + for (i = 0; i < digest_words; i++)
> dst[i] = cpu_to_be32(sctx->state[i]);
>
> /* Zeroize sensitive information. */
> @@ -281,4 +302,15 @@ int sha256_final(struct sha256_state *sctx, u8 *out)
>
> return 0;
> }
> +
> +int sha256_final(struct sha256_state *sctx, u8 *out)
> +{
> + return __sha256_final(sctx, out, 8);
> +}
> EXPORT_SYMBOL(sha256_final);
> +
> +int sha224_final(struct sha256_state *sctx, u8 *out)
> +{
> + return __sha256_final(sctx, out, 7);
> +}
> +EXPORT_SYMBOL(sha224_final);
Thank you for the patch, I agree with what you are suggesting, I'm
preparing a new version of the patch series with this added.
I'm adding a:
Suggested-by: Eric Biggers <ebiggers@...nel.org>
To credit you for your input on this.
Regards,
Hans
Powered by blists - more mailing lists