Skip to contentSkip to conversation

crypto: sha1: modify sha1_update to use SHA1_BLOCK_SIZE

PATCH Superseded Mandeep Singh Baines 5 participants 7 messages May 26, 2011 – Jun 27, 2011 Crypto API

lore
Share & tools
lore

Mandeep Singh Baines

author
#0 lore raw

PATCH 1/1 crypto: sha1: modify sha1_update to use SHA1_BLOCK_SIZE

Plus some other minor cleanup.

Signed-off-by Mandeep Singh Baines <[email protected]>
Cc Herbert Xu <[email protected]>
Cc David S. Miller <[email protected]>
Cc [email protected]
---
 crypto/sha1_generic.c |   39 +++++++++++++++++++--------------------
 1 files changed, 19 insertions(+), 20 deletions(-)
crypto/sha1_generic.c +19 -20
index 0416091..4bdd228 100644
@@ -40,33 +40,32 @@ static int sha1_update(struct shash_desc *desc, const u8 *data,
 			unsigned int len)
 {
 	struct sha1_state *sctx = shash_desc_ctx(desc);
-	unsigned int partial, done;
-	const u8 *src;
+	unsigned int partial = sctx->count % SHA1_BLOCK_SIZE;
+	u32 temp[SHA_WORKSPACE_WORDS];
+	const u8 *src = data;
 
-	partial = sctx->count & 0x3f;
 	sctx->count += len;
-	done = 0;
-	src = data;
 
-	if ((partial + len) > 63) {
-		u32 temp[SHA_WORKSPACE_WORDS];
+	if (partial && ((partial + len) >= SHA1_BLOCK_SIZE)) {
+		unsigned int done = SHA1_BLOCK_SIZE - partial;
 
-		if (partial) {
-			done = -partial;
-			memcpy(sctx->buffer + partial, data, done + 64);
-			src = sctx->buffer;
-		}
+		memcpy(sctx->buffer + partial, src, done);
+		sha_transform(sctx->state, sctx->buffer, temp);
+		len -= done;
+		src += done;
+		partial = 0;
+	}
 
-		do {
-			sha_transform(sctx->state, src, temp);
-			done += 64;
-			src = data + done;
-		} while (done + 63 < len);
+	while (len >= SHA1_BLOCK_SIZE) {
+		sha_transform(sctx->state, src, temp);
+		len -= SHA1_BLOCK_SIZE;
+		src += SHA1_BLOCK_SIZE;
+	}
 
+	if (src != data)
 		memset(temp, 0, sizeof(temp));
-		partial = 0;
-	}
-	memcpy(sctx->buffer + partial, src, len - done);
+
+	memcpy(sctx->buffer + partial, src, len);
 
 	return 0;
 }
Signature · 1 line
1.7.3.1
maintainer
#1 lore raw

From: Mandeep Singh Baines <[email protected]>
Date: Wed, 25 May 2011 20:11:17 -0700

Quoted 3 lines

Plus some other minor cleanup.

Signed-off-by Mandeep Singh Baines <[email protected]>

The temp[] buffer is explicitly places inside the inner most basic block so that the compiler doesn't allocate the stack space unless that code path is taken.

Besides the use of the SHA_* macros, I think your changes actually make the code worse.

#2 lore raw
Quoted 7 lines · David Miller

From: Mandeep Singh Baines <[email protected]>
Date: Wed, 25 May 2011 20:11:17 -0700

Quoted 2 lines

Plus some other minor cleanup.

Signed-off-by Mandeep Singh Baines <[email protected]>

The temp[] buffer is explicitly places inside the inner most basic block so that the compiler doesn't allocate the stack space unless that code path is taken.

Does any version of gcc manage to do that?

Regardless, it's still a good idea to keep
declarations in the use scope.

author
#3 lore raw

PATCH v2 1/1 crypto: sha1: modify sha1_update to use SHA1_BLOCK_SIZE

In reply to David Miller
Quoted 5 lines · David Miller

The temp[] buffer is explicitly places inside the inner most basic block so that the compiler doesn't allocate the stack space unless that code path is taken.

Fixed in V2 (this patch). Thanks for the review.

-- >8 -- (snip)

Plus some other minor cleanup.

Signed-off-by Mandeep Singh Baines <[email protected]>
Cc Herbert Xu <[email protected]>
Cc David S. Miller <[email protected]>
Cc Joe Perches <[email protected]>
Cc [email protected]
---
 crypto/sha1_generic.c |   33 +++++++++++++++------------------
 1 files changed, 15 insertions(+), 18 deletions(-)
