Re: [PATCH 6/7] crypto: qce: common: Add support for AEAD algorithms

[Thara Gopinath <thara.gopinath@linaro.org>]

On 4/13/21 7:09 PM, Bjorn Andersson wrote:
> On Tue 13 Apr 17:44 CDT 2021, Thara Gopinath wrote:

[..]

> 
> Yes, given that you just typecast things as you do it should just work
> to move the typecast to the qce_cpu_to_be32p_array().
> 
> But as I said, this would indicate that what is cpu_to_be32() should
> have been be32_to_cpu() (both performs the same swap, it's just a matter
> of what type goes in and what type goes out).
> 
> Looking at the other uses of qce_cpu_to_be32p_array() I suspect that
> it's the same situation there, so perhaps introduce a new function
> qce_be32_to_cpu() in this patchset (that returns number of words
> converted) and then look into the existing users after that?

Hi!

I have sent out the v2 with the new function. To me, it does look 
cleaner. So thanks!

> 
> [..]
>>>>>> +	if (IS_SHA_HMAC(rctx->flags)) {
>>>>>> +		/* Write default authentication iv */
>>>>>> +		if (IS_SHA1_HMAC(rctx->flags)) {
>>>>>> +			auth_ivsize = SHA1_DIGEST_SIZE;
>>>>>> +			memcpy(authiv, std_iv_sha1, auth_ivsize);
>>>>>> +		} else if (IS_SHA256_HMAC(rctx->flags)) {
>>>>>> +			auth_ivsize = SHA256_DIGEST_SIZE;
>>>>>> +			memcpy(authiv, std_iv_sha256, auth_ivsize);
>>>>>> +		}
>>>>>> +		authiv_words = auth_ivsize / sizeof(u32);
>>>>>> +		qce_write_array(qce, REG_AUTH_IV0, (u32 *)authiv, authiv_words);
>>>>>
>>>>> AUTH_IV0 is affected by the little endian configuration, does this imply
>>>>> that IS_SHA_HMAC() and IS_CCM() are exclusive bits of rctx->flags? If so
>>>>> I think it would be nice if you grouped the conditionals in a way that
>>>>> made that obvious when reading the function.
>>>>
>>>> So yes IS_SHA_HMAC() and IS_CCM() are exclusive bits of rctx->flags.
>>>> AUTH_IVn is 0 for ccm and has initial value for HMAC algorithms. I don't
>>>> understand the confusion here.
>>>>
>>>
>>> I'm just saying that writing is as below would have made it obvious to
>>> me that IS_SHA_HMAC() and IS_CCM() are exclusive:
>>
>> So regardless of the mode, it is a good idea  to clear the IV  registers
>> which happens above in
>>
>> 	qce_clear_array(qce, REG_AUTH_IV0, 16);
>>
>>
>> This is important becasue the size of IV varies between HMAC(SHA1) and
>> HMAC(SHA256) and we don't want any previous bits sticking around.
>> For CCM after the above step we don't do anything with AUTH_IV where as for
>> SHA_HMAC we have to go and program the registers. I can split it into
>> if (IS_SHA_HMAC(flags)) {
>> 	...
>> } else {
>> 	...
>> }
>>
>> but both snippets will have the above line code clearing the IV register and
>> the if part will have more stuff actually programming these registers.. Is
>> that what you are looking for ?
> 
> I didn't find an answer quickly to the question if the two where
> mutually exclusive and couldn't determine from the code flow either. But
> my comment seems to stem from my misunderstanding that the little endian
> bit was dependent on IS_CCM().
> 
> That said, if the logic really is "do this for IS_SHA_HMAC(), otherwise
> do that", then if else makes sense.

So, the logic is really. do "clearing of IV" in all cases. Do setting of 
initial IV only for HMAC. I tried the if..else like you said. It did not 
look correct to duplicate the code clearing the IV. So I have added 
comments around this section hopefully making it clearer. Do take a look 
and let me know. I can rework it further if you think so.

> 
> Regards,
> Bjorn
> 

-- 
Warm Regards
Thara