Re: [PATCH 1/4] crypto: add eboiv as a crypto API template

[Eric Biggers]

On Mon, Oct 26, 2020 at 11:24:50AM -0700, Eric Biggers wrote:
> > +static int eboiv_create(struct crypto_template *tmpl, struct rtattr **tb)
> > +{
> > +   struct crypto_attr_type *algt;
> > +   const char *inner_cipher_name;
> > +   struct skcipher_instance *skcipher_inst = NULL;
> > +   struct crypto_instance *inst;
> > +   struct crypto_alg *base, *block_base;
> > +   struct eboiv_instance_ctx *ictx;
> > +   struct skcipher_alg *skcipher_alg = NULL;
> > +   int ivsize;
> > +   u32 mask;
> > +   int err;
> > +
> > +   algt = crypto_get_attr_type(tb);
> > +   if (IS_ERR(algt))
> > +           return PTR_ERR(algt);
> 
> Need to check that the algorithm is being instantiated as skcipher.
> crypto_check_attr_type() should be used.
> 
> > +
> > +   inner_cipher_name = crypto_attr_alg_name(tb[1]);
> > +   if (IS_ERR(inner_cipher_name))
> > +           return PTR_ERR(inner_cipher_name);
> 
> The result of crypto_attr_alg_name() can be passed directly to
> crypto_grab_skcipher().
> 
> > +   mask = crypto_algt_inherited_mask(algt);
> > +
> > +   skcipher_inst = kzalloc(sizeof(*skcipher_inst) + sizeof(*ictx), 
> > GFP_KERNEL);
> > +   if (!skcipher_inst)
> > +           return -ENOMEM;
> > +
> > +   inst = skcipher_crypto_instance(skcipher_inst);
> > +   base = &skcipher_inst->alg.base;
> > +   ictx = crypto_instance_ctx(inst);
> > +
> > +   /* Symmetric cipher, e.g., "cbc(aes)" */
> > +   err = crypto_grab_skcipher(&ictx->skcipher_spawn, inst, 
> > inner_cipher_name, 0, mask);
> > +   if (err)
> > +           goto out_free_inst;
> > +
> > +   skcipher_alg = crypto_spawn_skcipher_alg(&ictx->skcipher_spawn);
> > +   block_base = &skcipher_alg->base;
> > +   ivsize = crypto_skcipher_alg_ivsize(skcipher_alg);
> > +
> > +   if (ivsize != block_base->cra_blocksize)
> > +           goto out_drop_skcipher;
> 
> Shouldn't it be verified that the underlying algorithm is actually cbc?
> 
> > +   skcipher_inst->alg.chunksize    = 
> > crypto_skcipher_alg_chunksize(skcipher_alg);
> > +   skcipher_inst->alg.walksize     = 
> > crypto_skcipher_alg_walksize(skcipher_alg);
> 
> Setting these isn't necessary.
> 
> > +
> > +   skcipher_inst->free             = eboiv_skcipher_free_instance;
> > +
> > +   err = skcipher_register_instance(tmpl, skcipher_inst);
> > +
> > +   if (err)
> > +           goto out_drop_skcipher;
> > +
> > +   return 0;
> > +
> > +out_drop_skcipher:
> > +   crypto_drop_skcipher(&ictx->skcipher_spawn);
> > +out_free_inst:
> > +   kfree(skcipher_inst);
> > +   return err;
> > +}
> 
> eboiv_skcipher_free_instance() can be called on the error path.

Here's the version of eboiv_create() I recommend (untested):

static int eboiv_create(struct crypto_template *tmpl, struct rtattr **tb)
{
        struct skcipher_instance *inst;
        struct eboiv_instance_ctx *ictx;
        struct skcipher_alg *alg;
        u32 mask;
        int err;

        err = crypto_check_attr_type(tb, CRYPTO_ALG_TYPE_SKCIPHER, &mask);
        if (err)
                return err;

        inst = kzalloc(sizeof(*inst) + sizeof(*ictx), GFP_KERNEL);
        if (!inst)
                return -ENOMEM;
        ictx = skcipher_instance_ctx(inst);

        err = crypto_grab_skcipher(&ictx->skcipher_spawn,
                                   skcipher_crypto_instance(inst),
                                   crypto_attr_alg_name(tb[1]), 0, mask);
        if (err)
                goto err_free_inst;
        alg = crypto_spawn_skcipher_alg(&ictx->skcipher_spawn);

        err = -EINVAL;
        if (strncmp(alg->base.cra_name, "cbc(", 4) ||
            crypto_skcipher_alg_ivsize(alg) != alg->base.cra_blocksize)
                goto err_free_inst;

        err = -ENAMETOOLONG;
        if (snprintf(inst->alg.base.cra_name, CRYPTO_MAX_ALG_NAME, "eboiv(%s)",
                     alg->base.cra_name) >= CRYPTO_MAX_ALG_NAME)
                goto err_free_inst;

        if (snprintf(inst->alg.base.cra_driver_name, CRYPTO_MAX_ALG_NAME,
                     "eboiv(%s)", alg->base.cra_driver_name) >=
            CRYPTO_MAX_ALG_NAME)
                goto err_free_inst;

        inst->alg.base.cra_blocksize    = alg->base.cra_blocksize;
        inst->alg.base.cra_ctxsize      = sizeof(struct eboiv_tfm_ctx);
        inst->alg.base.cra_alignmask    = alg->base.cra_alignmask;
        inst->alg.base.cra_priority     = alg->base.cra_priority;

        inst->alg.setkey        = eboiv_skcipher_setkey;
        inst->alg.encrypt       = eboiv_skcipher_encrypt;
        inst->alg.decrypt       = eboiv_skcipher_decrypt;
        inst->alg.init          = eboiv_skcipher_init_tfm;
        inst->alg.exit          = eboiv_skcipher_exit_tfm;

        inst->alg.min_keysize   = crypto_skcipher_alg_min_keysize(alg);
        inst->alg.max_keysize   = crypto_skcipher_alg_max_keysize(alg);
        inst->alg.ivsize        = crypto_skcipher_alg_ivsize(alg);

        inst->free              = eboiv_skcipher_free_instance;

        err = skcipher_register_instance(tmpl, inst);
        if (err) {
err_free_inst:
                eboiv_skcipher_free_instance(inst);
        }
        return err;
}