crypto: blake2b - use memcpy_and_pad in __blake2b_init
PATCH 1/2 crypto: blake2b - use memcpy_and_pad in __blake2b_init
Use memcpy_and_pad() instead of memcpy() followed by memset() to simplify __blake2b_init(). Use sizeof(ctx->buf) instead of the macro BLAKE2B_BLOCK_SIZE.
--- include/crypto/blake2b.h | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
include/crypto/blake2b.h +2 -3
@@ -66,9 +66,8 @@ static inline void __blake2b_init(struct blake2b_ctx *ctx, size_t outlen, ctx->buflen =0 ; ctx->outlen = outlen;if (keylen) { -memcpy (ctx->buf, key, keylen); -memset (&ctx->buf[keylen],0 ,BLAKE2B_BLOCK_SIZE - keylen); - ctx->buflen =BLAKE2B_BLOCK_SIZE ; +memcpy_and_pad (ctx->buf,sizeof (ctx->buf), key, keylen,0 ); + ctx->buflen =sizeof (ctx->buf); } }
PATCH 2/2 crypto: blake2s - use memcpy_and_pad in __blake2s_init
Use memcpy_and_pad() instead of memcpy() followed by memset() to simplify __blake2s_init(). Use sizeof(ctx->buf) instead of the macro BLAKE2S_BLOCK_SIZE.
--- include/crypto/blake2s.h | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
include/crypto/blake2s.h +2 -3
@@ -70,9 +70,8 @@ static inline void __blake2s_init(struct blake2s_ctx *ctx, size_t outlen, ctx->buflen =0 ; ctx->outlen = outlen;if (keylen) { -memcpy (ctx->buf, key, keylen); -memset (&ctx->buf[keylen],0 ,BLAKE2S_BLOCK_SIZE - keylen); - ctx->buflen =BLAKE2S_BLOCK_SIZE ; +memcpy_and_pad (ctx->buf,sizeof (ctx->buf), key, keylen,0 ); + ctx->buflen =sizeof (ctx->buf); } }
Re: [PATCH 2/2] crypto: blake2s - use memcpy_and_pad in __blake2s_init
Quoted 14 lines · Thorsten Blum
Use
memcpy_and_pad()instead ofmemcpy()followed bymemset()to simplify__blake2s_init(). Usesizeof(ctx->buf)instead of the macroBLAKE2S_BLOCK_SIZE.Signed-off-by Thorsten Blum <[email protected]>--- include/crypto/blake2s.h | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)include/crypto/blake2s.h +0
-0
include/crypto/blake2s.h static inline void __blake2s_init(struct blake2s_ctx *ctx, size_t outlen, · line 74
Quoted 8 lines
include/crypto/blake2s.h +2
-3@@ -70,9 +70,8 @@ static inline void __blake2s_init(struct blake2s_ctx *ctx, size_t outlen, ctx->buflen =0 ; ctx->outlen = outlen;if (keylen) { -memcpy (ctx->buf, key, keylen); -memset (&ctx->buf[keylen],0 ,BLAKE2S_BLOCK_SIZE - keylen); - ctx->buflen =BLAKE2S_BLOCK_SIZE ; +memcpy_and_pad (ctx->buf,sizeof (ctx->buf), key, keylen,0 ); + ctx->buflen =sizeof (ctx->buf);
I'm wondering if this is actually better. It's another helper function to remember. Also 'keylen' can be a compile-time constant here, and compilers know what memcpy() and memset() do, so they will optimize the code accordingly. The helper function takes away the compiler's ability to perform this optimization. If this was already an out-of-line function, it would be a bit more convincing.
- Eric
Re: [PATCH 2/2] crypto: blake2s - use memcpy_and_pad in __blake2s_init
Quoted 22 lines · Eric Biggers
Quoted 21 lines · Thorsten Blum
Use
memcpy_and_pad()instead ofmemcpy()followed bymemset()to simplify__blake2s_init(). Usesizeof(ctx->buf)instead of the macroBLAKE2S_BLOCK_SIZE.Signed-off-by Thorsten Blum <[email protected]>--- include/crypto/blake2s.h | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)include/crypto/blake2s.h +1
-3@@ -70,9 +70,8 @@ static inline void __blake2s_init(struct blake2s_ctx *ctx, size_t outlen, ctx->buflen =0 ; ctx->outlen = outlen;if (keylen) { -memcpy (ctx->buf, key, keylen); -memset (&ctx->buf[keylen],0 ,BLAKE2S_BLOCK_SIZE - keylen); - ctx->buflen =BLAKE2S_BLOCK_SIZE ; +memcpy_and_pad (ctx->buf,sizeof (ctx->buf), key, keylen,0 );
Quoted 8 lines
include/crypto/blake2s.h static inline void __blake2s_init(struct blake2s_ctx *ctx, size_t outlen, · line 74
Quoted 1 line
include/crypto/blake2s.h +1
-0+ ctx->buflen =sizeof (ctx->buf);I'm wondering if this is actually better. It's another helper function to remember. Also 'keylen' can be a compile-time constant here, and compilers know what
memcpy()andmemset()do, so they will optimize the code accordingly. The helper function takes away the compiler's ability to perform this optimization. If this was already an out-of-line function, it would be a bit more convincing.
My motivation was readability/maintainability and avoiding the manual tail-size arithmetic. memcpy_and_pad() is just a thin wrapper around memcpy()/memset(), with an additional safety check to prevent integer wraparound.
Currently, nothing stops __blake2s_init() or blake2s_init_key() from being called with keylen > BLAKE2S_BLOCK_SIZE. The WARN_ON() in blake2s_init_key() is only available in DEBUG builds.
That said, happy to drop the patch if the explicit version is preferred for performance reasons.
Thanks,
Thorsten
Re: [PATCH 2/2] crypto: blake2s - use memcpy_and_pad in __blake2s_init
Quoted 37 lines · Thorsten Blum
Quoted 22 lines · Eric Biggers
Quoted 21 lines · Thorsten Blum
Use
memcpy_and_pad()instead ofmemcpy()followed bymemset()to simplify__blake2s_init(). Usesizeof(ctx->buf)instead of the macroBLAKE2S_BLOCK_SIZE.Signed-off-by Thorsten Blum <[email protected]>--- include/crypto/blake2s.h | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)include/crypto/blake2s.h +1
-3@@ -70,9 +70,8 @@ static inline void __blake2s_init(struct blake2s_ctx *ctx, size_t outlen, ctx->buflen =0 ; ctx->outlen = outlen;if (keylen) { -memcpy (ctx->buf, key, keylen); -memset (&ctx->buf[keylen],0 ,BLAKE2S_BLOCK_SIZE - keylen); - ctx->buflen =BLAKE2S_BLOCK_SIZE ; +memcpy_and_pad (ctx->buf,sizeof (ctx->buf), key, keylen,0 );Quoted 8 lines
include/crypto/blake2s.h static inline void __blake2s_init(struct blake2s_ctx *ctx, size_t outlen, · line 74
Quoted 1 line
include/crypto/blake2s.h +1
-0+ ctx->buflen =sizeof (ctx->buf);I'm wondering if this is actually better. It's another helper function to remember. Also 'keylen' can be a compile-time constant here, and compilers know what
memcpy()andmemset()do, so they will optimize the code accordingly. The helper function takes away the compiler's ability to perform this optimization. If this was already an out-of-line function, it would be a bit more convincing.My motivation was readability/maintainability and avoiding the manual tail-size arithmetic.
memcpy_and_pad()is just a thin wrapper around memcpy()/memset(), with an additional safety check to prevent integer wraparound.
Quoted 6 lines
Currently, nothing stops
__blake2s_init()orblake2s_init_key()from being called with keylen >BLAKE2S_BLOCK_SIZE. TheWARN_ON()inblake2s_init_key()is only available in DEBUG builds.That said, happy to drop the patch if the explicit version is preferred for performance reasons.
I think the memcpy_and_pad() variant is *less* readable for most people. memcpy_and_pad() isn't widely known, so people have to jump to the definition to understand what it does.
Of course, we do create helper functions all the time when they are helpful. This one just doesn't seem all that helpful here, I'm afraid.
As for keylen > BLAKE2S_KEY_SIZE, that would be a usage error, which would need to be found and fixed. It certainly shouldn't be silently truncated. It looks like if this were to happen (currently it doesn't), the current code would do memcpy with an underflowed value, which would crash, similar to a BUG_ON(). That's actually better than the silent truncation you're proposing, as it allows the problem to be found and fixed. If we add the truncation, it should at least be noisy:
if (WARN_ON(keylen > BLAKE2S_KEY_SIZE))
keylen = BLAKE2S_KEY_SIZE;
- Eric