On Wed, 11 Nov 2020 at 18:46, Ondrej Mosnáček <omosnacek@xxxxxxxxx> wrote:
>
> ut 10. 11. 2020 o 20:04 Ard Biesheuvel <ardb@xxxxxxxxxx> napísal(a):
> > Wiring the SIMD code into the generic driver has the unfortunate side
> > effect that the tcrypt testing code cannot distinguish them, and will
> > therefore not use the latter to fuzz test the former, as it does for
> > other algorithms.
>
> Looking back at cf3d41adcc35 ("crypto: aegis128 - add support for SIMD
> acceleration"), I see that that there are aegis128_do_simd()
> conditionals also in the generic block update functions, so the
> "generic" variant is really only "half" generic. But maybe that
> doesn't really matter for the fuzzing, since the code paths are
> significantly different anyway...
>
Thanks for pointing that out. I think we should be pedantic here, and
ensure that the generic code never takes the SIMD code path.
> >
> > So let's refactor the code a bit so we can register two implementations:
> > aegis128-generic and aegis128-simd.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@xxxxxxxxxx>
> > ---
> > crypto/aegis128-core.c | 176 +++++++++++++-------
> > 1 file changed, 119 insertions(+), 57 deletions(-)
> >
> > diff --git a/crypto/aegis128-core.c b/crypto/aegis128-core.c
> > index 859c7b905618..19f38e8c1627 100644
> > --- a/crypto/aegis128-core.c
> > +++ b/crypto/aegis128-core.c
> [...]
> > @@ -482,42 +457,128 @@ static int crypto_aegis128_decrypt(struct aead_request *req)
> > return 0;
> > }
> >
> > -static struct aead_alg crypto_aegis128_alg = {
> > - .setkey = crypto_aegis128_setkey,
> > - .setauthsize = crypto_aegis128_setauthsize,
> > - .encrypt = crypto_aegis128_encrypt,
> > - .decrypt = crypto_aegis128_decrypt,
> > +static int crypto_aegis128_encrypt_simd(struct aead_request *req)
> > +{
> > + struct crypto_aead *tfm = crypto_aead_reqtfm(req);
> > + union aegis_block tag = {};
> > + unsigned int authsize = crypto_aead_authsize(tfm);
> > + struct aegis_ctx *ctx = crypto_aead_ctx(tfm);
> > + unsigned int cryptlen = req->cryptlen;
> > + struct skcipher_walk walk;
> > + struct aegis_state state;
> >
> > - .ivsize = AEGIS128_NONCE_SIZE,
> > - .maxauthsize = AEGIS128_MAX_AUTH_SIZE,
> > - .chunksize = AEGIS_BLOCK_SIZE,
> > + if (!aegis128_do_simd())
> > + return crypto_aegis128_encrypt_generic(req);
> >
> > - .base = {
> > - .cra_blocksize = 1,
> > - .cra_ctxsize = sizeof(struct aegis_ctx),
> > - .cra_alignmask = 0,
> > + skcipher_walk_aead_encrypt(&walk, req, false);
> > + crypto_aegis128_init_simd(&state, &ctx->key, req->iv);
> > + crypto_aegis128_process_ad(&state, req->src, req->assoclen);
> > + crypto_aegis128_process_crypt(&state, &walk,
> > + crypto_aegis128_encrypt_chunk_simd);
> > + crypto_aegis128_final_simd(&state, &tag, req->assoclen, cryptlen, 0);
> >
> > - .cra_priority = 100,
> > + scatterwalk_map_and_copy(tag.bytes, req->dst, req->assoclen + cryptlen,
> > + authsize, 1);
> > + return 0;
> > +}
> >
> > - .cra_name = "aegis128",
> > - .cra_driver_name = "aegis128-generic",
> > +static int crypto_aegis128_decrypt_simd(struct aead_request *req)
> > +{
> > + struct crypto_aead *tfm = crypto_aead_reqtfm(req);
> > + union aegis_block tag;
> > + unsigned int authsize = crypto_aead_authsize(tfm);
> > + unsigned int cryptlen = req->cryptlen - authsize;
> > + struct aegis_ctx *ctx = crypto_aead_ctx(tfm);
> > + struct skcipher_walk walk;
> > + struct aegis_state state;
> > +
> > + if (!aegis128_do_simd())
> > + return crypto_aegis128_decrypt_generic(req);
> > +
> > + scatterwalk_map_and_copy(tag.bytes, req->src, req->assoclen + cryptlen,
> > + authsize, 0);
> > +
> > + skcipher_walk_aead_decrypt(&walk, req, false);
> > + crypto_aegis128_init_simd(&state, &ctx->key, req->iv);
> > + crypto_aegis128_process_ad(&state, req->src, req->assoclen);
> > + crypto_aegis128_process_crypt(&state, &walk,
> > + crypto_aegis128_decrypt_chunk_simd);
> >
> > - .cra_module = THIS_MODULE,
> > + if (unlikely(crypto_aegis128_final_simd(&state, &tag, req->assoclen,
> > + cryptlen, authsize))) {
> > + skcipher_walk_aead_decrypt(&walk, req, false);
> > + crypto_aegis128_process_crypt(NULL, &walk,
> > + crypto_aegis128_wipe_chunk);
> > + return -EBADMSG;
> > }
> > + return 0;
> > +}
> > +
> > +static struct aead_alg crypto_aegis128_alg_generic = {
> > + .setkey = crypto_aegis128_setkey,
> > + .setauthsize = crypto_aegis128_setauthsize,
> > + .encrypt = crypto_aegis128_encrypt_generic,
> > + .decrypt = crypto_aegis128_decrypt_generic,
> > +
> > + .ivsize = AEGIS128_NONCE_SIZE,
> > + .maxauthsize = AEGIS128_MAX_AUTH_SIZE,
> > + .chunksize = AEGIS_BLOCK_SIZE,
> > +
> > + .base.cra_blocksize = 1,
> > + .base.cra_ctxsize = sizeof(struct aegis_ctx),
> > + .base.cra_alignmask = 0,
> > + .base.cra_priority = 100,
> > + .base.cra_name = "aegis128",
> > + .base.cra_driver_name = "aegis128-generic",
> > +};
> > +
> > +static struct aead_alg crypto_aegis128_alg_simd = {
> > + .base.cra_module = THIS_MODULE,
>
> This line is listed twice for "crypto_aegis128_alg_simd", but it's
> missing for "crypto_aegis128_alg_generic" - I think you meant to put
> this one three lines higher.
>
Oops. Will fix.
> > + .setkey = crypto_aegis128_setkey,
> > + .setauthsize = crypto_aegis128_setauthsize,
> > + .encrypt = crypto_aegis128_encrypt_simd,
> > + .decrypt = crypto_aegis128_decrypt_simd,
> > +
> > + .ivsize = AEGIS128_NONCE_SIZE,
> > + .maxauthsize = AEGIS128_MAX_AUTH_SIZE,
> > + .chunksize = AEGIS_BLOCK_SIZE,
> > +
> > + .base.cra_blocksize = 1,
> > + .base.cra_ctxsize = sizeof(struct aegis_ctx),
> > + .base.cra_alignmask = 0,
> > + .base.cra_priority = 200,
> > + .base.cra_name = "aegis128",
> > + .base.cra_driver_name = "aegis128-simd",
> > + .base.cra_module = THIS_MODULE,
> > };
> >
> > static int __init crypto_aegis128_module_init(void)
> > {
> > + int ret;
> > +
> > + ret = crypto_register_aead(&crypto_aegis128_alg_generic);
> > + if (ret)
> > + return ret;
> > +
> > if (IS_ENABLED(CONFIG_CRYPTO_AEGIS128_SIMD) &&
> > - crypto_aegis128_have_simd())
> > + crypto_aegis128_have_simd()) {
> > + ret = crypto_register_aead(&crypto_aegis128_alg_simd);
> > + if (ret) {
> > + crypto_unregister_aead(&crypto_aegis128_alg_generic);
> > + return ret;
> > + }
> > static_branch_enable(&have_simd);
> > -
> > - return crypto_register_aead(&crypto_aegis128_alg);
> > + }
> > + return 0;
> > }
> >
> > static void __exit crypto_aegis128_module_exit(void)
> > {
> > - crypto_unregister_aead(&crypto_aegis128_alg);
> > + if (IS_ENABLED(CONFIG_CRYPTO_AEGIS128_SIMD) &&
> > + crypto_aegis128_have_simd())
> > + crypto_unregister_aead(&crypto_aegis128_alg_simd);
> > +
> > + crypto_unregister_aead(&crypto_aegis128_alg_generic);
> > }
> >
> > subsys_initcall(crypto_aegis128_module_init);
> > @@ -528,3 +589,4 @@ MODULE_AUTHOR("Ondrej Mosnacek <omosnacek@xxxxxxxxx>");
> > MODULE_DESCRIPTION("AEGIS-128 AEAD algorithm");
> > MODULE_ALIAS_CRYPTO("aegis128");
> > MODULE_ALIAS_CRYPTO("aegis128-generic");
> > +MODULE_ALIAS_CRYPTO("aegis128-simd");
> > --
> > 2.17.1
> >
