Antoine,
> Hi Pascal,
>
> On Fri, Jul 05, 2019 at 08:49:23AM +0200, Pascal van Leeuwen wrote:
> > Signed-off-by: Pascal van Leeuwen <pvanleeuwen@xxxxxxxxxxxxxx>
>
> Could you add a commit message?
>
I can do that. Just didn't know it was really needed.
> > - /* 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.
> 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).
As for misconfiguration: you may just as well hit another enabled algorithm, the
odds of which will increase as I add more. And I'm very busy adding ALL of them,
by which time you wouldn't want to disable any anyway.
> > @@ -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 ...
> > +struct safexcel_alg_template safexcel_alg_ctr_aes = {
> > + .type = SAFEXCEL_ALG_TYPE_SKCIPHER,
>
> Same comment as in patch 1 about the .engines member of the struct.
>
Same answer: depends on the disappearing patch I will resend shortly.
> > + .alg.skcipher = {
> > + .setkey = safexcel_skcipher_aesctr_setkey,
> > + .encrypt = safexcel_ctr_aes_encrypt,
> > + .decrypt = safexcel_ctr_aes_decrypt,
> > + /* Add 4 to include the 4 byte nonce! */
> > + .min_keysize = AES_MIN_KEY_SIZE + 4,
> > + .max_keysize = AES_MAX_KEY_SIZE + 4,
>
> You could use CTR_RFC3686_NONCE_SIZE here (maybe in other places in the
> patch as well).
>
Makes sense. I did not know such a constant existed already.
> > + .ivsize = 8,
>
> And CTR_RFC3686_IV_SIZE here.
>
Idem
> Thanks!
> Antoine
>
> --
> Antoine Ténart, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
www.insidesecure.com
