The following series fixes the CAAM hash driver, allowing it to work
with the previously merged "crypto: ahash - ensure statesize is non-
zero" patch.
This is non-trivial, because CAAM exports a huge 1600 bytes of data,
which, if we set .statesize to this, still results in the core code
rejecting the driver. So, we need to shrink the amount of state
exported.
The first, most obvious one to get rid of is the export of the
caam_hash_ctx structure, which is shared between the socket being
exported from and imported to - copying it away and back again was
a complete no-op.
The second is that we don't need to export both pending-bytes buffers.
Only one will be in use at any time.
A problem was encountered while testing, where the size of the pending
bytes buffer was not added to the scatterlist with the correct length.
This is also fixed in this series, by patch 3.
drivers/crypto/caam/caamhash.c | 79 +++++++++++++++++++++++++++++++-----------
1 file changed, 59 insertions(+), 20 deletions(-)
--
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
Print the errno code when hash registration fails, so we know why the
failure occurred. This aids debugging.
Signed-off-by: Russell King <[email protected]>
---
drivers/crypto/caam/caamhash.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/crypto/caam/caamhash.c b/drivers/crypto/caam/caamhash.c
index 94433b9fc200..2faf71ccbd43 100644
--- a/drivers/crypto/caam/caamhash.c
+++ b/drivers/crypto/caam/caamhash.c
@@ -1952,8 +1952,9 @@ static int __init caam_algapi_hash_init(void)
err = crypto_register_ahash(&t_alg->ahash_alg);
if (err) {
- pr_warn("%s alg registration failed\n",
- t_alg->ahash_alg.halg.base.cra_driver_name);
+ pr_warn("%s alg registration failed: %d\n",
+ t_alg->ahash_alg.halg.base.cra_driver_name,
+ err);
kfree(t_alg);
} else
list_add_tail(&t_alg->entry, &hash_list);
@@ -1968,8 +1969,9 @@ static int __init caam_algapi_hash_init(void)
err = crypto_register_ahash(&t_alg->ahash_alg);
if (err) {
- pr_warn("%s alg registration failed\n",
- t_alg->ahash_alg.halg.base.cra_driver_name);
+ pr_warn("%s alg registration failed: %d\n",
+ t_alg->ahash_alg.halg.base.cra_driver_name,
+ err);
kfree(t_alg);
} else
list_add_tail(&t_alg->entry, &hash_list);
--
2.1.0
When exporting and importing the hash state, we will only export and
import into hashes which share the same struct crypto_ahash pointer.
(See hash_accept->af_alg_accept->hash_accept_parent.)
This means that saving the caam_hash_ctx structure on export, and
restoring it on import is a waste of resources. So, remove this code.
Signed-off-by: Russell King <[email protected]>
---
drivers/crypto/caam/caamhash.c | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/crypto/caam/caamhash.c b/drivers/crypto/caam/caamhash.c
index 2faf71ccbd43..3ce6083c2e43 100644
--- a/drivers/crypto/caam/caamhash.c
+++ b/drivers/crypto/caam/caamhash.c
@@ -1575,24 +1575,20 @@ static int ahash_final(struct ahash_request *req)
static int ahash_export(struct ahash_request *req, void *out)
{
struct crypto_ahash *ahash = crypto_ahash_reqtfm(req);
- struct caam_hash_ctx *ctx = crypto_ahash_ctx(ahash);
struct caam_hash_state *state = ahash_request_ctx(req);
- memcpy(out, ctx, sizeof(struct caam_hash_ctx));
- memcpy(out + sizeof(struct caam_hash_ctx), state,
- sizeof(struct caam_hash_state));
+ memcpy(out, state, sizeof(struct caam_hash_state));
+
return 0;
}
static int ahash_import(struct ahash_request *req, const void *in)
{
struct crypto_ahash *ahash = crypto_ahash_reqtfm(req);
- struct caam_hash_ctx *ctx = crypto_ahash_ctx(ahash);
struct caam_hash_state *state = ahash_request_ctx(req);
- memcpy(ctx, in, sizeof(struct caam_hash_ctx));
- memcpy(state, in + sizeof(struct caam_hash_ctx),
- sizeof(struct caam_hash_state));
+ memcpy(state, in, sizeof(struct caam_hash_state));
+
return 0;
}
--
2.1.0
caam does not properly calculate the size of the retained state
when non-block aligned hashes are requested - it uses the wrong
buffer sizes, which results in errors such as:
caam_jr 2102000.jr1: 40000501: DECO: desc idx 5: SGT Length Error. The descriptor is trying to read more data than is contained in the SGT table.
We end up here with:
in_len 0x46 blocksize 0x40 last_bufsize 0x0 next_bufsize 0x6
to_hash 0x40 ctx_len 0x28 nbytes 0x20
which results in a job descriptor of:
jobdesc@889: ed03d918: b0861c08 3daa0080 f1400000 3d03d938
jobdesc@889: ed03d928: 00000068 f8400000 3cde2a40 00000028
where the word at 0xed03d928 is the expected data size (0x68), and a
scatterlist containing:
sg@892: ed03d938: 00000000 3cde2a40 00000028 00000000
sg@892: ed03d948: 00000000 3d03d100 00000006 00000000
sg@892: ed03d958: 00000000 7e8aa700 40000020 00000000
0x68 comes from 0x28 (the context size) plus the "in_len" rounded down
to a block size (0x40). in_len comes from 0x26 bytes of unhashed data
from the previous operation, plus the 0x20 bytes from the latest
operation.
The fixed version would create:
sg@892: ed03d938: 00000000 3cde2a40 00000028 00000000
sg@892: ed03d948: 00000000 3d03d100 00000026 00000000
sg@892: ed03d958: 00000000 7e8aa700 40000020 00000000
which replaces the 0x06 length with the correct 0x26 bytes of previously
unhashed data.
Signed-off-by: Russell King <[email protected]>
---
drivers/crypto/caam/caamhash.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/crypto/caam/caamhash.c b/drivers/crypto/caam/caamhash.c
index 3ce6083c2e43..dcee360065f3 100644
--- a/drivers/crypto/caam/caamhash.c
+++ b/drivers/crypto/caam/caamhash.c
@@ -829,7 +829,7 @@ static int ahash_update_ctx(struct ahash_request *req)
state->buf_dma = try_buf_map_to_sec4_sg(jrdev,
edesc->sec4_sg + 1,
buf, state->buf_dma,
- *next_buflen, *buflen);
+ *buflen, last_buflen);
if (src_nents) {
src_map_to_sec4_sg(jrdev, req->src, src_nents,
--
2.1.0
The kernel's coding style suggests that closing braces for initialisers
should not be aligned to the open brace column. The CodingStyle doc
shows how this should be done. Remove the additional tab.
Signed-off-by: Russell King <[email protected]>
---
drivers/crypto/caam/caamhash.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/drivers/crypto/caam/caamhash.c b/drivers/crypto/caam/caamhash.c
index 7dd80b0b3f51..12b455948dbc 100644
--- a/drivers/crypto/caam/caamhash.c
+++ b/drivers/crypto/caam/caamhash.c
@@ -1652,9 +1652,14 @@ static struct caam_hash_template driver_hash[] = {
.setkey = ahash_setkey,
.halg = {
.digestsize = SHA1_DIGEST_SIZE,
+<<<<<<< HEAD
.statesize = sizeof(struct caam_export_state),
},
+=======
+ .statesize = sizeof(struct caam_hash_state),
+>>>>>>> crypto: caam: fix indentation of close braces
},
+ },
.alg_type = OP_ALG_ALGSEL_SHA1,
.alg_op = OP_ALG_ALGSEL_SHA1 | OP_ALG_AAI_HMAC,
}, {
@@ -1675,8 +1680,8 @@ static struct caam_hash_template driver_hash[] = {
.halg = {
.digestsize = SHA224_DIGEST_SIZE,
.statesize = sizeof(struct caam_export_state),
- },
},
+ },
.alg_type = OP_ALG_ALGSEL_SHA224,
.alg_op = OP_ALG_ALGSEL_SHA224 | OP_ALG_AAI_HMAC,
}, {
@@ -1697,8 +1702,8 @@ static struct caam_hash_template driver_hash[] = {
.halg = {
.digestsize = SHA256_DIGEST_SIZE,
.statesize = sizeof(struct caam_export_state),
- },
},
+ },
.alg_type = OP_ALG_ALGSEL_SHA256,
.alg_op = OP_ALG_ALGSEL_SHA256 | OP_ALG_AAI_HMAC,
}, {
@@ -1719,8 +1724,8 @@ static struct caam_hash_template driver_hash[] = {
.halg = {
.digestsize = SHA384_DIGEST_SIZE,
.statesize = sizeof(struct caam_export_state),
- },
},
+ },
.alg_type = OP_ALG_ALGSEL_SHA384,
.alg_op = OP_ALG_ALGSEL_SHA384 | OP_ALG_AAI_HMAC,
}, {
@@ -1741,8 +1746,8 @@ static struct caam_hash_template driver_hash[] = {
.halg = {
.digestsize = SHA512_DIGEST_SIZE,
.statesize = sizeof(struct caam_export_state),
- },
},
+ },
.alg_type = OP_ALG_ALGSEL_SHA512,
.alg_op = OP_ALG_ALGSEL_SHA512 | OP_ALG_AAI_HMAC,
}, {
@@ -1763,8 +1768,8 @@ static struct caam_hash_template driver_hash[] = {
.halg = {
.digestsize = MD5_DIGEST_SIZE,
.statesize = sizeof(struct caam_export_state),
- },
},
+ },
.alg_type = OP_ALG_ALGSEL_MD5,
.alg_op = OP_ALG_ALGSEL_MD5 | OP_ALG_AAI_HMAC,
},
--
2.1.0
Avoid exporting lots of state by only exporting what we really require,
which is the buffer containing the set of pending bytes to be hashed,
number of pending bytes, the context buffer, and the function pointer
state. This reduces down the exported state size to 216 bytes from
576 bytes.
Signed-off-by: Russell King <[email protected]>
---
drivers/crypto/caam/caamhash.c | 44 ++++++++++++++++++++++++++++++++++++++----
1 file changed, 40 insertions(+), 4 deletions(-)
diff --git a/drivers/crypto/caam/caamhash.c b/drivers/crypto/caam/caamhash.c
index dcee360065f3..7dd80b0b3f51 100644
--- a/drivers/crypto/caam/caamhash.c
+++ b/drivers/crypto/caam/caamhash.c
@@ -134,6 +134,15 @@ struct caam_hash_state {
int current_buf;
};
+struct caam_export_state {
+ u8 buf[CAAM_MAX_HASH_BLOCK_SIZE];
+ u8 caam_ctx[MAX_CTX_LEN];
+ int buflen;
+ int (*update)(struct ahash_request *req);
+ int (*final)(struct ahash_request *req);
+ int (*finup)(struct ahash_request *req);
+};
+
/* Common job descriptor seq in/out ptr routines */
/* Map state->caam_ctx, and append seq_out_ptr command that points to it */
@@ -1574,20 +1583,41 @@ static int ahash_final(struct ahash_request *req)
static int ahash_export(struct ahash_request *req, void *out)
{
- struct crypto_ahash *ahash = crypto_ahash_reqtfm(req);
struct caam_hash_state *state = ahash_request_ctx(req);
+ struct caam_export_state *export = out;
+ int len;
+ u8 *buf;
- memcpy(out, state, sizeof(struct caam_hash_state));
+ if (state->current_buf) {
+ buf = state->buf_1;
+ len = state->buflen_1;
+ } else {
+ buf = state->buf_0;
+ len = state->buflen_1;
+ }
+
+ memcpy(export->buf, buf, len);
+ memcpy(export->caam_ctx, state->caam_ctx, sizeof(export->caam_ctx));
+ export->buflen = len;
+ export->update = state->update;
+ export->final = state->final;
+ export->finup = state->finup;
return 0;
}
static int ahash_import(struct ahash_request *req, const void *in)
{
- struct crypto_ahash *ahash = crypto_ahash_reqtfm(req);
struct caam_hash_state *state = ahash_request_ctx(req);
+ const struct caam_export_state *export = in;
- memcpy(state, in, sizeof(struct caam_hash_state));
+ memset(state, 0, sizeof(*state));
+ memcpy(state->buf_0, export->buf, export->buflen);
+ memcpy(state->caam_ctx, export->caam_ctx, sizeof(state->caam_ctx));
+ state->buflen_0 = export->buflen;
+ state->update = export->update;
+ state->final = export->final;
+ state->finup = export->finup;
return 0;
}
@@ -1622,6 +1652,7 @@ static struct caam_hash_template driver_hash[] = {
.setkey = ahash_setkey,
.halg = {
.digestsize = SHA1_DIGEST_SIZE,
+ .statesize = sizeof(struct caam_export_state),
},
},
.alg_type = OP_ALG_ALGSEL_SHA1,
@@ -1643,6 +1674,7 @@ static struct caam_hash_template driver_hash[] = {
.setkey = ahash_setkey,
.halg = {
.digestsize = SHA224_DIGEST_SIZE,
+ .statesize = sizeof(struct caam_export_state),
},
},
.alg_type = OP_ALG_ALGSEL_SHA224,
@@ -1664,6 +1696,7 @@ static struct caam_hash_template driver_hash[] = {
.setkey = ahash_setkey,
.halg = {
.digestsize = SHA256_DIGEST_SIZE,
+ .statesize = sizeof(struct caam_export_state),
},
},
.alg_type = OP_ALG_ALGSEL_SHA256,
@@ -1685,6 +1718,7 @@ static struct caam_hash_template driver_hash[] = {
.setkey = ahash_setkey,
.halg = {
.digestsize = SHA384_DIGEST_SIZE,
+ .statesize = sizeof(struct caam_export_state),
},
},
.alg_type = OP_ALG_ALGSEL_SHA384,
@@ -1706,6 +1740,7 @@ static struct caam_hash_template driver_hash[] = {
.setkey = ahash_setkey,
.halg = {
.digestsize = SHA512_DIGEST_SIZE,
+ .statesize = sizeof(struct caam_export_state),
},
},
.alg_type = OP_ALG_ALGSEL_SHA512,
@@ -1727,6 +1762,7 @@ static struct caam_hash_template driver_hash[] = {
.setkey = ahash_setkey,
.halg = {
.digestsize = MD5_DIGEST_SIZE,
+ .statesize = sizeof(struct caam_export_state),
},
},
.alg_type = OP_ALG_ALGSEL_MD5,
--
2.1.0
On Sat, Oct 17, 2015 at 07:50:55PM +0100, Russell King - ARM Linux wrote:
> The following series fixes the CAAM hash driver, allowing it to work
> with the previously merged "crypto: ahash - ensure statesize is non-
> zero" patch.
>
> This is non-trivial, because CAAM exports a huge 1600 bytes of data,
> which, if we set .statesize to this, still results in the core code
> rejecting the driver. So, we need to shrink the amount of state
> exported.
>
> The first, most obvious one to get rid of is the export of the
> caam_hash_ctx structure, which is shared between the socket being
> exported from and imported to - copying it away and back again was
> a complete no-op.
>
> The second is that we don't need to export both pending-bytes buffers.
> Only one will be in use at any time.
>
> A problem was encountered while testing, where the size of the pending
> bytes buffer was not added to the scatterlist with the correct length.
> This is also fixed in this series, by patch 3.
>
> drivers/crypto/caam/caamhash.c | 79 +++++++++++++++++++++++++++++++-----------
> 1 file changed, 59 insertions(+), 20 deletions(-)
This series has been tested using openssl to validate the hashes against
md5sum, sha1sum, sha224sum, sha256sum and sha512sum programs of every
program in my /bin directory, which checks correct functioning of the
hashing.
It has also been tested with openssh, where the crypto hardware is used
via openssl by sshd, which makes use of accept() and hence tests the
export/import paths.
--
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
The following series fixes the CAAM hash driver, allowing it to work
with the previously merged "crypto: ahash - ensure statesize is non-
zero" patch.
This is non-trivial, because CAAM exports a huge 1600 bytes of data,
which, if we set .statesize to this, still results in the core code
rejecting the driver. So, we need to shrink the amount of state
exported.
The first, most obvious one to get rid of is the export of the
caam_hash_ctx structure, which is shared between the socket being
exported from and imported to - copying it away and back again was
a complete no-op.
The second is that we don't need to export both pending-bytes buffers.
Only one will be in use at any time.
A problem was encountered while testing, where the size of the pending
bytes buffer was not added to the scatterlist with the correct length.
This is also fixed in this series, by patch 3. This bug was introduced
by a prior commit trying to fix a tcrypt error. However, the change is
wrong, but I have to question whether the test was correct or not - the
backtrace contains a function "test_ahash_pnum" which doesn't seem to
exist in mainline, nor does it seem to exist in any previous mainline
kernel.
Version 2 of this series addresses a mismerge in patch 5 of the
original series, and adds further information to patch 3.
Further testing with tcrypt showed up a problem identified by the
DMA API debugging (different to the original one referred to in
patch 3) where we leak DMA mappings.
drivers/crypto/caam/caamhash.c | 101 +++++++++++++++++++++++++++++------------
1 file changed, 71 insertions(+), 30 deletions(-)
--
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
Print the errno code when hash registration fails, so we know why the
failure occurred. This aids debugging.
Signed-off-by: Russell King <[email protected]>
---
drivers/crypto/caam/caamhash.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/crypto/caam/caamhash.c b/drivers/crypto/caam/caamhash.c
index 94433b9fc200..2faf71ccbd43 100644
--- a/drivers/crypto/caam/caamhash.c
+++ b/drivers/crypto/caam/caamhash.c
@@ -1952,8 +1952,9 @@ static int __init caam_algapi_hash_init(void)
err = crypto_register_ahash(&t_alg->ahash_alg);
if (err) {
- pr_warn("%s alg registration failed\n",
- t_alg->ahash_alg.halg.base.cra_driver_name);
+ pr_warn("%s alg registration failed: %d\n",
+ t_alg->ahash_alg.halg.base.cra_driver_name,
+ err);
kfree(t_alg);
} else
list_add_tail(&t_alg->entry, &hash_list);
@@ -1968,8 +1969,9 @@ static int __init caam_algapi_hash_init(void)
err = crypto_register_ahash(&t_alg->ahash_alg);
if (err) {
- pr_warn("%s alg registration failed\n",
- t_alg->ahash_alg.halg.base.cra_driver_name);
+ pr_warn("%s alg registration failed: %d\n",
+ t_alg->ahash_alg.halg.base.cra_driver_name,
+ err);
kfree(t_alg);
} else
list_add_tail(&t_alg->entry, &hash_list);
--
2.1.0
When exporting and importing the hash state, we will only export and
import into hashes which share the same struct crypto_ahash pointer.
(See hash_accept->af_alg_accept->hash_accept_parent.)
This means that saving the caam_hash_ctx structure on export, and
restoring it on import is a waste of resources. So, remove this code.
Signed-off-by: Russell King <[email protected]>
---
drivers/crypto/caam/caamhash.c | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/crypto/caam/caamhash.c b/drivers/crypto/caam/caamhash.c
index 2faf71ccbd43..3ce6083c2e43 100644
--- a/drivers/crypto/caam/caamhash.c
+++ b/drivers/crypto/caam/caamhash.c
@@ -1575,24 +1575,20 @@ static int ahash_final(struct ahash_request *req)
static int ahash_export(struct ahash_request *req, void *out)
{
struct crypto_ahash *ahash = crypto_ahash_reqtfm(req);
- struct caam_hash_ctx *ctx = crypto_ahash_ctx(ahash);
struct caam_hash_state *state = ahash_request_ctx(req);
- memcpy(out, ctx, sizeof(struct caam_hash_ctx));
- memcpy(out + sizeof(struct caam_hash_ctx), state,
- sizeof(struct caam_hash_state));
+ memcpy(out, state, sizeof(struct caam_hash_state));
+
return 0;
}
static int ahash_import(struct ahash_request *req, const void *in)
{
struct crypto_ahash *ahash = crypto_ahash_reqtfm(req);
- struct caam_hash_ctx *ctx = crypto_ahash_ctx(ahash);
struct caam_hash_state *state = ahash_request_ctx(req);
- memcpy(ctx, in, sizeof(struct caam_hash_ctx));
- memcpy(state, in + sizeof(struct caam_hash_ctx),
- sizeof(struct caam_hash_state));
+ memcpy(state, in, sizeof(struct caam_hash_state));
+
return 0;
}
--
2.1.0
caam does not properly calculate the size of the retained state
when non-block aligned hashes are requested - it uses the wrong
buffer sizes, which results in errors such as:
caam_jr 2102000.jr1: 40000501: DECO: desc idx 5: SGT Length Error. The descriptor is trying to read more data than is contained in the SGT table.
We end up here with:
in_len 0x46 blocksize 0x40 last_bufsize 0x0 next_bufsize 0x6
to_hash 0x40 ctx_len 0x28 nbytes 0x20
which results in a job descriptor of:
jobdesc@889: ed03d918: b0861c08 3daa0080 f1400000 3d03d938
jobdesc@889: ed03d928: 00000068 f8400000 3cde2a40 00000028
where the word at 0xed03d928 is the expected data size (0x68), and a
scatterlist containing:
sg@892: ed03d938: 00000000 3cde2a40 00000028 00000000
sg@892: ed03d948: 00000000 3d03d100 00000006 00000000
sg@892: ed03d958: 00000000 7e8aa700 40000020 00000000
0x68 comes from 0x28 (the context size) plus the "in_len" rounded down
to a block size (0x40). in_len comes from 0x26 bytes of unhashed data
from the previous operation, plus the 0x20 bytes from the latest
operation.
The fixed version would create:
sg@892: ed03d938: 00000000 3cde2a40 00000028 00000000
sg@892: ed03d948: 00000000 3d03d100 00000026 00000000
sg@892: ed03d958: 00000000 7e8aa700 40000020 00000000
which replaces the 0x06 length with the correct 0x26 bytes of previously
unhashed data.
This fixes a previous commit which erroneously "fixed" this due to a
DMA-API bug report; that commit indicates that the bug was caused via a
test_ahash_pnum() function in the tcrypt module. No such function has
ever existed in the mainline kernel. Given that the change in this
commit has been tested with DMA API debug enabled and shows no issue,
I can only conclude that test_ahash_pnum() was triggering that bad
behaviour by CAAM.
Fixes: 7d5196aba3c8 ("crypto: caam - Correct DMA unmap size in ahash_update_ctx()")
Signed-off-by: Russell King <[email protected]>
---
drivers/crypto/caam/caamhash.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/crypto/caam/caamhash.c b/drivers/crypto/caam/caamhash.c
index 3ce6083c2e43..dcee360065f3 100644
--- a/drivers/crypto/caam/caamhash.c
+++ b/drivers/crypto/caam/caamhash.c
@@ -829,7 +829,7 @@ static int ahash_update_ctx(struct ahash_request *req)
state->buf_dma = try_buf_map_to_sec4_sg(jrdev,
edesc->sec4_sg + 1,
buf, state->buf_dma,
- *next_buflen, *buflen);
+ *buflen, last_buflen);
if (src_nents) {
src_map_to_sec4_sg(jrdev, req->src, src_nents,
--
2.1.0
Avoid exporting lots of state by only exporting what we really require,
which is the buffer containing the set of pending bytes to be hashed,
number of pending bytes, the context buffer, and the function pointer
state. This reduces down the exported state size to 216 bytes from
576 bytes.
Signed-off-by: Russell King <[email protected]>
---
drivers/crypto/caam/caamhash.c | 44 ++++++++++++++++++++++++++++++++++++++----
1 file changed, 40 insertions(+), 4 deletions(-)
diff --git a/drivers/crypto/caam/caamhash.c b/drivers/crypto/caam/caamhash.c
index dcee360065f3..7dd80b0b3f51 100644
--- a/drivers/crypto/caam/caamhash.c
+++ b/drivers/crypto/caam/caamhash.c
@@ -134,6 +134,15 @@ struct caam_hash_state {
int current_buf;
};
+struct caam_export_state {
+ u8 buf[CAAM_MAX_HASH_BLOCK_SIZE];
+ u8 caam_ctx[MAX_CTX_LEN];
+ int buflen;
+ int (*update)(struct ahash_request *req);
+ int (*final)(struct ahash_request *req);
+ int (*finup)(struct ahash_request *req);
+};
+
/* Common job descriptor seq in/out ptr routines */
/* Map state->caam_ctx, and append seq_out_ptr command that points to it */
@@ -1574,20 +1583,41 @@ static int ahash_final(struct ahash_request *req)
static int ahash_export(struct ahash_request *req, void *out)
{
- struct crypto_ahash *ahash = crypto_ahash_reqtfm(req);
struct caam_hash_state *state = ahash_request_ctx(req);
+ struct caam_export_state *export = out;
+ int len;
+ u8 *buf;
- memcpy(out, state, sizeof(struct caam_hash_state));
+ if (state->current_buf) {
+ buf = state->buf_1;
+ len = state->buflen_1;
+ } else {
+ buf = state->buf_0;
+ len = state->buflen_1;
+ }
+
+ memcpy(export->buf, buf, len);
+ memcpy(export->caam_ctx, state->caam_ctx, sizeof(export->caam_ctx));
+ export->buflen = len;
+ export->update = state->update;
+ export->final = state->final;
+ export->finup = state->finup;
return 0;
}
static int ahash_import(struct ahash_request *req, const void *in)
{
- struct crypto_ahash *ahash = crypto_ahash_reqtfm(req);
struct caam_hash_state *state = ahash_request_ctx(req);
+ const struct caam_export_state *export = in;
- memcpy(state, in, sizeof(struct caam_hash_state));
+ memset(state, 0, sizeof(*state));
+ memcpy(state->buf_0, export->buf, export->buflen);
+ memcpy(state->caam_ctx, export->caam_ctx, sizeof(state->caam_ctx));
+ state->buflen_0 = export->buflen;
+ state->update = export->update;
+ state->final = export->final;
+ state->finup = export->finup;
return 0;
}
@@ -1622,6 +1652,7 @@ static struct caam_hash_template driver_hash[] = {
.setkey = ahash_setkey,
.halg = {
.digestsize = SHA1_DIGEST_SIZE,
+ .statesize = sizeof(struct caam_export_state),
},
},
.alg_type = OP_ALG_ALGSEL_SHA1,
@@ -1643,6 +1674,7 @@ static struct caam_hash_template driver_hash[] = {
.setkey = ahash_setkey,
.halg = {
.digestsize = SHA224_DIGEST_SIZE,
+ .statesize = sizeof(struct caam_export_state),
},
},
.alg_type = OP_ALG_ALGSEL_SHA224,
@@ -1664,6 +1696,7 @@ static struct caam_hash_template driver_hash[] = {
.setkey = ahash_setkey,
.halg = {
.digestsize = SHA256_DIGEST_SIZE,
+ .statesize = sizeof(struct caam_export_state),
},
},
.alg_type = OP_ALG_ALGSEL_SHA256,
@@ -1685,6 +1718,7 @@ static struct caam_hash_template driver_hash[] = {
.setkey = ahash_setkey,
.halg = {
.digestsize = SHA384_DIGEST_SIZE,
+ .statesize = sizeof(struct caam_export_state),
},
},
.alg_type = OP_ALG_ALGSEL_SHA384,
@@ -1706,6 +1740,7 @@ static struct caam_hash_template driver_hash[] = {
.setkey = ahash_setkey,
.halg = {
.digestsize = SHA512_DIGEST_SIZE,
+ .statesize = sizeof(struct caam_export_state),
},
},
.alg_type = OP_ALG_ALGSEL_SHA512,
@@ -1727,6 +1762,7 @@ static struct caam_hash_template driver_hash[] = {
.setkey = ahash_setkey,
.halg = {
.digestsize = MD5_DIGEST_SIZE,
+ .statesize = sizeof(struct caam_export_state),
},
},
.alg_type = OP_ALG_ALGSEL_MD5,
--
2.1.0
The kernel's coding style suggests that closing braces for initialisers
should not be aligned to the open brace column. The CodingStyle doc
shows how this should be done. Remove the additional tab.
Signed-off-by: Russell King <[email protected]>
---
drivers/crypto/caam/caamhash.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/crypto/caam/caamhash.c b/drivers/crypto/caam/caamhash.c
index 7dd80b0b3f51..f30c93840bba 100644
--- a/drivers/crypto/caam/caamhash.c
+++ b/drivers/crypto/caam/caamhash.c
@@ -1653,8 +1653,8 @@ static struct caam_hash_template driver_hash[] = {
.halg = {
.digestsize = SHA1_DIGEST_SIZE,
.statesize = sizeof(struct caam_export_state),
- },
},
+ },
.alg_type = OP_ALG_ALGSEL_SHA1,
.alg_op = OP_ALG_ALGSEL_SHA1 | OP_ALG_AAI_HMAC,
}, {
@@ -1675,8 +1675,8 @@ static struct caam_hash_template driver_hash[] = {
.halg = {
.digestsize = SHA224_DIGEST_SIZE,
.statesize = sizeof(struct caam_export_state),
- },
},
+ },
.alg_type = OP_ALG_ALGSEL_SHA224,
.alg_op = OP_ALG_ALGSEL_SHA224 | OP_ALG_AAI_HMAC,
}, {
@@ -1697,8 +1697,8 @@ static struct caam_hash_template driver_hash[] = {
.halg = {
.digestsize = SHA256_DIGEST_SIZE,
.statesize = sizeof(struct caam_export_state),
- },
},
+ },
.alg_type = OP_ALG_ALGSEL_SHA256,
.alg_op = OP_ALG_ALGSEL_SHA256 | OP_ALG_AAI_HMAC,
}, {
@@ -1719,8 +1719,8 @@ static struct caam_hash_template driver_hash[] = {
.halg = {
.digestsize = SHA384_DIGEST_SIZE,
.statesize = sizeof(struct caam_export_state),
- },
},
+ },
.alg_type = OP_ALG_ALGSEL_SHA384,
.alg_op = OP_ALG_ALGSEL_SHA384 | OP_ALG_AAI_HMAC,
}, {
@@ -1741,8 +1741,8 @@ static struct caam_hash_template driver_hash[] = {
.halg = {
.digestsize = SHA512_DIGEST_SIZE,
.statesize = sizeof(struct caam_export_state),
- },
},
+ },
.alg_type = OP_ALG_ALGSEL_SHA512,
.alg_op = OP_ALG_ALGSEL_SHA512 | OP_ALG_AAI_HMAC,
}, {
@@ -1763,8 +1763,8 @@ static struct caam_hash_template driver_hash[] = {
.halg = {
.digestsize = MD5_DIGEST_SIZE,
.statesize = sizeof(struct caam_export_state),
- },
},
+ },
.alg_type = OP_ALG_ALGSEL_MD5,
.alg_op = OP_ALG_ALGSEL_MD5 | OP_ALG_AAI_HMAC,
},
--
2.1.0
caamhash contains this weird code:
src_nents = sg_count(req->src, req->nbytes, &chained);
dma_map_sg_chained(jrdev, req->src, src_nents ? : 1, DMA_TO_DEVICE,
chained);
...
edesc->src_nents = src_nents;
sg_count() returns zero when __sg_count() returns zero or one. This
is used to mean that we don't need to use a hardware scatterlist.
However, setting src_nents to zero causes problems when we unmap:
if (edesc->src_nents)
dma_unmap_sg_chained(dev, req->src, edesc->src_nents,
DMA_TO_DEVICE, edesc->chained);
as zero here means that we have no entries to unmap.
This can be fixed in two ways: either by writing the number of entries
that were requested of dma_map_sg_chained(), or by reworking the "no
SG required" case.
We adopt the re-work solution here - we replace sg_count() with
__sg_count(), so src_nents now contains the real number of scatterlist
entries, and we then change the test for using the hardware scatterlist
to src_nents > 1 rather than just non-zero.
This change passes my sshd, openssl tests hashing /bin and tcrypt
tests.
Signed-off-by: Russell King <[email protected]>
---
drivers/crypto/caam/caamhash.c | 25 ++++++++++++++++---------
1 file changed, 16 insertions(+), 9 deletions(-)
diff --git a/drivers/crypto/caam/caamhash.c b/drivers/crypto/caam/caamhash.c
index f30c93840bba..28434ad08cb4 100644
--- a/drivers/crypto/caam/caamhash.c
+++ b/drivers/crypto/caam/caamhash.c
@@ -1095,10 +1095,13 @@ static int ahash_digest(struct ahash_request *req)
u32 options;
int sh_len;
- src_nents = sg_count(req->src, req->nbytes, &chained);
- dma_map_sg_chained(jrdev, req->src, src_nents ? : 1, DMA_TO_DEVICE,
+ src_nents = __sg_count(req->src, req->nbytes, &chained);
+ dma_map_sg_chained(jrdev, req->src, src_nents, DMA_TO_DEVICE,
chained);
- sec4_sg_bytes = src_nents * sizeof(struct sec4_sg_entry);
+ if (src_nents > 1)
+ sec4_sg_bytes = src_nents * sizeof(struct sec4_sg_entry);
+ else
+ sec4_sg_bytes = 0;
/* allocate space for base edesc and hw desc commands, link tables */
edesc = kzalloc(sizeof(*edesc) + sec4_sg_bytes + DESC_JOB_IO_LEN,
@@ -1117,7 +1120,7 @@ static int ahash_digest(struct ahash_request *req)
desc = edesc->hw_desc;
init_job_desc_shared(desc, ptr, sh_len, HDR_SHARE_DEFER | HDR_REVERSE);
- if (src_nents) {
+ if (src_nents > 1) {
sg_to_sec4_sg_last(req->src, src_nents, edesc->sec4_sg, 0);
edesc->sec4_sg_dma = dma_map_single(jrdev, edesc->sec4_sg,
sec4_sg_bytes, DMA_TO_DEVICE);
@@ -1447,11 +1450,15 @@ static int ahash_update_first(struct ahash_request *req)
to_hash = req->nbytes - *next_buflen;
if (to_hash) {
- src_nents = sg_count(req->src, req->nbytes - (*next_buflen),
- &chained);
- dma_map_sg_chained(jrdev, req->src, src_nents ? : 1,
+ src_nents = __sg_count(req->src, req->nbytes - (*next_buflen),
+ &chained);
+ dma_map_sg_chained(jrdev, req->src, src_nents,
DMA_TO_DEVICE, chained);
- sec4_sg_bytes = src_nents * sizeof(struct sec4_sg_entry);
+ if (src_nents > 1)
+ sec4_sg_bytes = src_nents *
+ sizeof(struct sec4_sg_entry);
+ else
+ sec4_sg_bytes = 0;
/*
* allocate space for base edesc and hw desc commands,
@@ -1472,7 +1479,7 @@ static int ahash_update_first(struct ahash_request *req)
DESC_JOB_IO_LEN;
edesc->dst_dma = 0;
- if (src_nents) {
+ if (src_nents > 1) {
sg_to_sec4_sg_last(req->src, src_nents,
edesc->sec4_sg, 0);
edesc->sec4_sg_dma = dma_map_single(jrdev,
--
2.1.0
On Sun, Oct 18, 2015 at 05:50:47PM +0100, Russell King - ARM Linux wrote:
> The following series fixes the CAAM hash driver, allowing it to work
> with the previously merged "crypto: ahash - ensure statesize is non-
> zero" patch.
>
> This is non-trivial, because CAAM exports a huge 1600 bytes of data,
> which, if we set .statesize to this, still results in the core code
> rejecting the driver. So, we need to shrink the amount of state
> exported.
>
> The first, most obvious one to get rid of is the export of the
> caam_hash_ctx structure, which is shared between the socket being
> exported from and imported to - copying it away and back again was
> a complete no-op.
>
> The second is that we don't need to export both pending-bytes buffers.
> Only one will be in use at any time.
>
> A problem was encountered while testing, where the size of the pending
> bytes buffer was not added to the scatterlist with the correct length.
> This is also fixed in this series, by patch 3. This bug was introduced
> by a prior commit trying to fix a tcrypt error. However, the change is
> wrong, but I have to question whether the test was correct or not - the
> backtrace contains a function "test_ahash_pnum" which doesn't seem to
> exist in mainline, nor does it seem to exist in any previous mainline
> kernel.
>
> Version 2 of this series addresses a mismerge in patch 5 of the
> original series, and adds further information to patch 3.
>
> Further testing with tcrypt showed up a problem identified by the
> DMA API debugging (different to the original one referred to in
> patch 3) where we leak DMA mappings.
Patches 1-5 applied. Patch 6 failed to apply because the cryptodev
tree already has a patch that converts caam to use dma_map_sg instead
of the caam-specific dma_map_sg_chained. If your patch is still
needed could you please rebase it on top of cryptodev?
commit 13fb8fd7a81923f7a64b4e688fe0bdaf1ea26adf
Author: LABBE Corentin <[email protected]>
Date: Wed Sep 23 13:55:27 2015 +0200
crypto: caam - dma_map_sg can handle chained SG
Thanks,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On Sun, 18 Oct 2015 17:50:47 +0100
Russell King - ARM Linux <[email protected]> wrote:
> The following series fixes the CAAM hash driver, allowing it to work
> with the previously merged "crypto: ahash - ensure statesize is non-
> zero" patch.
>
> This is non-trivial, because CAAM exports a huge 1600 bytes of data,
> which, if we set .statesize to this, still results in the core code
> rejecting the driver. So, we need to shrink the amount of state
> exported.
>
> The first, most obvious one to get rid of is the export of the
> caam_hash_ctx structure, which is shared between the socket being
> exported from and imported to - copying it away and back again was
> a complete no-op.
>
> The second is that we don't need to export both pending-bytes buffers.
> Only one will be in use at any time.
>
> A problem was encountered while testing, where the size of the pending
> bytes buffer was not added to the scatterlist with the correct length.
> This is also fixed in this series, by patch 3. This bug was introduced
> by a prior commit trying to fix a tcrypt error. However, the change is
> wrong, but I have to question whether the test was correct or not - the
> backtrace contains a function "test_ahash_pnum" which doesn't seem to
> exist in mainline, nor does it seem to exist in any previous mainline
> kernel.
>
> Version 2 of this series addresses a mismerge in patch 5 of the
> original series, and adds further information to patch 3.
>
> Further testing with tcrypt showed up a problem identified by the
> DMA API debugging (different to the original one referred to in
> patch 3) where we leak DMA mappings.
>
> drivers/crypto/caam/caamhash.c | 101 +++++++++++++++++++++++++++++------------
> 1 file changed, 71 insertions(+), 30 deletions(-)
>
> --
> FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
> according to speedtest.net.
Russell,
Thanks for putting these patches together. I tested the contents
successfully with OpenSSL/AF_ALG and i.MX6.
Tested-by: Victoria Milhoan <[email protected]>
--
Victoria Milhoan <[email protected]>