Re: [PATCH 2/3] crypto: inside-secure - added support for rfc3686(ctr(aes))

[Antoine Tenart <antoine.tenart@bootlin.com>]

Hi Pascal,

On Fri, Jul 26, 2019 at 01:28:13PM +0000, Pascal Van Leeuwen wrote:
> > On Fri, Jul 05, 2019 at 08:49:23AM +0200, Pascal van Leeuwen wrote:
> 
> > > -		/* H/W capabilities selection */
> > > -		val = EIP197_FUNCTION_RSVD;
> > > -		val |= EIP197_PROTOCOL_ENCRYPT_ONLY | EIP197_PROTOCOL_HASH_ONLY;
> > > -		val |= EIP197_PROTOCOL_ENCRYPT_HASH | EIP197_PROTOCOL_HASH_DECRYPT;
> > > -		val |= EIP197_ALG_DES_ECB | EIP197_ALG_DES_CBC;
> > > -		val |= EIP197_ALG_3DES_ECB | EIP197_ALG_3DES_CBC;
> > > -		val |= EIP197_ALG_AES_ECB | EIP197_ALG_AES_CBC;
> > > -		val |= EIP197_ALG_MD5 | EIP197_ALG_HMAC_MD5;
> > > -		val |= EIP197_ALG_SHA1 | EIP197_ALG_HMAC_SHA1;
> > > -		val |= EIP197_ALG_SHA2 | EIP197_ALG_HMAC_SHA2;
> > > -		writel(val, EIP197_PE(priv) + EIP197_PE_EIP96_FUNCTION_EN(pe));
> > > +		/* H/W capabilities selection: just enable everything */
> > > +		writel(EIP197_FUNCTION_ALL,
> > > +		       EIP197_PE(priv) + EIP197_PE_EIP96_FUNCTION_EN(pe));
> > 
> > This should be in a separate patch.
> >
> It was added specifically to  get this functionality working as CTR mode was not
> enabled.  So I don't see why it should be a seperate patch, really?
> Instead of adding CTR mode to the list (which would have been perfectly valid
> in this context anyway), I just chose to enable everything instead.

Because it's not the same thing to enable everything and to add one extra
alg. This makes bissecting the driver harder because one would not except
this change in this kind of patch.

> > I'm also not sure about it, as
> > controlling exactly what algs are enabled in the h/w could prevent
> > misconfiguration issues in the control descriptors.
> > 
> That's not really what it's supposed to be used for. It's supposed to be used for
> disabling algorithms the application is not ALLOWED to use e.g. because they are
> not deemed secure enough (in case you have to comply, with,  say, FIPS or so).

OK, that answer my question and this makes sense.

> > > @@ -62,9 +63,9 @@ static void safexcel_skcipher_token(struct safexcel_cipher_ctx *ctx, u8 *iv,
> > >  				    u32 length)
> > > -	if (ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CBC) {
> > > +	if (ctx->mode != CONTEXT_CONTROL_CRYPTO_MODE_ECB) {
> > 
> > I think it's better for maintenance and readability to have something
> > like:
> > 
> >   if (ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CBC ||
> >       ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CTR_LOAD)
> > 
> Not really. I *really* want to execute that for any mode other than ECB,
> ECB being the *only* mode that does not require an IV (which I know
> for a fact, being the architect and all :-).
> And I don't believe a long list of modes that *do* require an IV would 
> be  more readable or easy to maintain than this single compare ...

That's where I disagree as you need extra knowledge to be aware of this.
Being explicit removes any possible question one may ask. But that's a
small point really :)

Thanks!
Antoine

-- 
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com