[PATCH 1/1] crypto: testmgr - call shash_init in crc32c algo

[PATCH 1/1] crypto: testmgr - call shash_init in crc32c algo

[Lionel Debieve]

In case of device call required in low level driver,
the context must be initialized before calling the final
function.

Signed-off-by: Lionel Debieve <lionel.debieve@st.com>
---
 crypto/testmgr.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/crypto/testmgr.c b/crypto/testmgr.c
index 8386038..4a00d7c 100644
--- a/crypto/testmgr.c
+++ b/crypto/testmgr.c
@@ -2181,6 +2181,13 @@ static int alg_test_crc32c(const struct alg_test_desc *desc,
 		shash->tfm = tfm;
 		shash->flags = 0;
 
+		err = crypto_shash_init(shash);
+		if (err) {
+			printk(KERN_ERR "alg: crc32c: init failed for "
+			       "%s: %d\n", driver, err);
+			break;
+		}
+
 		*ctx = 420553207;
 		err = crypto_shash_final(shash, (u8 *)&val);
 		if (err) {
-- 
2.7.4

Re: [PATCH 1/1] crypto: testmgr - call shash_init in crc32c algo

[Eric Biggers]

Hi Lionel,

On Mon, Apr 01, 2019 at 02:54:24PM +0200, Lionel Debieve wrote:
> In case of device call required in low level driver,
> the context must be initialized before calling the final
> function.
> 
> Signed-off-by: Lionel Debieve <lionel.debieve@st.com>
> ---
>  crypto/testmgr.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/crypto/testmgr.c b/crypto/testmgr.c
> index 8386038..4a00d7c 100644
> --- a/crypto/testmgr.c
> +++ b/crypto/testmgr.c
> @@ -2181,6 +2181,13 @@ static int alg_test_crc32c(const struct alg_test_desc *desc,
>  		shash->tfm = tfm;
>  		shash->flags = 0;
>  
> +		err = crypto_shash_init(shash);
> +		if (err) {
> +			printk(KERN_ERR "alg: crc32c: init failed for "
> +			       "%s: %d\n", driver, err);
> +			break;
> +		}
> +
>  		*ctx = 420553207;
>  		err = crypto_shash_final(shash, (u8 *)&val);
>  		if (err) {
> -- 
> 2.7.4
> 

This defeats the point of the test, which is that crc32c implementations are
expected to use the same shash_desc context format and be usable by calling
crypto_shash_update() directly after initializing the context manually, without
a prior crypto_shash_init().  See for example ext4_chksum() in fs/ext4/ext4.h:

	static inline u32 ext4_chksum(struct ext4_sb_info *sbi, u32 crc,
				      const void *address, unsigned int length)
	{
		struct {
			struct shash_desc shash;
			char ctx[4];
		} desc;

		BUG_ON(crypto_shash_descsize(sbi->s_chksum_driver)!=sizeof(desc.ctx));

		desc.shash.tfm = sbi->s_chksum_driver;
		desc.shash.flags = 0;
		*(u32 *)desc.ctx = crc;

		BUG_ON(crypto_shash_update(&desc.shash, address, length));

		return *(u32 *)desc.ctx;
	}

I think you need to fix the stm32 crc32 driver to not store anything extra in
the shash_desc context, and only use hardware during ->update().

- Eric

Re: [PATCH 1/1] crypto: testmgr - call shash_init in crc32c algo

[Lionel DEBIEVE]

Hi Eric,

On 4/1/19 7:30 PM, Eric Biggers wrote:

> Hi Lionel,
>
> On Mon, Apr 01, 2019 at 02:54:24PM +0200, Lionel Debieve wrote:
>> In case of device call required in low level driver,
>> the context must be initialized before calling the final
>> function.
>>
>> Signed-off-by: Lionel Debieve<lionel.debieve@st.com>
>> ---
>>   crypto/testmgr.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/crypto/testmgr.c b/crypto/testmgr.c
>> index 8386038..4a00d7c 100644
>> --- a/crypto/testmgr.c
>> +++ b/crypto/testmgr.c
>> @@ -2181,6 +2181,13 @@ static int alg_test_crc32c(const struct alg_test_desc *desc,
>>   		shash->tfm = tfm;
>>   		shash->flags = 0;
>>   
>> +		err = crypto_shash_init(shash);
>> +		if (err) {
>> +			printk(KERN_ERR "alg: crc32c: init failed for "
>> +			       "%s: %d\n", driver, err);
>> +			break;
>> +		}
>> +
>>   		*ctx = 420553207;
>>   		err = crypto_shash_final(shash, (u8 *)&val);
>>   		if (err) {
>> -- 
>> 2.7.4
>>
> This defeats the point of the test, which is that crc32c implementations are
> expected to use the same shash_desc context format and be usable by calling
> crypto_shash_update() directly after initializing the context manually, without
> a prior crypto_shash_init().  See for example ext4_chksum() in fs/ext4/ext4.h:
>
> 	static inline u32 ext4_chksum(struct ext4_sb_info *sbi, u32 crc,
> 				      const void *address, unsigned int length)
> 	{
> 		struct {
> 			struct shash_desc shash;
> 			char ctx[4];
> 		} desc;
>
> 		BUG_ON(crypto_shash_descsize(sbi->s_chksum_driver)!=sizeof(desc.ctx));
>
> 		desc.shash.tfm = sbi->s_chksum_driver;
> 		desc.shash.flags = 0;
> 		*(u32 *)desc.ctx = crc;
>
> 		BUG_ON(crypto_shash_update(&desc.shash, address, length));
>
> 		return *(u32 *)desc.ctx;
> 	}
>
> I think you need to fix the stm32 crc32 driver to not store anything extra in
> the shash_desc context, and only use hardware during ->update().
>
> - Eric
>
OK, catch your point but refering to the devel-algos.rst documentation, it seems that there is no way
to bypass the init part. I'm based on a hardware that need a clear initialization to be up and running.
Is the doc not updated? I'm working to optimized data transfer into my hardware by pushing only 32 bits
datas and I'm saving the others to be pushed later.

If we assume that hardware must be only used in the update function, I've to rework the driver as today,
HW init is made in the init function too.

BR
- Lionel