2016-02-08 15:26:47

by Cyrille Pitchen

[permalink] [raw]
Subject: [PATCH 0/2] crypto: atmel-sha - fix .import()/.export() implementation

This series of patches includes two fixes for the atmel-sha.c driver.

The first one fixes the implementation of the import/export hooks, which
did not pass the recent updates in testmgr.c (partial update exercise).
The new implementation now passes the tcrypt tests: tested with
next-20160208.

The second patch fixes a race condition if acquiring the hardware when it
is already busy.

Cyrille Pitchen (2):
crypto: atmel-sha - fix .import()/.export() implementation
crypto: atmel-sha - fix race in atmel_sha_final()

drivers/crypto/atmel-sha.c | 68 ++++++++--------------------------------------
1 file changed, 12 insertions(+), 56 deletions(-)

--
1.8.2.2


2016-02-08 15:26:48

by Cyrille Pitchen

[permalink] [raw]
Subject: [PATCH 1/2] crypto: atmel-sha - fix .import()/.export() implementation

Using only the digest, digcnt[], bufcnt and buffer[] fields of the
struct atmel_sha_reqctx was not enough to import/export the request state,
so now we use the whole structure.

Signed-off-by: Cyrille Pitchen <[email protected]>
---
drivers/crypto/atmel-sha.c | 44 ++++++++++----------------------------------
1 file changed, 10 insertions(+), 34 deletions(-)

