Re: [Qemu-devel] [PATCH 03/10] qcrypto-luks: implement the encryption key management
On Fri, 2019-09-06 at 14:55 +0100, Daniel P. Berrangé wrote:
> On Fri, Aug 30, 2019 at 11:56:01PM +0300, Maxim Levitsky wrote:
> > Signed-off-by: Maxim Levitsky
> > ---
> > crypto/block-luks.c | 366 +++-
> > 1 file changed, 364 insertions(+), 2 deletions(-)
> >
> > diff --git a/crypto/block-luks.c b/crypto/block-luks.c
> > index ba20d55246..21325fbc79 100644
> > --- a/crypto/block-luks.c
> > +++ b/crypto/block-luks.c
> > @@ -70,6 +70,9 @@ typedef struct QCryptoBlockLUKSKeySlot
> > QCryptoBlockLUKSKeySlot;
> >
> > #define QCRYPTO_BLOCK_LUKS_SECTOR_SIZE 512LL
> >
> > +#define QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME 2000
>
> Perhaps use ITER_TIME_MS to make it clear it is millisecs
Why not... done.
>
> > +#define QCRYPTO_BLOCK_LUKS_ERASE_ITERATIONS 40
> > +
> > static const char qcrypto_block_luks_magic[QCRYPTO_BLOCK_LUKS_MAGIC_LEN] =
> > {
> > 'L', 'U', 'K', 'S', 0xBA, 0xBE
> > };
> > @@ -219,6 +222,9 @@ struct QCryptoBlockLUKS {
> >
> > /* Hash algorithm used in pbkdf2 function */
> > QCryptoHashAlgorithm hash_alg;
> > +
> > +/* Name of the secret that was used to open the image */
> > +char *secret;
> > +
> > +/*
> > + * Returns true if a slot i is marked as active
> > + * (contains encrypted copy of the master key)
> > + */
> > +
> > +static bool
>
> No blank line is wanted between the comment & function.
> Likewise for the rest of this patch series
No problem!
>
> > +qcrypto_block_luks_slot_active(const QCryptoBlockLUKS *luks,
> > + unsigned int slot_idx)
> > +{
> > +uint32_t val = luks->header.key_slots[slot_idx].active;
> > +return val == QCRYPTO_BLOCK_LUKS_KEY_SLOT_ENABLED;
> > +}
>
>
>
> > +static int
> > +qcrypto_block_luks_erase_key(QCryptoBlock *block,
> > + unsigned int slot_idx,
> > + QCryptoBlockWriteFunc writefunc,
> > + void *opaque,
> > + Error **errp)
> > +{
> > +QCryptoBlockLUKS *luks = block->opaque;
> > +QCryptoBlockLUKSKeySlot *slot = >header.key_slots[slot_idx];
> > +g_autofree uint8_t *garbagesplitkey = NULL;
> > +size_t splitkeylen = luks->header.master_key_len * slot->stripes;
> > +size_t i;
> > +
> > +assert(slot_idx < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS);
> > +assert(splitkeylen > 0);
> > +
> > +garbagesplitkey = g_malloc0(splitkeylen);
>
> I'd prefer g_new0(uint8_t, splitkeylen)
Done.
>
> > +
> > +/* Reset the key slot header */
> > +memset(slot->salt, 0, QCRYPTO_BLOCK_LUKS_SALT_LEN);
> > +slot->iterations = 0;
> > +slot->active = QCRYPTO_BLOCK_LUKS_KEY_SLOT_DISABLED;
> > +
>
>
> > @@ -1522,6 +1700,187 @@ qcrypto_block_luks_create(QCryptoBlock *block,
> > }
> >
> >
> > +#define CHECK_NON_AMEND_OPTION(luks, luks_opts, name) \
> > +if (luks_opts.has_##name && luks_opts.name != luks->name) { \
> > +error_setg(errp, "Option \"" #name "\" can't be amended"); \
> > +goto cleanup; \
> > +}
> > +
> > +static int
> > +qcrypto_block_luks_amend_options(QCryptoBlock *block,
> > + QCryptoBlockReadFunc readfunc,
> > + QCryptoBlockWriteFunc writefunc,
> > + void *opaque,
> > + QCryptoBlockCreateOptions *options,
> > + bool force,
> > + Error **errp)
> > +{
> > +QCryptoBlockLUKS *luks = block->opaque;
> > +QCryptoBlockCreateOptionsLUKS luks_opts;
> > +g_autofree char *old_password = NULL;
> > +g_autofree char *password = NULL;
> > +const char *unlock_secret = luks->secret;
> > +g_autofree uint8_t *masterkey = NULL;
> > +int slot = -1;
> > +int ret = -1;
> > +bool active = true;
> > +int64_t iter_time = QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME;
> > +
> > +memcpy(_opts, >u.luks, sizeof(luks_opts));
> > +
> > +CHECK_NON_AMEND_OPTION(luks, luks_opts, cipher_alg);
> > +CHECK_NON_AMEND_OPTION(luks, luks_opts, cipher_mode);
> > +CHECK_NON_AMEND_OPTION(luks, luks_opts, ivgen_alg);
> > +CHECK_NON_AMEND_OPTION(luks, luks_opts, ivgen_hash_alg);
> > +CHECK_NON_AMEND_OPTION(luks, luks_opts, hash_alg);
> > +
> > +/* Read given slot and check it */
> > +if (luks_opts.has_slot) {
> > +slot = luks_opts.slot;
> > +if (slot < 0 || slot >= QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS) {
> > +error_setg(errp,
> > + "Given key slot %i is not supported by LUKS", slot);
> > + goto cleanup;
>
> Off by 1 one indent
Sorry about that.
>
> > +}
> > +}
> > +
> > +if (luks_opts.has_iter_time) {
> > +iter_time = luks_opts.iter_time;
> > +}
> > +
> > +if (luks_opts.has_active && luks_opts.active == false) {
> > +active = false;
> > +}
> > +
> > +if
Re: [Qemu-devel] [PATCH 03/10] qcrypto-luks: implement the encryption key management
On Fri, Aug 30, 2019 at 11:56:01PM +0300, Maxim Levitsky wrote:
> Signed-off-by: Maxim Levitsky
> ---
> crypto/block-luks.c | 366 +++-
> 1 file changed, 364 insertions(+), 2 deletions(-)
>
> diff --git a/crypto/block-luks.c b/crypto/block-luks.c
> index ba20d55246..21325fbc79 100644
> --- a/crypto/block-luks.c
> +++ b/crypto/block-luks.c
> @@ -70,6 +70,9 @@ typedef struct QCryptoBlockLUKSKeySlot
> QCryptoBlockLUKSKeySlot;
>
> #define QCRYPTO_BLOCK_LUKS_SECTOR_SIZE 512LL
>
> +#define QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME 2000
Perhaps use ITER_TIME_MS to make it clear it is millisecs
> +#define QCRYPTO_BLOCK_LUKS_ERASE_ITERATIONS 40
> +
> static const char qcrypto_block_luks_magic[QCRYPTO_BLOCK_LUKS_MAGIC_LEN] = {
> 'L', 'U', 'K', 'S', 0xBA, 0xBE
> };
> @@ -219,6 +222,9 @@ struct QCryptoBlockLUKS {
>
> /* Hash algorithm used in pbkdf2 function */
> QCryptoHashAlgorithm hash_alg;
> +
> +/* Name of the secret that was used to open the image */
> +char *secret;
> +
> +/*
> + * Returns true if a slot i is marked as active
> + * (contains encrypted copy of the master key)
> + */
> +
> +static bool
No blank line is wanted between the comment & function.
Likewise for the rest of this patch series
> +qcrypto_block_luks_slot_active(const QCryptoBlockLUKS *luks,
> + unsigned int slot_idx)
> +{
> +uint32_t val = luks->header.key_slots[slot_idx].active;
> +return val == QCRYPTO_BLOCK_LUKS_KEY_SLOT_ENABLED;
> +}
> +static int
> +qcrypto_block_luks_erase_key(QCryptoBlock *block,
> + unsigned int slot_idx,
> + QCryptoBlockWriteFunc writefunc,
> + void *opaque,
> + Error **errp)
> +{
> +QCryptoBlockLUKS *luks = block->opaque;
> +QCryptoBlockLUKSKeySlot *slot = >header.key_slots[slot_idx];
> +g_autofree uint8_t *garbagesplitkey = NULL;
> +size_t splitkeylen = luks->header.master_key_len * slot->stripes;
> +size_t i;
> +
> +assert(slot_idx < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS);
> +assert(splitkeylen > 0);
> +
> +garbagesplitkey = g_malloc0(splitkeylen);
I'd prefer g_new0(uint8_t, splitkeylen)
> +
> +/* Reset the key slot header */
> +memset(slot->salt, 0, QCRYPTO_BLOCK_LUKS_SALT_LEN);
> +slot->iterations = 0;
> +slot->active = QCRYPTO_BLOCK_LUKS_KEY_SLOT_DISABLED;
> +
> @@ -1522,6 +1700,187 @@ qcrypto_block_luks_create(QCryptoBlock *block,
> }
>
>
> +#define CHECK_NON_AMEND_OPTION(luks, luks_opts, name) \
> +if (luks_opts.has_##name && luks_opts.name != luks->name) { \
> +error_setg(errp, "Option \"" #name "\" can't be amended"); \
> +goto cleanup; \
> +}
> +
> +static int
> +qcrypto_block_luks_amend_options(QCryptoBlock *block,
> + QCryptoBlockReadFunc readfunc,
> + QCryptoBlockWriteFunc writefunc,
> + void *opaque,
> + QCryptoBlockCreateOptions *options,
> + bool force,
> + Error **errp)
> +{
> +QCryptoBlockLUKS *luks = block->opaque;
> +QCryptoBlockCreateOptionsLUKS luks_opts;
> +g_autofree char *old_password = NULL;
> +g_autofree char *password = NULL;
> +const char *unlock_secret = luks->secret;
> +g_autofree uint8_t *masterkey = NULL;
> +int slot = -1;
> +int ret = -1;
> +bool active = true;
> +int64_t iter_time = QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME;
> +
> +memcpy(_opts, >u.luks, sizeof(luks_opts));
> +
> +CHECK_NON_AMEND_OPTION(luks, luks_opts, cipher_alg);
> +CHECK_NON_AMEND_OPTION(luks, luks_opts, cipher_mode);
> +CHECK_NON_AMEND_OPTION(luks, luks_opts, ivgen_alg);
> +CHECK_NON_AMEND_OPTION(luks, luks_opts, ivgen_hash_alg);
> +CHECK_NON_AMEND_OPTION(luks, luks_opts, hash_alg);
> +
> +/* Read given slot and check it */
> +if (luks_opts.has_slot) {
> +slot = luks_opts.slot;
> +if (slot < 0 || slot >= QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS) {
> +error_setg(errp,
> + "Given key slot %i is not supported by LUKS", slot);
> + goto cleanup;
Off by 1 one indent
> +}
> +}
> +
> +if (luks_opts.has_iter_time) {
> +iter_time = luks_opts.iter_time;
> +}
> +
> +if (luks_opts.has_active && luks_opts.active == false) {
> +active = false;
> +}
> +
> +if (active) {
> +
> +/* Check that we are not overwriting an active slot */
> +if (!force && slot != -1 &&
> +qcrypto_block_luks_slot_active(luks, slot)) {
> +
> +error_setg(errp, "Can't update an active key slot %i",
> + slot);
> +goto cleanup;
> +}
> +
> +/* check that we have the passwords*/
> +if
