Re: [PATCH 1/4] crypto: add eboiv as a crypto API template
On Mon, Oct 26, 2020 at 8:26 PM Eric Biggers wrote:
>
> 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;
...
Thank you very much for the review and assistance. I will send out a
revised version.
Thanks,
Gilad
--
Gilad Ben-Yossef
Chief Coffee Drinker
values of β will give rise to dom!
Re: [PATCH 1/4] crypto: add eboiv as a crypto API template
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) {
Re: [PATCH 1/4] crypto: add eboiv as a crypto API template
On Mon, Oct 26, 2020 at 03:04:44PM +0200, Gilad Ben-Yossef wrote:
> diff --git a/crypto/eboiv.c b/crypto/eboiv.c
> new file mode 100644
> index ..467833e89139
> --- /dev/null
> +++ b/crypto/eboiv.c
> @@ -0,0 +1,295 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * EBOIV skcipher template for block encryption
> + *
> + * This template encapsulates the Encrypted byte-offset IV generation
> algorithm
> + * used in Bitlocker in CBC mode by dm-crypt, which converts the initial
> vector
> + * for the skcipher used for block encryption, by encrypting it using the
> same
> + * skcipher key as encryption key. Usually, the input IV is a 64-bit sector
> + * offset (the byte offset of the start of the sector) in LE representation
> + * zero-padded to the size of the IV, but this * is not assumed by this
> driver.
> + *
> + * The typical use of this template is to instantiate the skcipher
> + * 'eboiv(cbc(aes))', which is the only instantiation used by
> + * dm-crypt for supporting BitLocker AES-CBC mode as specified in
> + * https://www.jedec.org/sites/default/files/docs/JESD223C.pdf
> + *
> + * Copyright (C) 2020 ARM Limited or its affiliates.
> + * Written by Gilad Ben-Yossef
> + *
> + * Heavily based on:
> + *
> + * ESSIV skcipher and aead template for block encryption
> + * Copyright (c) 2019 Linaro, Ltd.
> + *
> + * and
> + *
> + * dm-crypt eboiv code
> + * by Milan Broz and Ard Biesheuvel
> + *
> + */
> +
> +#include
> +#include
> +#include
> +
> +#include "internal.h"
internal.h shouldn't be included here.
> +
> +struct eboiv_instance_ctx {
> + struct crypto_skcipher_spawn skcipher_spawn;
> + char eboiv_cipher_name[CRYPTO_MAX_ALG_NAME];
> +};
The 'eboiv_cipher_name' field isn't used.
> +static void eboiv_skcipher_iv_done(struct crypto_async_request *areq, int
> err)
> +{
> + struct eboiv_req_ctx *req_ctx = areq->data;
> + struct skcipher_request *req = req_ctx->req;
> + int ret;
> +
> + if (!err) {
> +
> + ret = eboiv_skcipher_do_crypt(req, req_ctx->enc);
> +
> + if ((ret == -EINPROGRESS) || (ret == -EBUSY))
> + return;
> + }
> +
> + skcipher_request_complete(req, err);
> +}
This looks wrong, because if eboiv_skcipher_do_crypt() fails,
skcipher_request_complete() will be called with err=0.
> +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.
> +/* eboiv(cipher_name) */
> +static struct crypto_template eboiv_tmpl = {
> + .name = "eboiv",
> + .create = eboiv_create,
> + .module = THIS_MODULE,
> +};
"cipher_name" => "cbc_cipher_name", si
