[1/2] crypto: arm/ghash-ce - add missing async import/export

Message ID 1472469594-27315-1-git-send-email-ard.biesheuvel@linaro.org
State New
Headers show

Commit Message

Ard Biesheuvel Aug. 29, 2016, 11:19 a.m.
Since commit 8996eafdcbad ("crypto: ahash - ensure statesize is non-zero"),
all ahash drivers are required to implement import()/export(), and must have
a non-zero statesize. Fix this for the ARM Crypto Extensions GHASH
implementation.

Fixes: 8996eafdcbad ("crypto: ahash - ensure statesize is non-zero")
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

---
 arch/arm/crypto/ghash-ce-glue.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Ard Biesheuvel Sept. 1, 2016, 1:19 p.m. | #1
On 31 August 2016 at 15:41, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Mon, Aug 29, 2016 at 12:19:53PM +0100, Ard Biesheuvel wrote:

>> Since commit 8996eafdcbad ("crypto: ahash - ensure statesize is non-zero"),

>> all ahash drivers are required to implement import()/export(), and must have

>> a non-zero statesize. Fix this for the ARM Crypto Extensions GHASH

>> implementation.

>>

>> Fixes: 8996eafdcbad ("crypto: ahash - ensure statesize is non-zero")

>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>> ---

>>  arch/arm/crypto/ghash-ce-glue.c | 26 ++++++++++++++++++++++++++

>>  1 file changed, 26 insertions(+)

>>

>> diff --git a/arch/arm/crypto/ghash-ce-glue.c b/arch/arm/crypto/ghash-ce-glue.c

>> index 1568cb5cd870..212aaa715fdb 100644

>> --- a/arch/arm/crypto/ghash-ce-glue.c

>> +++ b/arch/arm/crypto/ghash-ce-glue.c

>> @@ -220,6 +220,29 @@ static int ghash_async_digest(struct ahash_request *req)

>>       }

>>  }

>>

>> +static int ghash_async_import(struct ahash_request *req, const void *in)

>> +{

>> +     struct ahash_request *cryptd_req = ahash_request_ctx(req);

>> +     struct shash_desc *desc = cryptd_shash_desc(cryptd_req);

>> +     struct ghash_desc_ctx *dctx = shash_desc_ctx(desc);

>> +

>> +     ghash_async_init(req);

>

> Is this really needed?

>


Actually, yes, and more. I could not find in the documentation whether
the .import() hook requires .init() to be called first, but based on
the test case in testmgr.c, it appears that is not the case.

This means that the stuff that occurs in .init() to establish the
relation between this desc and its child transform needs to be copied
here as well. In fact, the only way I could make this work is by
adding it to cryptd's import() routines as well, otherwise the test
crashes reliably.

>> +     *dctx = *(const struct ghash_desc_ctx *)in;

>

> I'd prefer to call the underlying shash import/export functions

> like we do in cryptd.

>


OK, I can change that

Thanks,
Ard.
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ard Biesheuvel Sept. 1, 2016, 3:14 p.m. | #2
On 1 September 2016 at 14:19, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 31 August 2016 at 15:41, Herbert Xu <herbert@gondor.apana.org.au> wrote:

>> On Mon, Aug 29, 2016 at 12:19:53PM +0100, Ard Biesheuvel wrote:

>>> Since commit 8996eafdcbad ("crypto: ahash - ensure statesize is non-zero"),

>>> all ahash drivers are required to implement import()/export(), and must have

>>> a non-zero statesize. Fix this for the ARM Crypto Extensions GHASH

>>> implementation.

>>>

>>> Fixes: 8996eafdcbad ("crypto: ahash - ensure statesize is non-zero")

>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>>> ---

>>>  arch/arm/crypto/ghash-ce-glue.c | 26 ++++++++++++++++++++++++++

>>>  1 file changed, 26 insertions(+)

>>>

>>> diff --git a/arch/arm/crypto/ghash-ce-glue.c b/arch/arm/crypto/ghash-ce-glue.c

>>> index 1568cb5cd870..212aaa715fdb 100644

>>> --- a/arch/arm/crypto/ghash-ce-glue.c

>>> +++ b/arch/arm/crypto/ghash-ce-glue.c

>>> @@ -220,6 +220,29 @@ static int ghash_async_digest(struct ahash_request *req)

>>>       }

