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

[Thara Gopinath]

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