crypto/sha1_generic.c +15 -18
index 0416091..0b56719 100644
@@ -40,33 +40,30 @@ static int sha1_update(struct shash_desc *desc, const u8 *data,
 			unsigned int len)
 {
 	struct sha1_state *sctx = shash_desc_ctx(desc);
-	unsigned int partial, done;
-	const u8 *src;
+	unsigned int partial = sctx->count % SHA1_BLOCK_SIZE;
 
-	partial = sctx->count & 0x3f;
 	sctx->count += len;
-	done = 0;
-	src = data;
 
-	if ((partial + len) > 63) {
+	if ((partial + len) >= SHA1_BLOCK_SIZE) {
 		u32 temp[SHA_WORKSPACE_WORDS];
 
 		if (partial) {
-			done = -partial;
-			memcpy(sctx->buffer + partial, data, done + 64);
-			src = sctx->buffer;
-		}
-
-		do {
-			sha_transform(sctx->state, src, temp);
-			done += 64;
-			src = data + done;
-		} while (done + 63 < len);
+			unsigned int done = SHA1_BLOCK_SIZE - partial;
 
+			memcpy(sctx->buffer + partial, data, done);
+			sha_transform(sctx->state, sctx->buffer, temp);
+			len -= done;
+			data += done;
+			partial = 0;
+		}
+		while (len >= SHA1_BLOCK_SIZE) {
+			sha_transform(sctx->state, data, temp);
+			len -= SHA1_BLOCK_SIZE;
+			data += SHA1_BLOCK_SIZE;
+		}
 		memset(temp, 0, sizeof(temp));
-		partial = 0;
 	}
-	memcpy(sctx->buffer + partial, src, len - done);
+	memcpy(sctx->buffer + partial, data, len);
 
 	return 0;
 }
Signature · 1 line
1.7.3.1
#4 lore raw
Quoted 7 lines · Mandeep Singh Baines
Quoted 5 lines · David Miller

The temp[] buffer is explicitly places inside the inner most basic block so that the compiler doesn't allocate the stack space unless that code path is taken.

Quoted 5 lines

Fixed in V2 (this patch). Thanks for the review.

-- >8 -- (snip)

Plus some other minor cleanup.

I don't really like the cleanups. In any case, mixing up the use of SHA1_BLOCK_SIZE with cleanups increases the chance of introducing a bug.

So please redo the patch with only the addition of SHA1_BLOCK_SIZE and nothing else.

Thanks,

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#5 lore raw

PATCH v3 1/1 crypto: sha1: use SHA1_BLOCK_SIZE

Modify sha1_update to use SHA1_BLOCK_SIZE.

Signed-off-by Mandeep Singh Baines <[email protected]>
---
 crypto/sha1_generic.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)
crypto/sha1_generic.c +6 -5
index 0416091..00ae60e 100644
@@ -43,25 +43,26 @@ static int sha1_update(struct shash_desc *desc, const u8 *data,
 	unsigned int partial, done;
 	const u8 *src;
 
-	partial = sctx->count & 0x3f;
+	partial = sctx->count % SHA1_BLOCK_SIZE;
 	sctx->count += len;
 	done = 0;
 	src = data;
 
-	if ((partial + len) > 63) {
+	if ((partial + len) >= SHA1_BLOCK_SIZE) {
 		u32 temp[SHA_WORKSPACE_WORDS];
 
 		if (partial) {
 			done = -partial;
-			memcpy(sctx->buffer + partial, data, done + 64);
+			memcpy(sctx->buffer + partial, data,
+			       done + SHA1_BLOCK_SIZE);
 			src = sctx->buffer;
 		}
 
 		do {
 			sha_transform(sctx->state, src, temp);
-			done += 64;
+			done += SHA1_BLOCK_SIZE;
 			src = data + done;
-		} while (done + 63 < len);
+		} while (done + SHA1_BLOCK_SIZE <= len);
 
 		memset(temp, 0, sizeof(temp));
 		partial = 0;
Signature · 1 line
1.7.3.1
#6 lore raw

Re: [PATCH v3] crypto: sha1: use SHA1_BLOCK_SIZE

Quoted 3 lines · Mandeep Singh Baines

Modify sha1_update to use SHA1_BLOCK_SIZE.

Signed-off-by Mandeep Singh Baines <[email protected]>

Patch applied. Thanks a lot!

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt