crypto: fixes for Marvell hash

If the algorithm passed a zero statesize, do not pass a valid pointer into the export/import functions. Passing a valid pointer covers up bugs in driver code which then go on to smash the kernel stack. Instead, pass NULL, which will cause any attempt to write to the pointer to fail.

Signed-off-by Russell King <[email protected]>

Quoted 7 lines · Russell King

If the algorithm passed a zero statesize, do not pass a valid pointer into the export/import functions. Passing a valid pointer covers up bugs in driver code which then go on to smash the kernel stack. Instead, pass NULL, which will cause any attempt to write to the pointer to fail.

Signed-off-by Russell King <[email protected]>

The state size should never be zero for a hash algorithm. Having a zero state means that the hash output must always be identical. Such an algorithm would be quite useless.

So how about adding a check upon hash registration to verify that the state size is greater than zero? The place to do it would be shash_prepare_alg and ahash_prepare_alg.

Do you mean something like this? As statesize is an unsigned int, testing for zero should be sufficient.

This small series of patches addresses oopses seen when trying to use the AF_ALG interface via openssl with openssh. This series does not address all problems, but merely stops the kernel from smashing its kernel stack and oopsing.

With these fixes in place, the kernel no longer oopses. However, with the digests enabled in openssl, openssh refuses to work, producing the following when attempting to connect to the target system:

Corrupted MAC on input.
Disconnecting: Packet corrupt

It's been hard enough to get this far; the crypto code is not the easiest code to debug for a new-comer due to the amount of state needed to be retained to understand the code (all the inline functions masking multiple levels of containerisation and pointer dereference does not make it easy to track what is stored where, and once I've been through one bit of code, I find I'm having to revisit the same piece of code a bit later to re-understand what it's doing.)

It's been difficult enough to find the engine plugin for openssl - the original git repo which hosted it is now dead
(http://src.carnivore.it/users/common/af_alg/). All that seems to be left is someone's modified version on github, which seems to get some maintanence. Debian doesn't seem to carry AF_ALG openssl support, and seems to only carry one package (strongswan) which supports this interface.

Hence, I'm leaving further debugging to other parties, especially as the userspace tooling for the AF_ALG seems rather lacking. (Are there any test programs, if so, can their location be documented and placed in Documentation/crypto please?)

I'm not sure who the maintainer for drivers/crypto/marvell is, so I've picked Thomas. It would be nice if there was an entry in MAINTAINERS for this driver.

The first patch in this series avoids kernel stack smashing if a crypto driver forgets to set the 'statesize' member, but writes to what seems to be a valid pointer passed to its export function. Of course, this won't completely stop stack smashing if the statesize member is smaller than the data which the export function writes. This patch is optional.

The second patch adds the necessary statesize members to the Marvell code which were previously missing. Fixing this uncovered a further problem, which the third patch addresses.

Several of the algorithms in marvell/hash.c have a statesize of zero. When an AF_ALG accept() on an already-accepted file descriptor to calls into hash_accept(), this causes:

	char state[crypto_ahash_statesize(crypto_ahash_reqtfm(req))];

to be zero-sized, but we still pass this to:

	err = crypto_ahash_export(req, state);

which proceeds to write to 'state' as if it was a "struct md5_state", "struct sha1_state" etc. Add the necessary initialisers for the .statesize member.

Signed-off-by Russell King <[email protected]>

---
 drivers/crypto/marvell/hash.c | 3 +++
 1 file changed, 3 insertions(+)

drivers/crypto/marvell/hash.c +3 -0

index e8d0d7128137..a259aced3b42 100644
@@ -870,6 +870,7 @@ struct ahash_alg mv_md5_alg = {
 	.import = mv_cesa_md5_import,
 	.halg = {
 		.digestsize = MD5_DIGEST_SIZE,
+		.statesize = sizeof(struct md5_state),
 		.base = {
 			.cra_name = "md5",
 			.cra_driver_name = "mv-md5",

@@ -959,6 +960,7 @@ struct ahash_alg mv_sha1_alg = {
 	.import = mv_cesa_sha1_import,
 	.halg = {
 		.digestsize = SHA1_DIGEST_SIZE,
+		.statesize = sizeof(struct sha1_state),
 		.base = {
 			.cra_name = "sha1",
 			.cra_driver_name = "mv-sha1",

@@ -1048,6 +1050,7 @@ struct ahash_alg mv_sha256_alg = {
 	.import = mv_cesa_sha256_import,
 	.halg = {
 		.digestsize = SHA256_DIGEST_SIZE,
+		.statesize = sizeof(struct sha256_state),
 		.base = {
 			.cra_name = "sha256",
 			.cra_driver_name = "mv-sha256",

When a AF_ALG fd is accepted a second time (hence hash_accept() is used), hash_accept_parent() allocates a new private context using sock_kmalloc(). This context is uninitialised. After use of the new fd, we eventually end up with the kernel complaining:

marvell-cesa f1090000.crypto: dma_pool_free cesa_padding, c0627770/0 (bad dma)

where c0627770 is a random address. Poisoning the memory allocated by the above sock_kmalloc() produces kernel oopses within the marvell hash code, particularly the interrupt handling.

The following simplfied call sequence occurs:

hash_accept()
crypto_ahash_export()
marvell hash export function
af_alg_accept()

    hash_accept_parent()	<== allocates uninitialised struct hash_ctx

crypto_ahash_import()
marvell hash import function

hash_ctx contains the struct mv_cesa_ahash_req in its req.__ctx member, and, as the marvell hash import function only partially initialises this structure, we end up with a lot of members which are left with whatever data was in memory prior to sock_kmalloc().

Add zero-initialisation of this structure.

Signed-off-by Russell King <[email protected]>

---
 drivers/crypto/marvell/hash.c | 6 ++++++
 1 file changed, 6 insertions(+)

drivers/crypto/marvell/hash.c +6 -0

index a259aced3b42..699839816505 100644
@@ -831,6 +831,8 @@ static int mv_cesa_md5_import(struct ahash_request *req, const void *in)
 	unsigned int cache_ptr;
 	int ret;
 
+	memset(creq, 0, sizeof(*creq));
+
 	creq->len = in_state->byte_count;
 	memcpy(creq->state, in_state->hash, digsize);
 	creq->cache_ptr = 0;

@@ -921,6 +923,8 @@ static int mv_cesa_sha1_import(struct ahash_request *req, const void *in)
 	unsigned int cache_ptr;
 	int ret;
 
+	memset(creq, 0, sizeof(*creq));
+
 	creq->len = in_state->count;
 	memcpy(creq->state, in_state->state, digsize);
 	creq->cache_ptr = 0;

@@ -1022,6 +1026,8 @@ static int mv_cesa_sha256_import(struct ahash_request *req, const void *in)
 	unsigned int cache_ptr;
 	int ret;
 
+	memset(creq, 0, sizeof(*creq));
+
 	creq->len = in_state->count;
 	memcpy(creq->state, in_state->state, digsize);
 	creq->cache_ptr = 0;

Version 2, same as version 1 but with a different patch 1, thanks to Herbert for an alternative approach on that one.

 crypto/ahash.c                | 3 ++-
 crypto/shash.c                | 3 ++-
 drivers/crypto/marvell/hash.c | 9 +++++++++
 3 files changed, 13 insertions(+), 2 deletions(-)

Quoted 47 lines · Russell King - ARM Linux

This small series of patches addresses oopses seen when trying to use the AF_ALG interface via openssl with openssh. This series does not address all problems, but merely stops the kernel from smashing its kernel stack and oopsing.

With these fixes in place, the kernel no longer oopses. However, with the digests enabled in openssl, openssh refuses to work, producing the following when attempting to connect to the target system:

Corrupted MAC on input.
Disconnecting: Packet corrupt

It's been hard enough to get this far; the crypto code is not the easiest code to debug for a new-comer due to the amount of state needed to be retained to understand the code (all the inline functions masking multiple levels of containerisation and pointer dereference does not make it easy to track what is stored where, and once I've been through one bit of code, I find I'm having to revisit the same piece of code a bit later to re-understand what it's doing.)

It's been difficult enough to find the engine plugin for openssl - the original git repo which hosted it is now dead
(http://src.carnivore.it/users/common/af_alg/). All that seems to be left is someone's modified version on github, which seems to get some maintanence. Debian doesn't seem to carry AF_ALG openssl support, and seems to only carry one package (strongswan) which supports this interface.

Hence, I'm leaving further debugging to other parties, especially as the userspace tooling for the AF_ALG seems rather lacking. (Are there any test programs, if so, can their location be documented and placed in Documentation/crypto please?)

I'm not sure who the maintainer for drivers/crypto/marvell is, so I've picked Thomas. It would be nice if there was an entry in MAINTAINERS for this driver.

The first patch in this series avoids kernel stack smashing if a crypto driver forgets to set the 'statesize' member, but writes to what seems to be a valid pointer passed to its export function. Of course, this won't completely stop stack smashing if the statesize member is smaller than the data which the export function writes. This patch is optional.

The second patch adds the necessary statesize members to the Marvell code which were previously missing. Fixing this uncovered a further problem, which the third patch addresses.

Signature · 2 lines

FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Version 2, same as version 1 but with a different patch 1, thanks to Herbert for an alternative approach on that one.

 crypto/ahash.c                | 3 ++-
 crypto/shash.c                | 3 ++-
 drivers/crypto/marvell/hash.c | 9 +++++++++
 3 files changed, 13 insertions(+), 2 deletions(-)

Quoted 19 lines · Russell King - ARM Linux

This small series of patches addresses oopses seen when trying to use the AF_ALG interface via openssl with openssh. This series does not address all problems, but merely stops the kernel from smashing its kernel stack and oopsing.

With these fixes in place, the kernel no longer oopses. However, with the digests enabled in openssl, openssh refuses to work, producing the following when attempting to connect to the target system:

Corrupted MAC on input.
Disconnecting: Packet corrupt

It's been hard enough to get this far; the crypto code is not the easiest code to debug for a new-comer due to the amount of state needed to be retained to understand the code (all the inline functions masking multiple levels of containerisation and pointer dereference does not make it easy to track what is stored where, and once I've been through one bit of code, I find I'm having to revisit the same piece of code a bit later to re-understand what it's doing.)

Quoted 9 lines

It's been difficult enough to find the engine plugin for openssl - the original git repo which hosted it is now dead
(http://src.carnivore.it/users/common/af_alg/). All that seems to be left is someone's modified version on github, which seems to get some maintanence. Debian doesn't seem to carry AF_ALG openssl support, and seems to only carry one package (strongswan) which supports this interface.

Quoted 8 lines

Hence, I'm leaving further debugging to other parties, especially as the userspace tooling for the AF_ALG seems rather lacking. (Are there any test programs, if so, can their location be documented and placed in Documentation/crypto please?)

I'm not sure who the maintainer for drivers/crypto/marvell is, so I've picked Thomas. It would be nice if there was an entry in MAINTAINERS for this driver.

Quoted 8 lines

The first patch in this series avoids kernel stack smashing if a crypto driver forgets to set the 'statesize' member, but writes to what seems to be a valid pointer passed to its export function. Of course, this won't completely stop stack smashing if the statesize member is smaller than the data which the export function writes. This patch is optional.

Quoted 3 lines

The second patch adds the necessary statesize members to the Marvell code which were previously missing. Fixing this uncovered a further problem, which the third patch addresses.

Version 2, same as version 1 but with a different patch 1, thanks to Herbert for an alternative approach on that one.

 crypto/ahash.c                | 3 ++-
 crypto/shash.c                | 3 ++-
 drivers/crypto/marvell/hash.c | 9 +++++++++
 3 files changed, 13 insertions(+), 2 deletions(-)

Quoted 47 lines · Russell King - ARM Linux

This small series of patches addresses oopses seen when trying to use the AF_ALG interface via openssl with openssh. This series does not address all problems, but merely stops the kernel from smashing its kernel stack and oopsing.

With these fixes in place, the kernel no longer oopses. However, with the digests enabled in openssl, openssh refuses to work, producing the following when attempting to connect to the target system:

Corrupted MAC on input.
Disconnecting: Packet corrupt

It's been hard enough to get this far; the crypto code is not the easiest code to debug for a new-comer due to the amount of state needed to be retained to understand the code (all the inline functions masking multiple levels of containerisation and pointer dereference does not make it easy to track what is stored where, and once I've been through one bit of code, I find I'm having to revisit the same piece of code a bit later to re-understand what it's doing.)

It's been difficult enough to find the engine plugin for openssl - the original git repo which hosted it is now dead
(http://src.carnivore.it/users/common/af_alg/). All that seems to be left is someone's modified version on github, which seems to get some maintanence. Debian doesn't seem to carry AF_ALG openssl support, and seems to only carry one package (strongswan) which supports this interface.

Hence, I'm leaving further debugging to other parties, especially as the userspace tooling for the AF_ALG seems rather lacking. (Are there any test programs, if so, can their location be documented and placed in Documentation/crypto please?)

I'm not sure who the maintainer for drivers/crypto/marvell is, so I've picked Thomas. It would be nice if there was an entry in MAINTAINERS for this driver.

The first patch in this series avoids kernel stack smashing if a crypto driver forgets to set the 'statesize' member, but writes to what seems to be a valid pointer passed to its export function. Of course, this won't completely stop stack smashing if the statesize member is smaller than the data which the export function writes. This patch is optional.

The second patch adds the necessary statesize members to the Marvell code which were previously missing. Fixing this uncovered a further problem, which the third patch addresses.

Signature · 2 lines

FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

If the algorithm passed a zero statesize, do not pass a valid pointer into the export/import functions. Passing a valid pointer covers up bugs in driver code which then go on to smash the kernel stack. Instead, pass NULL, which will cause any attempt to write to the pointer to fail.

Signed-off-by Russell King <[email protected]>

---
 crypto/ahash.c | 3 ++-
 crypto/shash.c | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

crypto/ahash.c +0 -0

index 8acb886032ae..9c1dc8d6106a 100644

crypto/ahash.c +0 -0

 		return -EINVAL;
 
 	base->cra_type = &crypto_ahash_type;

crypto/shash.c +2 -1

index ecb1e3d39bf0..ab3384b38542 100644
@@ -585,7 +585,8 @@ static int shash_prepare_alg(struct shash_alg *alg)
 
 	if (alg->digestsize > PAGE_SIZE / 8 ||
 	    alg->descsize > PAGE_SIZE / 8 ||
-	    alg->statesize > PAGE_SIZE / 8)
+	    alg->statesize > PAGE_SIZE / 8 ||
+	    alg->statesize == 0)
 		return -EINVAL;
 
 	base->cra_type = &crypto_shash_type;

Quoted 17 lines · On Fri, 09 Oct 2015 20:43:33 +0100 Russell King <[email protected]>…

If the algorithm passed a zero statesize, do not pass a valid pointer into the export/import functions. Passing a valid pointer covers up bugs in driver code which then go on to smash the kernel stack. Instead, pass NULL, which will cause any attempt to write to the pointer to fail.

Signed-off-by Russell King <[email protected]>

---
 crypto/ahash.c | 3 ++-
 crypto/shash.c | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

crypto/ahash.c +0 -0

index 8acb886032ae..9c1dc8d6106a 100644

crypto/ahash.c static int ahash_prepare_alg(struct ahash_alg *alg) · line 548

Quoted 6 lines

crypto/ahash.c +2 -1

@@ -544,7 +544,8 @@ static int ahash_prepare_alg(struct ahash_alg *alg)
 	struct crypto_alg *base = &alg->halg.base;
 
 	if (alg->halg.digestsize > PAGE_SIZE / 8 ||
-	    alg->halg.statesize > PAGE_SIZE / 8)
+	    alg->halg.statesize > PAGE_SIZE / 8 ||
+	    alg->halg.statesize == 0)

Just read Russel's answer to the cover letter, and I wonder if the following test wouldn't fix the problem:

	    (alg->halg.statesize == 0 && (alg->import || alg->export))

I mean, the only valid case where statesize can be zero is when you don't have any state associated to the crypto algorithm, and if that's the case, ->import() and ->export() functions are useless, isn't ?

If you're using AF_ALG, and you attach to (say) the ARM Neon SHA512 implementation through it, and then use accept() to duplicate it's state, what prevents the kernel from oopsing when hash_accept() calls crypto_ahash_export(), which then dereferences the NULL alg->export function pointer?

I mean, the only valid case where statesize can be zero is when you don't have any state associated to the crypto algorithm, and if that's the case, ->import() and ->export() functions are useless, isn't ?

If the algorithm passed a zero statesize, do not pass a valid pointer into the export/import functions. Passing a valid pointer covers up bugs in driver code which then go on to smash the kernel stack. Instead, pass NULL, which will cause any attempt to write to the pointer to fail.

Signed-off-by Russell King <[email protected]>

Quoted 7 lines · Russell King

If the algorithm passed a zero statesize, do not pass a valid pointer into the export/import functions. Passing a valid pointer covers up bugs in driver code which then go on to smash the kernel stack. Instead, pass NULL, which will cause any attempt to write to the pointer to fail.

Signed-off-by Russell King <[email protected]>

Patch applied without the shash hunk. I also replaced your commit message as it no longer makes any sense:

crypto: ahash - ensure statesize is non-zero

Unlike shash algorithms, ahash drivers must implement export and import as their descriptors may contain hardware state and cannot be exported as is. Unfortunately some ahash drivers did not provide them and end up causing crashes with algif_hash.

This patch adds a check to prevent these drivers from registering ahash algorithms until they are fixed.

Thanks,

The CAAM driver is similarly buggy - it has export/import functions in its ahash drivers, but zero statesize.

User exploitable kernel stack smashing... I'd suggest putting this patch into stable kernels as high priority, as I'm pretty sure this could be

Quoted 6 lines · Russell King - ARM Linux

The CAAM driver is similarly buggy - it has export/import functions in its ahash drivers, but zero statesize.

User exploitable kernel stack smashing... I'd suggest putting this patch into stable kernels as high priority, as I'm pretty sure this could be

I agree. It should already be on its way to stable as Linus has pulled it into his tree and it carries a stable cc.

I think the CAAM driver is pretty unfixable from a trivial point of view. This driver exports a huge amount of state - it contains both a struct caam_hash_ctx and a struct caam_hash_state, which totals up to 1600 bytes. This fails the:

Quoted 5 lines · Russell King - ARM Linux

I think the CAAM driver is pretty unfixable from a trivial point of view. This driver exports a huge amount of state - it contains both a struct caam_hash_ctx and a struct caam_hash_state, which totals up to 1600 bytes. This fails the:

Right just dumping the state out as is not going to work. This is not supposed to be how export works anyway. But it doesn't look too bad as most of that 1600 is consumed by the hardware program descriptor which can easily be regenerated upon import.

The only things that need to be exported AFAICS are key and buf_X.

Cheers,

Signature · 7 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 12 lines · On Thu, 15 Oct 2015 21:13:38 +0800 Herbert Xu <[email protected]>…

Quoted 5 lines · Russell King - ARM Linux

I think the CAAM driver is pretty unfixable from a trivial point of view. This driver exports a huge amount of state - it contains both a struct caam_hash_ctx and a struct caam_hash_state, which totals up to 1600 bytes. This fails the:

Right just dumping the state out as is not going to work. This is not supposed to be how export works anyway. But it doesn't look too bad as most of that 1600 is consumed by the hardware program descriptor which can easily be regenerated upon import.

Quoted 1 line

The only things that need to be exported AFAICS are key and buf_X.

I just pushed out a patch for export/import functions in the CAAM driver. The patch has been through testing with OpenSSL and the AF_ALG plugin on the MX6.

When a AF_ALG fd is accepted a second time (hence hash_accept() is used), hash_accept_parent() allocates a new private context using sock_kmalloc(). This context is uninitialised. After use of the new fd, we eventually end up with the kernel complaining:

marvell-cesa f1090000.crypto: dma_pool_free cesa_padding, c0627770/0 (bad dma)

where c0627770 is a random address. Poisoning the memory allocated by the above sock_kmalloc() produces kernel oopses within the marvell hash code, particularly the interrupt handling.

The following simplfied call sequence occurs:

hash_accept()
crypto_ahash_export()
marvell hash export function
af_alg_accept()

    hash_accept_parent()	<== allocates uninitialised struct hash_ctx

crypto_ahash_import()
marvell hash import function

hash_ctx contains the struct mv_cesa_ahash_req in its req.__ctx member, and, as the marvell hash import function only partially initialises this structure, we end up with a lot of members which are left with whatever data was in memory prior to sock_kmalloc().

Add zero-initialisation of this structure.

Signed-off-by Russell King <[email protected]>

---
 drivers/crypto/marvell/hash.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

drivers/crypto/marvell/hash.c +12 -0

index a259aced3b42..458867ce9515 100644
@@ -831,6 +831,10 @@ static int mv_cesa_md5_import(struct ahash_request *req, const void *in)
 	unsigned int cache_ptr;
 	int ret;
 
+	ret = crypto_ahash_init(req);
+	if (ret)
+		return ret;
+
 	creq->len = in_state->byte_count;
 	memcpy(creq->state, in_state->hash, digsize);
 	creq->cache_ptr = 0;

@@ -921,6 +925,10 @@ static int mv_cesa_sha1_import(struct ahash_request *req, const void *in)
 	unsigned int cache_ptr;
 	int ret;
 
+	ret = crypto_ahash_init(req);
+	if (ret)
+		return ret;
+
 	creq->len = in_state->count;
 	memcpy(creq->state, in_state->state, digsize);
 	creq->cache_ptr = 0;

@@ -1022,6 +1030,10 @@ static int mv_cesa_sha256_import(struct ahash_request *req, const void *in)
 	unsigned int cache_ptr;
 	int ret;
 
+	ret = crypto_ahash_init(req);
+	if (ret)
+		return ret;
+
 	creq->len = in_state->count;
 	memcpy(creq->state, in_state->state, digsize);
 	creq->cache_ptr = 0;

Hi Russel,

Quoted 26 lines · On Fri, 09 Oct 2015 20:43:43 +0100 Russell King <[email protected]>…

When a AF_ALG fd is accepted a second time (hence hash_accept() is used), hash_accept_parent() allocates a new private context using sock_kmalloc(). This context is uninitialised. After use of the new fd, we eventually end up with the kernel complaining:

marvell-cesa f1090000.crypto: dma_pool_free cesa_padding, c0627770/0 (bad dma)

where c0627770 is a random address. Poisoning the memory allocated by the above sock_kmalloc() produces kernel oopses within the marvell hash code, particularly the interrupt handling.

The following simplfied call sequence occurs:

hash_accept()
crypto_ahash_export()
marvell hash export function
af_alg_accept()

    hash_accept_parent()	<== allocates uninitialised struct hash_ctx

crypto_ahash_import()
marvell hash import function

hash_ctx contains the struct mv_cesa_ahash_req in its req.__ctx member, and, as the marvell hash import function only partially initialises this structure, we end up with a lot of members which are left with whatever data was in memory prior to sock_kmalloc().

Quoted 1 line

Add zero-initialisation of this structure.

Maybe you should also change your commit message since this patch no longer initializes the req struct to zero, otherwise

Acked-by Boris Brezillon <[email protected]>

Quoted 43 lines

Signed-off-by Russell King <[email protected]>

---
 drivers/crypto/marvell/hash.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

drivers/crypto/marvell/hash.c +12 -0

index a259aced3b42..458867ce9515 100644
@@ -831,6 +831,10 @@ static int mv_cesa_md5_import(struct ahash_request *req, const void *in)
 	unsigned int cache_ptr;
 	int ret;
 
+	ret = crypto_ahash_init(req);
+	if (ret)
+		return ret;
+
 	creq->len = in_state->byte_count;
 	memcpy(creq->state, in_state->hash, digsize);
 	creq->cache_ptr = 0;

@@ -921,6 +925,10 @@ static int mv_cesa_sha1_import(struct ahash_request *req, const void *in)
 	unsigned int cache_ptr;
 	int ret;
 
+	ret = crypto_ahash_init(req);
+	if (ret)
+		return ret;
+
 	creq->len = in_state->count;
 	memcpy(creq->state, in_state->state, digsize);
 	creq->cache_ptr = 0;

@@ -1022,6 +1030,10 @@ static int mv_cesa_sha256_import(struct ahash_request *req, const void *in)
 	unsigned int cache_ptr;
 	int ret;
 
+	ret = crypto_ahash_init(req);
+	if (ret)
+		return ret;
+
 	creq->len = in_state->count;
 	memcpy(creq->state, in_state->state, digsize);
 	creq->cache_ptr = 0;

Signature · 3 lines

Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Attempting to use the sha1 digest for openssh via openssl reveals that the result from the hash is wrong: this happens when we export the state from one socket and import it into another via calling accept().

The reason for this is because the operation is reset to "initial block" state, whereas we may be past the first fragment of data to be hashed.

Arrange for the operation code to avoid the initialisation of the state, thereby preserving the imported state.

Signed-off-by Russell King <[email protected]>

---
 drivers/crypto/marvell/hash.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

drivers/crypto/marvell/hash.c +15 -0

index 458867ce9515..b7c2c05f1a01 100644
@@ -835,6 +835,11 @@ static int mv_cesa_md5_import(struct ahash_request *req, const void *in)
 	if (ret)
 		return ret;
 
+	if (in_state->byte_count >= sizeof(in_state->block))
+		mv_cesa_update_op_cfg(&creq->op_tmpl,
+				      CESA_SA_DESC_CFG_MID_FRAG,
+				      CESA_SA_DESC_CFG_FRAG_MSK);
+
 	creq->len = in_state->byte_count;
 	memcpy(creq->state, in_state->hash, digsize);
 	creq->cache_ptr = 0;

@@ -929,6 +934,11 @@ static int mv_cesa_sha1_import(struct ahash_request *req, const void *in)
 	if (ret)
 		return ret;
 
+	if (in_state->count >= SHA1_BLOCK_SIZE)
+		mv_cesa_update_op_cfg(&creq->op_tmpl,
+				      CESA_SA_DESC_CFG_MID_FRAG,
+				      CESA_SA_DESC_CFG_FRAG_MSK);
+
 	creq->len = in_state->count;
 	memcpy(creq->state, in_state->state, digsize);
 	creq->cache_ptr = 0;

@@ -1034,6 +1044,11 @@ static int mv_cesa_sha256_import(struct ahash_request *req, const void *in)
 	if (ret)
 		return ret;
 
+	if (in_state->count >= SHA256_BLOCK_SIZE)
+		mv_cesa_update_op_cfg(&creq->op_tmpl,
+				      CESA_SA_DESC_CFG_MID_FRAG,
+				      CESA_SA_DESC_CFG_FRAG_MSK);
+
 	creq->len = in_state->count;
 	memcpy(creq->state, in_state->state, digsize);
 	creq->cache_ptr = 0;

As all the import functions are virtually identical, factor out their common parts into a generic mv_cesa_import(). This performs the actual import, and we pass the data pointers and current length into this function.

We have to switch a % const operation to do_div() to avoid provoking gcc to use the expensive 64-bit by 64-bit modulus operation.

Signed-off-by Russell King <[email protected]>

---
 drivers/crypto/marvell/hash.c | 88 +++++++++++--------------------------------
 1 file changed, 21 insertions(+), 67 deletions(-)

drivers/crypto/marvell/hash.c +21 -67

index b7c2c05f1a01..4f328b2a3a22 100644
@@ -822,12 +822,13 @@ static int mv_cesa_md5_export(struct ahash_request *req, void *out)
 	return 0;
 }
 
-static int mv_cesa_md5_import(struct ahash_request *req, const void *in)
+static int mv_cesa_import(struct ahash_request *req, const void *hash, u64 len,
+			  const void *cache)
 {
-	const struct md5_state *in_state = in;
 	struct crypto_ahash *ahash = crypto_ahash_reqtfm(req);
 	struct mv_cesa_ahash_req *creq = ahash_request_ctx(req);
 	unsigned int digsize = crypto_ahash_digestsize(ahash);
+	unsigned int blocksize;
 	unsigned int cache_ptr;
 	int ret;
 

@@ -835,16 +836,17 @@ static int mv_cesa_md5_import(struct ahash_request *req, const void *in)
 	if (ret)
 		return ret;
 
-	if (in_state->byte_count >= sizeof(in_state->block))
+	blocksize = crypto_tfm_alg_blocksize(crypto_ahash_tfm(ahash));
+	if (len >= blocksize)
 		mv_cesa_update_op_cfg(&creq->op_tmpl,
 				      CESA_SA_DESC_CFG_MID_FRAG,
 				      CESA_SA_DESC_CFG_FRAG_MSK);
 
-	creq->len = in_state->byte_count;
-	memcpy(creq->state, in_state->hash, digsize);
+	creq->len = len;
+	memcpy(creq->state, hash, digsize);
 	creq->cache_ptr = 0;
 
-	cache_ptr = creq->len % sizeof(in_state->block);
+	cache_ptr = do_div(len, blocksize);
 	if (!cache_ptr)
 		return 0;
 

@@ -852,12 +854,20 @@ static int mv_cesa_md5_import(struct ahash_request *req, const void *in)
 	if (ret)
 		return ret;
 
-	memcpy(creq->cache, in_state->block, cache_ptr);
+	memcpy(creq->cache, cache, cache_ptr);
 	creq->cache_ptr = cache_ptr;
 
 	return 0;
 }
 
+static int mv_cesa_md5_import(struct ahash_request *req, const void *in)
+{
+	const struct md5_state *in_state = in;
+
+	return mv_cesa_import(req, in_state->hash, in_state->byte_count,
+			      in_state->block);
+}
+
 static int mv_cesa_md5_digest(struct ahash_request *req)
 {
 	int ret;

@@ -924,37 +934,9 @@ static int mv_cesa_sha1_export(struct ahash_request *req, void *out)
 static int mv_cesa_sha1_import(struct ahash_request *req, const void *in)
 {
 	const struct sha1_state *in_state = in;
-	struct crypto_ahash *ahash = crypto_ahash_reqtfm(req);
-	struct mv_cesa_ahash_req *creq = ahash_request_ctx(req);
-	unsigned int digsize = crypto_ahash_digestsize(ahash);
-	unsigned int cache_ptr;
-	int ret;
-
-	ret = crypto_ahash_init(req);
-	if (ret)
-		return ret;
-
-	if (in_state->count >= SHA1_BLOCK_SIZE)
-		mv_cesa_update_op_cfg(&creq->op_tmpl,
-				      CESA_SA_DESC_CFG_MID_FRAG,
-				      CESA_SA_DESC_CFG_FRAG_MSK);
 
-	creq->len = in_state->count;
-	memcpy(creq->state, in_state->state, digsize);
-	creq->cache_ptr = 0;
-
-	cache_ptr = creq->len % SHA1_BLOCK_SIZE;
-	if (!cache_ptr)
-		return 0;
-
-	ret = mv_cesa_ahash_alloc_cache(req);
-	if (ret)
-		return ret;
-
-	memcpy(creq->cache, in_state->buffer, cache_ptr);
-	creq->cache_ptr = cache_ptr;
-
-	return 0;
+	return mv_cesa_import(req, in_state->state, in_state->count,
+			      in_state->buffer);
 }
 
 static int mv_cesa_sha1_digest(struct ahash_request *req)

@@ -1034,37 +1016,9 @@ static int mv_cesa_sha256_export(struct ahash_request *req, void *out)
 static int mv_cesa_sha256_import(struct ahash_request *req, const void *in)
 {
 	const struct sha256_state *in_state = in;
-	struct crypto_ahash *ahash = crypto_ahash_reqtfm(req);
-	struct mv_cesa_ahash_req *creq = ahash_request_ctx(req);
-	unsigned int digsize = crypto_ahash_digestsize(ahash);
-	unsigned int cache_ptr;
-	int ret;
 
-	ret = crypto_ahash_init(req);
-	if (ret)
-		return ret;
-
-	if (in_state->count >= SHA256_BLOCK_SIZE)
-		mv_cesa_update_op_cfg(&creq->op_tmpl,
-				      CESA_SA_DESC_CFG_MID_FRAG,
-				      CESA_SA_DESC_CFG_FRAG_MSK);
-
-	creq->len = in_state->count;
-	memcpy(creq->state, in_state->state, digsize);
-	creq->cache_ptr = 0;
-
-	cache_ptr = creq->len % SHA256_BLOCK_SIZE;
-	if (!cache_ptr)
-		return 0;
-
-	ret = mv_cesa_ahash_alloc_cache(req);
-	if (ret)
-		return ret;
-
-	memcpy(creq->cache, in_state->buf, cache_ptr);
-	creq->cache_ptr = cache_ptr;
-
-	return 0;
+	return mv_cesa_import(req, in_state->state, in_state->count,
+			      in_state->buf);
 }
 
 struct ahash_alg mv_sha256_alg = {

As all the import functions and export functions are virtually identical, factor out their common parts into a generic mv_cesa_ahash_import() and mv_cesa_ahash_export() respectively. This performs the actual import or export, and we pass the data pointers and length into these functions.

We have to switch a % const operation to do_div() in the common import function to avoid provoking gcc to use the expensive 64-bit by 64-bit modulus operation.

Signed-off-by Russell King <[email protected]>

---
Replacement to patch 5, going a little further, as requested by Boris.

 drivers/crypto/marvell/hash.c | 157 ++++++++++++++----------------------------
 1 file changed, 53 insertions(+), 104 deletions(-)

drivers/crypto/marvell/hash.c +53 -104

index b7c2c05f1a01..a24ceda7acfe 100644
@@ -795,39 +795,32 @@ static int mv_cesa_ahash_finup(struct ahash_request *req)
 	return ret;
 }
 
-static int mv_cesa_md5_init(struct ahash_request *req)
+static int mv_cesa_ahash_export(struct ahash_request *req, void *hash,
+				u64 *len, void *cache)
 {
-	struct mv_cesa_op_ctx tmpl;
-
-	mv_cesa_set_op_cfg(&tmpl, CESA_SA_DESC_CFG_MACM_MD5);
-
-	mv_cesa_ahash_init(req, &tmpl);
-
-	return 0;
-}
-
-static int mv_cesa_md5_export(struct ahash_request *req, void *out)
-{
-	struct md5_state *out_state = out;
 	struct crypto_ahash *ahash = crypto_ahash_reqtfm(req);
 	struct mv_cesa_ahash_req *creq = ahash_request_ctx(req);
 	unsigned int digsize = crypto_ahash_digestsize(ahash);
+	unsigned int blocksize;
+
+	blocksize = crypto_tfm_alg_blocksize(crypto_ahash_tfm(ahash));
 
-	out_state->byte_count = creq->len;
-	memcpy(out_state->hash, creq->state, digsize);
-	memset(out_state->block, 0, sizeof(out_state->block));
+	*len = creq->len;
+	memcpy(hash, creq->state, digsize);
+	memset(cache, 0, blocksize);
 	if (creq->cache)
-		memcpy(out_state->block, creq->cache, creq->cache_ptr);
+		memcpy(cache, creq->cache, creq->cache_ptr);
 
 	return 0;
 }
 
-static int mv_cesa_md5_import(struct ahash_request *req, const void *in)
+static int mv_cesa_ahash_import(struct ahash_request *req, const void *hash,
+				u64 len, const void *cache)
 {
-	const struct md5_state *in_state = in;
 	struct crypto_ahash *ahash = crypto_ahash_reqtfm(req);
 	struct mv_cesa_ahash_req *creq = ahash_request_ctx(req);
 	unsigned int digsize = crypto_ahash_digestsize(ahash);
+	unsigned int blocksize;
 	unsigned int cache_ptr;
 	int ret;
 

@@ -835,16 +828,17 @@ static int mv_cesa_md5_import(struct ahash_request *req, const void *in)
 	if (ret)
 		return ret;
 
-	if (in_state->byte_count >= sizeof(in_state->block))
+	blocksize = crypto_tfm_alg_blocksize(crypto_ahash_tfm(ahash));
+	if (len >= blocksize)
 		mv_cesa_update_op_cfg(&creq->op_tmpl,
 				      CESA_SA_DESC_CFG_MID_FRAG,
 				      CESA_SA_DESC_CFG_FRAG_MSK);
 
-	creq->len = in_state->byte_count;
-	memcpy(creq->state, in_state->hash, digsize);
+	creq->len = len;
+	memcpy(creq->state, hash, digsize);
 	creq->cache_ptr = 0;
 
-	cache_ptr = creq->len % sizeof(in_state->block);
+	cache_ptr = do_div(len, blocksize);
 	if (!cache_ptr)
 		return 0;
 

@@ -852,12 +846,39 @@ static int mv_cesa_md5_import(struct ahash_request *req, const void *in)
 	if (ret)
 		return ret;
 
-	memcpy(creq->cache, in_state->block, cache_ptr);
+	memcpy(creq->cache, cache, cache_ptr);
 	creq->cache_ptr = cache_ptr;
 
 	return 0;
 }
 
+static int mv_cesa_md5_init(struct ahash_request *req)
+{
+	struct mv_cesa_op_ctx tmpl;
+
+	mv_cesa_set_op_cfg(&tmpl, CESA_SA_DESC_CFG_MACM_MD5);
+
+	mv_cesa_ahash_init(req, &tmpl);
+
+	return 0;
+}
+
+static int mv_cesa_md5_export(struct ahash_request *req, void *out)
+{
+	struct md5_state *out_state = out;
+
+	return mv_cesa_ahash_export(req, out_state->hash,
+				    &out_state->byte_count, out_state->block);
+}
+
+static int mv_cesa_md5_import(struct ahash_request *req, const void *in)
+{
+	const struct md5_state *in_state = in;
+
+	return mv_cesa_ahash_import(req, in_state->hash, in_state->byte_count,
+				    in_state->block);
+}
+
 static int mv_cesa_md5_digest(struct ahash_request *req)
 {
 	int ret;

@@ -908,53 +929,17 @@ static int mv_cesa_sha1_init(struct ahash_request *req)
 static int mv_cesa_sha1_export(struct ahash_request *req, void *out)
 {
 	struct sha1_state *out_state = out;
-	struct crypto_ahash *ahash = crypto_ahash_reqtfm(req);
-	struct mv_cesa_ahash_req *creq = ahash_request_ctx(req);
-	unsigned int digsize = crypto_ahash_digestsize(ahash);
-
-	out_state->count = creq->len;
-	memcpy(out_state->state, creq->state, digsize);
-	memset(out_state->buffer, 0, sizeof(out_state->buffer));
-	if (creq->cache)
-		memcpy(out_state->buffer, creq->cache, creq->cache_ptr);
 
-	return 0;
+	return mv_cesa_ahash_export(req, out_state->state, &out_state->count,
+				    out_state->buffer);
 }
 
 static int mv_cesa_sha1_import(struct ahash_request *req, const void *in)
 {
 	const struct sha1_state *in_state = in;
-	struct crypto_ahash *ahash = crypto_ahash_reqtfm(req);
-	struct mv_cesa_ahash_req *creq = ahash_request_ctx(req);
-	unsigned int digsize = crypto_ahash_digestsize(ahash);
-	unsigned int cache_ptr;
-	int ret;
-
-	ret = crypto_ahash_init(req);
-	if (ret)
-		return ret;
-
-	if (in_state->count >= SHA1_BLOCK_SIZE)
-		mv_cesa_update_op_cfg(&creq->op_tmpl,
-				      CESA_SA_DESC_CFG_MID_FRAG,
-				      CESA_SA_DESC_CFG_FRAG_MSK);
-
-	creq->len = in_state->count;
-	memcpy(creq->state, in_state->state, digsize);
-	creq->cache_ptr = 0;
-
-	cache_ptr = creq->len % SHA1_BLOCK_SIZE;
-	if (!cache_ptr)
-		return 0;
-
-	ret = mv_cesa_ahash_alloc_cache(req);
-	if (ret)
-		return ret;
-
-	memcpy(creq->cache, in_state->buffer, cache_ptr);
-	creq->cache_ptr = cache_ptr;
 
-	return 0;
+	return mv_cesa_ahash_import(req, in_state->state, in_state->count,
+				    in_state->buffer);
 }
 
 static int mv_cesa_sha1_digest(struct ahash_request *req)

@@ -1018,53 +1003,17 @@ static int mv_cesa_sha256_digest(struct ahash_request *req)
 static int mv_cesa_sha256_export(struct ahash_request *req, void *out)
 {
 	struct sha256_state *out_state = out;
-	struct crypto_ahash *ahash = crypto_ahash_reqtfm(req);
-	struct mv_cesa_ahash_req *creq = ahash_request_ctx(req);
-	unsigned int ds = crypto_ahash_digestsize(ahash);
 
-	out_state->count = creq->len;
-	memcpy(out_state->state, creq->state, ds);
-	memset(out_state->buf, 0, sizeof(out_state->buf));
-	if (creq->cache)
-		memcpy(out_state->buf, creq->cache, creq->cache_ptr);
-
-	return 0;
+	return mv_cesa_ahash_export(req, out_state->state, &out_state->count,
+				    out_state->buf);
 }
 
 static int mv_cesa_sha256_import(struct ahash_request *req, const void *in)
 {
 	const struct sha256_state *in_state = in;
-	struct crypto_ahash *ahash = crypto_ahash_reqtfm(req);
-	struct mv_cesa_ahash_req *creq = ahash_request_ctx(req);
-	unsigned int digsize = crypto_ahash_digestsize(ahash);
-	unsigned int cache_ptr;
-	int ret;
-
-	ret = crypto_ahash_init(req);
-	if (ret)
-		return ret;
-
-	if (in_state->count >= SHA256_BLOCK_SIZE)
-		mv_cesa_update_op_cfg(&creq->op_tmpl,
-				      CESA_SA_DESC_CFG_MID_FRAG,
-				      CESA_SA_DESC_CFG_FRAG_MSK);
 
-	creq->len = in_state->count;
-	memcpy(creq->state, in_state->state, digsize);
-	creq->cache_ptr = 0;
-
-	cache_ptr = creq->len % SHA256_BLOCK_SIZE;
-	if (!cache_ptr)
-		return 0;
-
-	ret = mv_cesa_ahash_alloc_cache(req);
-	if (ret)
-		return ret;
-
-	memcpy(creq->cache, in_state->buf, cache_ptr);
-	creq->cache_ptr = cache_ptr;
-
-	return 0;
+	return mv_cesa_ahash_import(req, in_state->state, in_state->count,
+				    in_state->buf);
 }
 
 struct ahash_alg mv_sha256_alg = {

As all the import functions and export functions are virtually identical, factor out their common parts into a generic mv_cesa_ahash_import() and mv_cesa_ahash_export() respectively. This performs the actual import or export, and we pass the data pointers and length into these functions.

We have to switch a % const operation to do_div() in the common import function to avoid provoking gcc to use the expensive 64-bit by 64-bit modulus operation.

Signed-off-by Russell King <[email protected]>

Hi Russel,

Quoted 10 lines · Russell King

As all the import functions and export functions are virtually identical, factor out their common parts into a generic mv_cesa_ahash_import() and mv_cesa_ahash_export() respectively. This performs the actual import or export, and we pass the data pointers and length into these functions.

We have to switch a % const operation to do_div() in the common import function to avoid provoking gcc to use the expensive 64-bit by 64-bit modulus operation.

Quoted 1 line

Signed-off-by Russell King <[email protected]>

Thanks for the refactoring and for the fixes. All patches look good to me. Out of curiosity, can I ask what perf you get w/ openssh or openssl using AF_ALG and the CESA?

Quoted 14 lines · Arnaud Ebalard

Hi Russel,

Quoted 10 lines · Russell King

As all the import functions and export functions are virtually identical, factor out their common parts into a generic mv_cesa_ahash_import() and mv_cesa_ahash_export() respectively. This performs the actual import or export, and we pass the data pointers and length into these functions.

We have to switch a % const operation to do_div() in the common import function to avoid provoking gcc to use the expensive 64-bit by 64-bit modulus operation.

Quoted 5 lines

Quoted 1 line

Signed-off-by Russell King <[email protected]>

Thanks for the refactoring and for the fixes. All patches look good to me. Out of curiosity, can I ask what perf you get w/ openssh or openssl using AF_ALG and the CESA?

I would do, but it seems this AF_ALG plugin for openssl isn't actually using it for encryption. When I try:

openssl speed -engine af_alg aes-128-cbc

I get results for using openssl's software implementation. If I do:

openssl speed -engine af_alg md5

then I get results from using the kernel's MD5. Hence, I think the only thing that I think openssh is using it for is the digest stuff, not the crypto itself. I can't be certain about that though.

I've tried debugging the af_alg engine plugin, but I'm not getting very far (I'm not an openssl hacker!) I see it registering the function to get the ciphers (via ENGINE_set_ciphers), and I see this called several times, returning a list of NID_xxx values describing the methods it supports, which includes aes-128-cbc. However, unlike the equivalent digest function, I never see it called requesting any of the ciphers. Maybe it's an openssl bug, or a "feature" preventing hardware crypto? Maybe something is missing from its initialisation? I've no idea yet. It seems I'm not alone in this - this report from April 2015 is exactly what I'm seeing:

https://mta.openssl.org/pipermail/openssl-users/2015-April/001124.html

However, I'm coming to the conclusion that AF_ALG with openssl is a dead project, and the only interface that everyone is using for that is cryptodev - probably contary to Herbert and/or DaveM's wishes. For example, the openwrt guys seem to only support cryptodev, according to their wiki page on the subject of hardware crypto:

http://wiki.openwrt.org/doc/hardware/cryptographic.hardware.accelerators

Here's the references to code for AF_ALG with openssl I've found so far:

Original af_alg plugin (dead):

http://src.carnivore.it/users/common/af_alg/

3rd party "maintained" af_alg openssl plugin, derived from commit 1851bbb010c38878c83729be844f168192059189 in the above repo but with no history:

https://github.com/RidgeRun/af-alg-rr

and that doesn't contain any changes to the C code originally committed. Whether this C code contains changes or not is anyone's guess: there's no way to refer back to the original repository.

Anyway, here's the digest results:

Software:
The 'numbers' are in 1000s of bytes per second processed.

type             16 bytes     64 bytes    256 bytes   1024 bytes   8192 bytes
md5              13948.89k    42477.61k   104619.41k   165140.82k   199273.13k
sha1             13091.91k    36463.89k    75393.88k   103893.33k   117104.50k
sha256           13573.92k    30492.25k    52700.33k    64247.81k    68722.69k

Hardware:
The 'numbers' are in 1000s of bytes per second processed.

type             16 bytes     64 bytes    256 bytes   1024 bytes   8192 bytes
md5               3964.55k    13782.11k    43181.71k   180263.38k  1446616.18k
sha1              4609.16k     8922.35k    35422.87k   333575.31k  2122547.20k

sha256 13519.62k 30484.10k 52547.47k 64285.21k 68530.60k

There's actually something suspicious while running these tests:

Doing md5 for 3s on 16 size blocks: 32212 md5's in 0.13s Doing md5 for 3s on 64 size blocks: 23688 md5's in 0.11s Doing md5 for 3s on 256 size blocks: 23615 md5's in 0.14s Doing md5 for 3s on 1024 size blocks: 22885 md5's in 0.13s Doing md5 for 3s on 8192 size blocks: 15893 md5's in 0.09s Doing sha1 for 3s on 16 size blocks: 31688 sha1's in 0.11s Doing sha1 for 3s on 64 size blocks: 23700 sha1's in 0.17s Doing sha1 for 3s on 256 size blocks: 23523 sha1's in 0.17s Doing sha1 for 3s on 1024 size blocks: 22803 sha1's in 0.07s Doing sha1 for 3s on 8192 size blocks: 15546 sha1's in 0.06s Doing sha256 for 3s on 16 size blocks: 2518030 sha256's in 2.98s Doing sha256 for 3s on 64 size blocks: 1419416 sha256's in 2.98s Doing sha256 for 3s on 256 size blocks: 613738 sha256's in 2.99s Doing sha256 for 3s on 1024 size blocks: 187080 sha256's in 2.98s Doing sha256 for 3s on 8192 size blocks: 25013 sha256's in 2.99s

from the hardware - note the "in" figures are rediculously low, yet they do wait 3s for each test. Also, the sha256 results are close enough to being the software version.

No ideas on any of this yet... but I'm not about to start digging in the openssl code to try and work out what it's up to. As I say, I think this is AF_ALG with openssl is a dead project.

Hi Russel,

Quoted 7 lines · Russell King - ARM Linux

Software:
The 'numbers' are in 1000s of bytes per second processed.

type             16 bytes     64 bytes    256 bytes   1024 bytes   8192 bytes
md5              13948.89k    42477.61k   104619.41k   165140.82k   199273.13k
sha1             13091.91k    36463.89k    75393.88k   103893.33k   117104.50k
sha256           13573.92k    30492.25k    52700.33k    64247.81k    68722.69k

Quoted 6 lines

Hardware:
The 'numbers' are in 1000s of bytes per second processed.

type             16 bytes     64 bytes    256 bytes   1024 bytes   8192 bytes
md5               3964.55k    13782.11k    43181.71k   180263.38k  1446616.18k
sha1              4609.16k     8922.35k    35422.87k   333575.31k  2122547.20k

sha256 13519.62k 30484.10k 52547.47k 64285.21k 68530.60k

I had a lot of performance results at various levels (tcrypt module on variations of the drivers (tasklet, threaded irq, full polling, etc), IPsec tunnel and transport mode through to see how it behaves w/ two mvneta instances also eating CPU cycles for incoming/outgoing packets) but those where done on an encryption use case. Some are provided in [2]. In an early (read dirty) polling-based version of the driver, the CESA on an Armada 370 (mirabox) was verified to be capable of near 100MB/s on buffers of 1500+ bytes for AES CBC encryption. Current version of the driver is not as good (say half that value) but it behaves better. A Mirabox can easily route 1500 bytes packets at 100MB/s between its two interfaces but when you mix both using IPsec in tunnel mode on one side, you end up w/ perfs between 10 to 15MB/s, IIRC. I think it's interesting to see where it ends up w/ the engine exposed to userland consumers (e.g. sth like SSH).

I cannot promise a huge amount of time but I'll try and find some to play w/ AF_ALG using openssl and CESA in the coming weeks.

Quoted 8 lines · Arnaud Ebalard

Quoted 7 lines · Russell King - ARM Linux

Software:
The 'numbers' are in 1000s of bytes per second processed.

type             16 bytes     64 bytes    256 bytes   1024 bytes   8192 bytes
md5              13948.89k    42477.61k   104619.41k   165140.82k   199273.13k
sha1             13091.91k    36463.89k    75393.88k   103893.33k   117104.50k
sha256           13573.92k    30492.25k    52700.33k    64247.81k    68722.69k

Quoted 6 lines

Quoted 6 lines

Hardware:
The 'numbers' are in 1000s of bytes per second processed.

type             16 bytes     64 bytes    256 bytes   1024 bytes   8192 bytes
md5               3964.55k    13782.11k    43181.71k   180263.38k  1446616.18k
sha1              4609.16k     8922.35k    35422.87k   333575.31k  2122547.20k

sha256 13519.62k 30484.10k 52547.47k 64285.21k 68530.60k

Okay, the reason for the difference in SHA256 speed is because the "openssl speed" code *totally* *bypasses* the engine support, whereas the md5 and sha1 do not. It even bypasses the normal method used to get hold of the sha256 implementation (EVP_sha256), and goes straight to using SHA256() directly in openssl/crypto/sha/sha256.c. It looks like the same goes for the AES tests too.

I _also_ notice that despite having the ARM assembly crypto functions enabled and built-in to the kernel, they don't appear in /proc/crypto - and this is because of patch 1 in this series, which blocks out any crypto driver which has a zero statesize (as the ARM crypto functions appear to have.) I found this by rebuilding the ARM crypto stuff as modules, and then trying to insert them:

# modprobe sha512-arm
modprobe: ERROR: could not insert 'sha512_arm': Invalid argument

So, I think it's best if this patch series is *NOT* merged until someone who knows the kernel crypto code gets to grips with what's supposed to be done in various parts of the code. Yes, I know that Herbert suggested the approach in patch 1, but that _will_ cause regressions like the above when if it's merged.

Quoted 7 lines · Russell King - ARM Linux

I _also_ notice that despite having the ARM assembly crypto functions enabled and built-in to the kernel, they don't appear in /proc/crypto - and this is because of patch 1 in this series, which blocks out any crypto driver which has a zero statesize (as the ARM crypto functions appear to have.) I found this by rebuilding the ARM crypto stuff as modules, and then trying to insert them:

They're buggy and unfortunately this wasn't caught during the review process. The import/export functions are required and certainly not optional.

Quoted 9 lines · Herbert Xu

Quoted 7 lines · Russell King - ARM Linux

I _also_ notice that despite having the ARM assembly crypto functions enabled and built-in to the kernel, they don't appear in /proc/crypto - and this is because of patch 1 in this series, which blocks out any crypto driver which has a zero statesize (as the ARM crypto functions appear to have.) I found this by rebuilding the ARM crypto stuff as modules, and then trying to insert them:

Quoted 3 lines

They're buggy and unfortunately this wasn't caught during the review process. The import/export functions are required and certainly not optional.

OK it looks like I was confused. Yes the import and export functions are required but for shash algorithms we provide a simple default. This means that implementations can simply leave it NULL and they will then use the default import/export functions which exports the shash_desc as the state.

So Russell what I'll do is apply your patch without the hunk for shash_prepare_alg. IOW we will block any ahash algorithms that leave state as zero since ahash does not provide a default import/export function (it can't because it may involve hardware state). shash algorithms on the other hand won't be affected.

Given it's size so far, I'm not sure it makes much sense to backport any of these fixes to stable; I think maybe we should just say "okay, before these fixes, it's just broken."

Quoted 4 lines · Russell King - ARM Linux

Given it's size so far, I'm not sure it makes much sense to backport any of these fixes to stable; I think maybe we should just say "okay, before these fixes, it's just broken."

OK as long as your first patch goes into stable it should be fine as it'll essentially disable the marvell driver.

It will disable the Marvell driver, along with the SW implementations in arch/arm/crypto/ which don't set .statesize, .import and .export. I would not be surprised if it also affects others too.

Quoted 1 line · Arnaud Ebalard

Hi Russel,

           ^

Quoted 9 lines

Quoted 8 lines · Russell King - ARM Linux

Software:
The 'numbers' are in 1000s of bytes per second processed.
type 16 bytes 64 bytes 256 bytes 1024 bytes 8192
bytes md5 13948.89k 42477.61k 104619.41k
165140.82k 199273.13k sha1 13091.91k 36463.89k
75393.88k 103893.33k 117104.50k sha256 13573.92k
30492.25k 52700.33k 64247.81k 68722.69k

Quoted 7 lines

Quoted 7 lines

Hardware:
The 'numbers' are in 1000s of bytes per second processed.
type 16 bytes 64 bytes 256 bytes 1024 bytes 8192
bytes md5 3964.55k 13782.11k 43181.71k
180263.38k 1446616.18k sha1 4609.16k 8922.35k
35422.87k 333575.31k 2122547.20k sha256 13519.62k
30484.10k 52547.47k 64285.21k 68530.60k

Okay, the reason for the difference in SHA256 speed is because the "openssl speed" code *totally* *bypasses* the engine support, whereas the md5 and sha1 do not. It even bypasses the normal method used to get hold of the sha256 implementation (EVP_sha256), and goes straight to using SHA256() directly in openssl/crypto/sha/sha256.c. It looks like the same goes for the AES tests too.

Quoted 15 lines

I had a lot of performance results at various levels (tcrypt module on variations of the drivers (tasklet, threaded irq, full polling, etc), IPsec tunnel and transport mode through to see how it behaves w/ two mvneta instances also eating CPU cycles for incoming/outgoing packets) but those where done on an encryption use case. Some are provided in [2]. In an early (read dirty) polling-based version of the driver, the CESA on an Armada 370 (mirabox) was verified to be capable of near 100MB/s on buffers of 1500+ bytes for AES CBC encryption. Current version of the driver is not as good (say half that value) but it behaves better. A Mirabox can easily route 1500 bytes packets at 100MB/s between its two interfaces but when you mix both using IPsec in tunnel mode on one side, you end up w/ perfs between 10 to 15MB/s, IIRC. I think it's interesting to see where it ends up w/ the engine exposed to userland consumers (e.g. sth like SSH).

Quoted 2 lines

I cannot promise a huge amount of time but I'll try and find some to play w/ AF_ALG using openssl and CESA in the coming weeks.

I think what we draw from my investigation is that "openssl speed" is utterly crap - you don't actually know what's being tested there. Some things test the engine, others bypass the engine infrastructure totally and test the openssl software implementation instead.

So, if you think "openssl speed" is a good way to measure the speed of digests and ciphers that openssl supplies to applications, *think again*. It doesn't.

After all, here's a version 3, which fixes the other bug I mentioned below - we now have correct hash results. A couple of patches have been added, one to fix that, and a second patch to factor out the duplication in the import functions. This gets openssh working with the digests enabled.

Acks so far received have been added. Patch 3 has changed, so I didn't add the ack for the previous version. Please re-review patch 3.

There's an easier way to get at the hash transform - rather than using crypto_ahash_tfm(ahash), we can get it directly from req->base.tfm.

Signed-off-by Russell King <[email protected]>

Rather than determining whether we're using a MD5 hash by looking at the digest size, switch to a cleaner solution using a per-request flag initialised by the method type.

Signed-off-by Russell King <[email protected]>

---
 drivers/crypto/marvell/cesa.h |  1 +
 drivers/crypto/marvell/hash.c | 17 +++++++++--------
 2 files changed, 10 insertions(+), 8 deletions(-)

drivers/crypto/marvell/hash.c +1 -1

index f59faabcd34f..aa12274608ab 100644
@@ -351,7 +351,7 @@ static int mv_cesa_ahash_process(struct crypto_async_request *req, u32 status)
 		 * Hardware's MD5 digest is in little endian format, but
 		 * SHA in big endian format
 		 */
-		if (digsize == MD5_DIGEST_SIZE) {
+		if (creq->algo_le) {
 			__le32 *result = (void *)ahashreq->result;
 
 			for (i = 0; i < digsize / 4; i++)

Hey Russell,

drivers/crypto/marvell/hash.c static const struct mv_cesa_req_ops mv_cesa_ahash_req_ops = { · line 410

Quoted 3 lines · Russell King

static int mv_cesa_ahash_init(struct ahash_request *req,

drivers/crypto/marvell/hash.c +1 -1

-			      struct mv_cesa_op_ctx *tmpl)
+			      struct mv_cesa_op_ctx *tmpl, bool algo_le)

nit: Would it make more sense the do bool algo_endian, and then ...

drivers/crypto/marvell/hash.c static int mv_cesa_md5_init(struct ahash_request *req) · line 865

Quoted 7 lines

drivers/crypto/marvell/hash.c +1 -1

@@ -861,7 +862,7 @@ static int mv_cesa_md5_init(struct ahash_request *req)
 
 	mv_cesa_set_op_cfg(&tmpl, CESA_SA_DESC_CFG_MACM_MD5);
 
-	mv_cesa_ahash_init(req, &tmpl);
+	mv_cesa_ahash_init(req, &tmpl, true);

	mv_cesa_ahash_init(req, &tmpl, ALGO_ENDIAN_LITTLE);

'true' doesn't seem as obvious. But again, nit-picky.

Quoted 11 lines

drivers/crypto/marvell/hash.c static int mv_cesa_md5_init(struct ahash_request *req) · line 865

Quoted 7 lines

drivers/crypto/marvell/hash.c +1 -1

@@ -861,7 +862,7 @@ static int mv_cesa_md5_init(struct ahash_request *req)
 
 	mv_cesa_set_op_cfg(&tmpl, CESA_SA_DESC_CFG_MACM_MD5);
 
-	mv_cesa_ahash_init(req, &tmpl);
+	mv_cesa_ahash_init(req, &tmpl, true);

	mv_cesa_ahash_init(req, &tmpl, ALGO_ENDIAN_LITTLE);

'true' doesn't seem as obvious. But again, nit-picky.

I did think about:

enum {
ALGO_LITTLE_ENDIAN,
ALGO_BIG_ENDIAN,
};

and passing an int algo_endian around, but that seemed to be like too much bloat... though if you want to insist, I could make that change.

Quoted 3 lines · Jason Cooper

Hey Russell,

On Sun, Oct 18, 2015 at 05:23:40PM +0100, Russell King wrote:

Quoted 5 lines

drivers/crypto/marvell/hash.c static const struct mv_cesa_req_ops mv_cesa_ahash_req_ops = { · line 410

Quoted 3 lines

static int mv_cesa_ahash_init(struct ahash_request *req,

drivers/crypto/marvell/hash.c +1 -1

-			      struct mv_cesa_op_ctx *tmpl)
+			      struct mv_cesa_op_ctx *tmpl, bool algo_le)

nit: Would it make more sense the do bool algo_endian, and then ...

I don't like "bool"s that don't self-document what they mean. What does "if (algo_endian)" mean? It's pretty clear what

"if (algo_le)" means in the context of endianness, though I'll give you that "if (algo_little_endian)" would be a lot better.

Quoted 10 lines

drivers/crypto/marvell/hash.c static int mv_cesa_md5_init(struct ahash_request *req) · line 865

Quoted 6 lines

drivers/crypto/marvell/hash.c +1 -1

@@ -861,7 +862,7 @@ static int mv_cesa_md5_init(struct ahash_request *req)
 
 	mv_cesa_set_op_cfg(&tmpl, CESA_SA_DESC_CFG_MACM_MD5);
 
-	mv_cesa_ahash_init(req, &tmpl);
+	mv_cesa_ahash_init(req, &tmpl, true);

	mv_cesa_ahash_init(req, &tmpl, ALGO_ENDIAN_LITTLE);

'true' doesn't seem as obvious. But again, nit-picky.

I did think about:

enum {
ALGO_LITTLE_ENDIAN,
ALGO_BIG_ENDIAN,
};

and passing an int algo_endian around, but that seemed to be like too much bloat... though if you want to insist, I could make that change.