Messages in this thread | | | Subject | Re: [PATCH 6/7] crypto: qce: common: Add support for AEAD algorithms | From | Thara Gopinath <> | Date | Sat, 17 Apr 2021 09:31:00 -0400 |
| |
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
| |