>>>  }

>>>

>>> +static int ghash_async_import(struct ahash_request *req, const void *in)

>>> +{

>>> +     struct ahash_request *cryptd_req = ahash_request_ctx(req);

>>> +     struct shash_desc *desc = cryptd_shash_desc(cryptd_req);

>>> +     struct ghash_desc_ctx *dctx = shash_desc_ctx(desc);

>>> +

>>> +     ghash_async_init(req);

>>

>> Is this really needed?

>>

>

> Actually, yes, and more. I could not find in the documentation whether

> the .import() hook requires .init() to be called first, but based on

> the test case in testmgr.c, it appears that is not the case.

>

> This means that the stuff that occurs in .init() to establish the

> relation between this desc and its child transform needs to be copied

> here as well. In fact, the only way I could make this work is by

> adding it to cryptd's import() routines as well, otherwise the test

> crashes reliably.

>

>>> +     *dctx = *(const struct ghash_desc_ctx *)in;

>>

>> I'd prefer to call the underlying shash import/export functions

>> like we do in cryptd.

>>

>


I managed to track down why this issue seems specific to ARM (i.e.,
why it does not affect the x86 clmul-ni driver as well)

The culprit appears to be that the .cra_name of the internal shash is
"ghash", (and not "__ghash" like in the x86 case) which causes the
test code to run the test on not only the public ahash, but also on
the internal cryptd() encapsulated shash, and also on the internal
shash itself.

However, that does not answer the question whether .init() must be
called before .import() [which the test code does not do]. If not,
then please disregard my v2, and I will followup with a patch that
renames ghash to __ghash (but .import() will still require the .init()
bits as well). Given that these internal shashes/ahashes are in fact
callable, and calling .import() will result in a crash, I suppose
duplicating some of the init() code in .import() makes sense
regardless.
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch hide | download patch | download mbox

diff --git a/arch/arm/crypto/ghash-ce-glue.c b/arch/arm/crypto/ghash-ce-glue.c
index 1568cb5cd870..212aaa715fdb 100644
--- a/arch/arm/crypto/ghash-ce-glue.c
+++ b/arch/arm/crypto/ghash-ce-glue.c
@@ -220,6 +220,29 @@  static int ghash_async_digest(struct ahash_request *req)
 	}
 }
 
+static int ghash_async_import(struct ahash_request *req, const void *in)
+{
+	struct ahash_request *cryptd_req = ahash_request_ctx(req);
+	struct shash_desc *desc = cryptd_shash_desc(cryptd_req);
+	struct ghash_desc_ctx *dctx = shash_desc_ctx(desc);
+
+	ghash_async_init(req);
+
+	*dctx = *(const struct ghash_desc_ctx *)in;
+	return 0;
+
+}
+
+static int ghash_async_export(struct ahash_request *req, void *out)
+{
+	struct ahash_request *cryptd_req = ahash_request_ctx(req);
+	struct shash_desc *desc = cryptd_shash_desc(cryptd_req);
+	struct ghash_desc_ctx *dctx = shash_desc_ctx(desc);
+
+	*(struct ghash_desc_ctx *)out = *dctx;
+	return 0;
+}
+
 static int ghash_async_setkey(struct crypto_ahash *tfm, const u8 *key,
 			      unsigned int keylen)
 {
@@ -268,7 +291,10 @@  static struct ahash_alg ghash_async_alg = {
 	.final			= ghash_async_final,
 	.setkey			= ghash_async_setkey,
 	.digest			= ghash_async_digest,
+	.import			= ghash_async_import,
+	.export			= ghash_async_export,
 	.halg.digestsize	= GHASH_DIGEST_SIZE,
+	.halg.statesize		= sizeof(struct ghash_desc_ctx),
 	.halg.base		= {
 		.cra_name	= "ghash",
 		.cra_driver_name = "ghash-ce",