diff --git a/drivers/crypto/atmel-sha.c b/drivers/crypto/atmel-sha.c
index f8407dc7dd38..7d27f96cdf02 100644
--- a/drivers/crypto/atmel-sha.c
+++ b/drivers/crypto/atmel-sha.c
@@ -81,16 +81,9 @@ struct atmel_sha_caps {
struct atmel_sha_dev;

/*
- * .statesize = sizeof(struct atmel_sha_state) must be <= PAGE_SIZE / 8 as
+ * .statesize = sizeof(struct atmel_sha_reqctx) must be <= PAGE_SIZE / 8 as
* tested by the ahash_prepare_alg() function.
*/
-struct atmel_sha_state {
- u8 digest[SHA512_DIGEST_SIZE];
- u8 buffer[SHA_BUFFER_LEN];
- u64 digcnt[2];
- size_t bufcnt;
-};
-
struct atmel_sha_reqctx {
struct atmel_sha_dev *dd;
unsigned long flags;
@@ -109,7 +102,7 @@ struct atmel_sha_reqctx {

size_t block_size;

- u8 buffer[0] __aligned(sizeof(u32));
+ u8 buffer[SHA_BUFFER_LEN + SHA512_BLOCK_SIZE] __aligned(sizeof(u32));
};

struct atmel_sha_ctx {
@@ -1048,40 +1041,23 @@ static int atmel_sha_digest(struct ahash_request *req)
static int atmel_sha_export(struct ahash_request *req, void *out)
{
const struct atmel_sha_reqctx *ctx = ahash_request_ctx(req);
- struct atmel_sha_state state;
-
- memcpy(state.digest, ctx->digest, SHA512_DIGEST_SIZE);
- memcpy(state.buffer, ctx->buffer, ctx->bufcnt);
- state.bufcnt = ctx->bufcnt;
- state.digcnt[0] = ctx->digcnt[0];
- state.digcnt[1] = ctx->digcnt[1];

- /* out might be unaligned. */
- memcpy(out, &state, sizeof(state));
+ memcpy(out, ctx, sizeof(*ctx));
return 0;
}

static int atmel_sha_import(struct ahash_request *req, const void *in)
{
struct atmel_sha_reqctx *ctx = ahash_request_ctx(req);
- struct atmel_sha_state state;
-
- /* in might be unaligned. */
- memcpy(&state, in, sizeof(state));

- memcpy(ctx->digest, state.digest, SHA512_DIGEST_SIZE);
- memcpy(ctx->buffer, state.buffer, state.bufcnt);
- ctx->bufcnt = state.bufcnt;
- ctx->digcnt[0] = state.digcnt[0];
- ctx->digcnt[1] = state.digcnt[1];
+ memcpy(ctx, in, sizeof(*ctx));
return 0;
}

static int atmel_sha_cra_init(struct crypto_tfm *tfm)
{
crypto_ahash_set_reqsize(__crypto_ahash_cast(tfm),
- sizeof(struct atmel_sha_reqctx) +
- SHA_BUFFER_LEN + SHA512_BLOCK_SIZE);
+ sizeof(struct atmel_sha_reqctx));

return 0;
}
@@ -1097,7 +1073,7 @@ static struct ahash_alg sha_1_256_algs[] = {
.import = atmel_sha_import,
.halg = {
.digestsize = SHA1_DIGEST_SIZE,
- .statesize = sizeof(struct atmel_sha_state),
+ .statesize = sizeof(struct atmel_sha_reqctx),
.base = {
.cra_name = "sha1",
.cra_driver_name = "atmel-sha1",
@@ -1121,7 +1097,7 @@ static struct ahash_alg sha_1_256_algs[] = {
.import = atmel_sha_import,
.halg = {
.digestsize = SHA256_DIGEST_SIZE,
- .statesize = sizeof(struct atmel_sha_state),
+ .statesize = sizeof(struct atmel_sha_reqctx),
.base = {
.cra_name = "sha256",
.cra_driver_name = "atmel-sha256",
@@ -1147,7 +1123,7 @@ static struct ahash_alg sha_224_alg = {
.import = atmel_sha_import,
.halg = {
.digestsize = SHA224_DIGEST_SIZE,
- .statesize = sizeof(struct atmel_sha_state),
+ .statesize = sizeof(struct atmel_sha_reqctx),
.base = {
.cra_name = "sha224",
.cra_driver_name = "atmel-sha224",
@@ -1173,7 +1149,7 @@ static struct ahash_alg sha_384_512_algs[] = {
.import = atmel_sha_import,
.halg = {
.digestsize = SHA384_DIGEST_SIZE,
- .statesize = sizeof(struct atmel_sha_state),
+ .statesize = sizeof(struct atmel_sha_reqctx),
.base = {
.cra_name = "sha384",
.cra_driver_name = "atmel-sha384",
@@ -1197,7 +1173,7 @@ static struct ahash_alg sha_384_512_algs[] = {
.import = atmel_sha_import,
.halg = {
.digestsize = SHA512_DIGEST_SIZE,
- .statesize = sizeof(struct atmel_sha_state),
+ .statesize = sizeof(struct atmel_sha_reqctx),
.base = {
.cra_name = "sha512",
.cra_driver_name = "atmel-sha512",
--
1.8.2.2

2016-02-08 15:26:49

by Cyrille Pitchen

[permalink] [raw]
Subject: [PATCH 2/2] crypto: atmel-sha - fix race in atmel_sha_final()

When (!ctx->bufcnt && !(ctx->flags & SHA_FLAGS_PAD)), the former source
code used to set the SHA_FLAGS_BUSY without checking whether this flag was
already set. If so, the hardware is already processing another hash
request so the processing of the req argument of atmel_sha_final() should
be delayed by queueing this request, the same way as done for the
(ctx->bufcnt != 0) case.

Signed-off-by: Cyrille Pitchen <[email protected]>
---
drivers/crypto/atmel-sha.c | 24 ++----------------------
1 file changed, 2 insertions(+), 22 deletions(-)

diff --git a/drivers/crypto/atmel-sha.c b/drivers/crypto/atmel-sha.c
index 7d27f96cdf02..596e489a4ca9 100644
--- a/drivers/crypto/atmel-sha.c
+++ b/drivers/crypto/atmel-sha.c
@@ -979,37 +979,17 @@ static int atmel_sha_update(struct ahash_request *req)
static int atmel_sha_final(struct ahash_request *req)
{
struct atmel_sha_reqctx *ctx = ahash_request_ctx(req);
- struct atmel_sha_ctx *tctx = crypto_tfm_ctx(req->base.tfm);
- struct atmel_sha_dev *dd = tctx->dd;
-
- int err = 0;

ctx->flags |= SHA_FLAGS_FINUP;

if (ctx->flags & SHA_FLAGS_ERROR)
return 0; /* uncompleted hash is not needed */

- if (ctx->bufcnt) {
- return atmel_sha_enqueue(req, SHA_OP_FINAL);
- } else if (!(ctx->flags & SHA_FLAGS_PAD)) { /* add padding */
- err = atmel_sha_hw_init(dd);
- if (err)
- goto err1;
-
- dd->req = req;
- dd->flags |= SHA_FLAGS_BUSY;
- err = atmel_sha_final_req(dd);
- } else {
+ if (ctx->flags & SHA_FLAGS_PAD)
/* copy ready hash (+ finalize hmac) */
return atmel_sha_finish(req);
- }
-
-err1:
- if (err != -EINPROGRESS)
- /* done_task will not finish it, so do it here */
- atmel_sha_finish_req(req, err);

- return err;
+ return atmel_sha_enqueue(req, SHA_OP_FINAL);
}

static int atmel_sha_finup(struct ahash_request *req)
--
1.8.2.2

2016-02-16 20:21:36

by Herbert Xu

[permalink] [raw]
Subject: Re: [PATCH 0/2] crypto: atmel-sha - fix .import()/.export() implementation

On Mon, Feb 08, 2016 at 04:26:47PM +0100, Cyrille Pitchen wrote:
> This series of patches includes two fixes for the atmel-sha.c driver.
>
> The first one fixes the implementation of the import/export hooks, which
> did not pass the recent updates in testmgr.c (partial update exercise).
> The new implementation now passes the tcrypt tests: tested with
> next-20160208.
>
> The second patch fixes a race condition if acquiring the hardware when it
> is already busy.

All applied.
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt