Re: [PATCH v4 02/35] crypto: chacha - move existing library code into lib/crypto

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On Wed, 23 Oct 2019 at 05:05, Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Thu, Oct 17, 2019 at 09:08:59PM +0200, Ard Biesheuvel wrote:
> > +static inline void chacha_crypt(u32 *state, u8 *dst, const u8 *src,
> > +                             unsigned int bytes, int nrounds)
> > +{
> > +     if (IS_ENABLED(CONFIG_CRYPTO_ARCH_HAVE_LIB_CHACHA))
> > +             chacha_crypt_arch(state, dst, src, bytes, nrounds);
> > +     else
> > +             chacha_crypt_generic(state, dst, src, bytes, nrounds);
> > +}
>
> How about also providing chacha20_crypt() which calls chacha_crypt(..., 20)?
> The 'nrounds' parameter is really for implementations, rather than users of the
> library API.  Users don't really have any business specifying the number of
> rounds as an int.
>

OK

> > +static inline int chacha_setkey(struct crypto_skcipher *tfm, const u8 *key,
> > +                             unsigned int keysize, int nrounds)
> > +{
> > +     struct chacha_ctx *ctx = crypto_skcipher_ctx(tfm);
> > +     int i;
> > +
> > +     if (keysize != CHACHA_KEY_SIZE)
> > +             return -EINVAL;
> > +
> > +     for (i = 0; i < ARRAY_SIZE(ctx->key); i++)
> > +             ctx->key[i] = get_unaligned_le32(key + i * sizeof(u32));
> > +
> > +     ctx->nrounds = nrounds;
> > +     return 0;
> > +}
>
> At the end of this patch series there are 5 drivers which wrap chacha_setkey()
> with chacha20_setkey() and chacha12_setkey() -- all 5 pairs identical.  How
> about providing those as inline functions here?
>

The reason I avoided this is because they are only invoked via
indirect calls, but actually, there's nothing wrong with letting the
compiler instantiate it where needed, so I'll change this.

> > +config CRYPTO_LIB_CHACHA
> > +     tristate "ChaCha library interface"
> > +     depends on CRYPTO_ARCH_HAVE_LIB_CHACHA || !CRYPTO_ARCH_HAVE_LIB_CHACHA
> > +     select CRYPTO_LIB_CHACHA_GENERIC if CRYPTO_ARCH_HAVE_LIB_CHACHA=n
> > +     help
> > +       Enable the ChaCha library interface. This interface may be fulfilled
> > +       by either the generic implementation or an arch-specific one, if one
> > +       is available and enabled.
>
> Since this is a library for use within the kernel, and not a user-visible
> feature, I don't think it should be explicitly selectable.  I.e. it should just
> be "tristate", without the prompt string.
>

The issue here is that it is not implicitly selectable either due to
the dependencies. E.g, if you select the generic version as a builtin
and the accelerated version as a module, you could enable the
chachapoly library interface as a builtin but it won't use the
accelerated code at all. So either we declare here that both need to
be either builtin or modules, or we ignore that and allow people to
create suboptimal Kconfigs.