Re: [Qemu-devel] [PATCH 02/13] qcrypto-luks: misc refactoring
On Thu, 2019-08-22 at 16:32 +0200, Max Reitz wrote: > On 22.08.19 01:59, Maxim Levitsky wrote: > > On Tue, 2019-08-20 at 19:36 +0200, Max Reitz wrote: > > > On 14.08.19 22:22, Maxim Levitsky wrote: > > > > This is also a preparation for key read/write/erase functions > > > > > > > > * use master key len from the header > > > > * prefer to use crypto params in the QCryptoBlockLUKS > > > > over passing them as function arguments > > > > * define QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME > > > > * Add comments to various crypto parameters in the QCryptoBlockLUKS > > > > > > > > Signed-off-by: Maxim Levitsky > > > > --- > > > > crypto/block-luks.c | 213 ++-- > > > > 1 file changed, 105 insertions(+), 108 deletions(-) > > > [...] > > > > > @@ -410,21 +430,15 @@ > > > > qcrypto_block_luks_essiv_cipher(QCryptoCipherAlgorithm cipher, > > > > */ > > > > static int > > > > qcrypto_block_luks_load_key(QCryptoBlock *block, > > > > -QCryptoBlockLUKSKeySlot *slot, > > > > +uint slot_idx, > > > > > > Did you use uint on purpose or do you mean a plain “unsigned”? > > > > Well there are just 8 slots, but yea I don't mind to make this an unsigned > > int. > > My point was that “uint” is not a standard C type. There are lot of non standard types, like the u8/u8/u32/u64 I used to use in the kernel, so I kind of missed that. Won't use that type anymore :-) > > [...] > > > > > @@ -930,6 +922,17 @@ qcrypto_block_luks_create(QCryptoBlock *block, > > > > luks_opts.has_ivgen_hash_alg = true; > > > > } > > > > } > > > > + > > > > +luks = g_new0(QCryptoBlockLUKS, 1); > > > > +block->opaque = luks; > > > > + > > > > +luks->cipher_alg = luks_opts.cipher_alg; > > > > +luks->cipher_mode = luks_opts.cipher_mode; > > > > +luks->ivgen_alg = luks_opts.ivgen_alg; > > > > +luks->ivgen_hash_alg = luks_opts.ivgen_hash_alg; > > > > +luks->hash_alg = luks_opts.hash_alg; > > > > + > > > > + > > > > > > Why did you pull this up? Now @luks is leaked in both of the next error > > > paths. > > > > This is because the purpose of these fields changed. As Daniel explained to > > me > > they were initially added after the fact to serve as a cache of into to > > present in qemu-img info callback. > > But now I use these everywhere in the code so I won't them to be correct as > > soon as possible to minimize > > the risk of calling some function that uses these and would read garbage. > > I get that, but I was wondering why you pulled the allocation of @luks > up above the next two conditional blocks. Allocating and initializing > there should have worked just fine. Yea, I didn't have to, just thought that putting the initialization as above as possible is a good thing for future. > > Max > Best regards, Maxim Levitsky
Re: [Qemu-devel] [PATCH 02/13] qcrypto-luks: misc refactoring
On 22.08.19 01:59, Maxim Levitsky wrote: > On Tue, 2019-08-20 at 19:36 +0200, Max Reitz wrote: >> On 14.08.19 22:22, Maxim Levitsky wrote: >>> This is also a preparation for key read/write/erase functions >>> >>> * use master key len from the header >>> * prefer to use crypto params in the QCryptoBlockLUKS >>> over passing them as function arguments >>> * define QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME >>> * Add comments to various crypto parameters in the QCryptoBlockLUKS >>> >>> Signed-off-by: Maxim Levitsky >>> --- >>> crypto/block-luks.c | 213 ++-- >>> 1 file changed, 105 insertions(+), 108 deletions(-) [...] >>> @@ -410,21 +430,15 @@ >>> qcrypto_block_luks_essiv_cipher(QCryptoCipherAlgorithm cipher, >>> */ >>> static int >>> qcrypto_block_luks_load_key(QCryptoBlock *block, >>> -QCryptoBlockLUKSKeySlot *slot, >>> +uint slot_idx, >> >> Did you use uint on purpose or do you mean a plain “unsigned”? > Well there are just 8 slots, but yea I don't mind to make this an unsigned > int. My point was that “uint” is not a standard C type. [...] >>> @@ -930,6 +922,17 @@ qcrypto_block_luks_create(QCryptoBlock *block, >>> luks_opts.has_ivgen_hash_alg = true; >>> } >>> } >>> + >>> +luks = g_new0(QCryptoBlockLUKS, 1); >>> +block->opaque = luks; >>> + >>> +luks->cipher_alg = luks_opts.cipher_alg; >>> +luks->cipher_mode = luks_opts.cipher_mode; >>> +luks->ivgen_alg = luks_opts.ivgen_alg; >>> +luks->ivgen_hash_alg = luks_opts.ivgen_hash_alg; >>> +luks->hash_alg = luks_opts.hash_alg; >>> + >>> + >> >> Why did you pull this up? Now @luks is leaked in both of the next error >> paths. > > This is because the purpose of these fields changed. As Daniel explained to me > they were initially added after the fact to serve as a cache of into to > present in qemu-img info callback. > But now I use these everywhere in the code so I won't them to be correct as > soon as possible to minimize > the risk of calling some function that uses these and would read garbage. I get that, but I was wondering why you pulled the allocation of @luks up above the next two conditional blocks. Allocating and initializing there should have worked just fine. Max signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH 02/13] qcrypto-luks: misc refactoring
On Tue, 2019-08-20 at 19:36 +0200, Max Reitz wrote:
> On 14.08.19 22:22, Maxim Levitsky wrote:
> > This is also a preparation for key read/write/erase functions
> >
> > * use master key len from the header
> > * prefer to use crypto params in the QCryptoBlockLUKS
> > over passing them as function arguments
> > * define QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME
> > * Add comments to various crypto parameters in the QCryptoBlockLUKS
> >
> > Signed-off-by: Maxim Levitsky
> > ---
> > crypto/block-luks.c | 213 ++--
> > 1 file changed, 105 insertions(+), 108 deletions(-)
> >
> > diff --git a/crypto/block-luks.c b/crypto/block-luks.c
> > index 409ab50f20..48213abde7 100644
> > --- a/crypto/block-luks.c
> > +++ b/crypto/block-luks.c
>
> [...]
>
> > @@ -199,13 +201,25 @@ QEMU_BUILD_BUG_ON(sizeof(struct
> > QCryptoBlockLUKSHeader) != 592);
> > struct QCryptoBlockLUKS {
> > QCryptoBlockLUKSHeader header;
> >
> > -/* Cache parsed versions of what's in header fields,
> > - * as we can't rely on QCryptoBlock.cipher being
> > - * non-NULL */
>
> Hm, why remove this comment?
Because the intended uses of these fields changed.
As Daniel explained to me initially none of the crypto parameters
were stored in the QCryptoBlockLUKS, and thus there were all passed
as function arguments.
Later when qemu-img info was added/implemented, there was need to 'cache' them
in the header so that info command could use them after image was opened.
Now after my changes this is no longer true. now these crypto parameters are
set early
on create/load and used everywhere to avoid passing them over and over to each
function.
>
> > +/* Main encryption algorithm used for encryption*/
> > QCryptoCipherAlgorithm cipher_alg;
> > +
> > +/* Mode of encryption for the selected encryption algorithm */
> > QCryptoCipherMode cipher_mode;
> > +
> > +/* Initialization vector generation algorithm */
> > QCryptoIVGenAlgorithm ivgen_alg;
> > +
> > +/* Hash algorithm used for IV generation*/
> > QCryptoHashAlgorithm ivgen_hash_alg;
> > +
> > +/*
> > + * Encryption algorithm used for IV generation.
> > + * Usually the same as main encryption algorithm
> > + */
> > +QCryptoCipherAlgorithm ivgen_cipher_alg;
> > +
> > +/* Hash algorithm used in pbkdf2 function */
> > QCryptoHashAlgorithm hash_alg;
> > };
> >
> > @@ -397,6 +411,12 @@ qcrypto_block_luks_essiv_cipher(QCryptoCipherAlgorithm
> > cipher,
> > }
> > }
> >
> > +static int masterkeylen(QCryptoBlockLUKS *luks)
>
> This should be a const pointer.
I haven't had const in mind while writing this but you are right.
Fixed.
>
> > +{
> > +return luks->header.key_bytes;
> > +}
> > +
> > +
> > /*
> > * Given a key slot, and user password, this will attempt to unlock
> > * the master encryption key from the key slot.
> > @@ -410,21 +430,15 @@
> > qcrypto_block_luks_essiv_cipher(QCryptoCipherAlgorithm cipher,
> > */
> > static int
> > qcrypto_block_luks_load_key(QCryptoBlock *block,
> > -QCryptoBlockLUKSKeySlot *slot,
> > +uint slot_idx,
>
> Did you use uint on purpose or do you mean a plain “unsigned”?
Well there are just 8 slots, but yea I don't mind to make this an unsigned int.
>
> > const char *password,
> > -QCryptoCipherAlgorithm cipheralg,
> > -QCryptoCipherMode ciphermode,
> > -QCryptoHashAlgorithm hash,
> > -QCryptoIVGenAlgorithm ivalg,
> > -QCryptoCipherAlgorithm ivcipheralg,
> > -QCryptoHashAlgorithm ivhash,
> > uint8_t *masterkey,
> > -size_t masterkeylen,
> > QCryptoBlockReadFunc readfunc,
> > void *opaque,
> > Error **errp)
> > {
> > QCryptoBlockLUKS *luks = block->opaque;
> > +QCryptoBlockLUKSKeySlot *slot = >header.key_slots[slot_idx];
>
> I think this is a great opportunity to make this a const pointer.
Agree. Done.
>
> > uint8_t *splitkey;
> > size_t splitkeylen;
> > uint8_t *possiblekey;
>
> [...]
>
> > @@ -710,6 +696,8 @@ qcrypto_block_luks_open(QCryptoBlock *block,
> > goto fail;
> > }
> >
> > +cipher_mode = g_strdup(luks->header.cipher_mode);
> > +
>
> This should be freed under the fail label.
>
> (And maybe the fact that this no longer modifies
> luks->header.cipher_mode should be mentioned in the commit message, I
> don’t know.)
I swear I documented that in the commit message... yea in the next commit (:-()
Fixed that now.
>
> > /*
> > * The cipher_mode header contains a string that we have
> > * to further parse, of the format
>
> [...]
>
> > @@ -730,13 +718,13 @@
Re: [Qemu-devel] [PATCH 02/13] qcrypto-luks: misc refactoring
On 14.08.19 22:22, Maxim Levitsky wrote:
> This is also a preparation for key read/write/erase functions
>
> * use master key len from the header
> * prefer to use crypto params in the QCryptoBlockLUKS
> over passing them as function arguments
> * define QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME
> * Add comments to various crypto parameters in the QCryptoBlockLUKS
>
> Signed-off-by: Maxim Levitsky
> ---
> crypto/block-luks.c | 213 ++--
> 1 file changed, 105 insertions(+), 108 deletions(-)
>
> diff --git a/crypto/block-luks.c b/crypto/block-luks.c
> index 409ab50f20..48213abde7 100644
> --- a/crypto/block-luks.c
> +++ b/crypto/block-luks.c
[...]
> @@ -199,13 +201,25 @@ QEMU_BUILD_BUG_ON(sizeof(struct QCryptoBlockLUKSHeader)
> != 592);
> struct QCryptoBlockLUKS {
> QCryptoBlockLUKSHeader header;
>
> -/* Cache parsed versions of what's in header fields,
> - * as we can't rely on QCryptoBlock.cipher being
> - * non-NULL */
Hm, why remove this comment?
> +/* Main encryption algorithm used for encryption*/
> QCryptoCipherAlgorithm cipher_alg;
> +
> +/* Mode of encryption for the selected encryption algorithm */
> QCryptoCipherMode cipher_mode;
> +
> +/* Initialization vector generation algorithm */
> QCryptoIVGenAlgorithm ivgen_alg;
> +
> +/* Hash algorithm used for IV generation*/
> QCryptoHashAlgorithm ivgen_hash_alg;
> +
> +/*
> + * Encryption algorithm used for IV generation.
> + * Usually the same as main encryption algorithm
> + */
> +QCryptoCipherAlgorithm ivgen_cipher_alg;
> +
> +/* Hash algorithm used in pbkdf2 function */
> QCryptoHashAlgorithm hash_alg;
> };
>
> @@ -397,6 +411,12 @@ qcrypto_block_luks_essiv_cipher(QCryptoCipherAlgorithm
> cipher,
> }
> }
>
> +static int masterkeylen(QCryptoBlockLUKS *luks)
This should be a const pointer.
> +{
> +return luks->header.key_bytes;
> +}
> +
> +
> /*
> * Given a key slot, and user password, this will attempt to unlock
> * the master encryption key from the key slot.
> @@ -410,21 +430,15 @@ qcrypto_block_luks_essiv_cipher(QCryptoCipherAlgorithm
> cipher,
> */
> static int
> qcrypto_block_luks_load_key(QCryptoBlock *block,
> -QCryptoBlockLUKSKeySlot *slot,
> +uint slot_idx,
Did you use uint on purpose or do you mean a plain “unsigned”?
> const char *password,
> -QCryptoCipherAlgorithm cipheralg,
> -QCryptoCipherMode ciphermode,
> -QCryptoHashAlgorithm hash,
> -QCryptoIVGenAlgorithm ivalg,
> -QCryptoCipherAlgorithm ivcipheralg,
> -QCryptoHashAlgorithm ivhash,
> uint8_t *masterkey,
> -size_t masterkeylen,
> QCryptoBlockReadFunc readfunc,
> void *opaque,
> Error **errp)
> {
> QCryptoBlockLUKS *luks = block->opaque;
> +QCryptoBlockLUKSKeySlot *slot = >header.key_slots[slot_idx];
I think this is a great opportunity to make this a const pointer.
> uint8_t *splitkey;
> size_t splitkeylen;
> uint8_t *possiblekey;
[...]
> @@ -710,6 +696,8 @@ qcrypto_block_luks_open(QCryptoBlock *block,
> goto fail;
> }
>
> +cipher_mode = g_strdup(luks->header.cipher_mode);
> +
This should be freed under the fail label.
(And maybe the fact that this no longer modifies
luks->header.cipher_mode should be mentioned in the commit message, I
don’t know.)
> /*
> * The cipher_mode header contains a string that we have
> * to further parse, of the format
[...]
> @@ -730,13 +718,13 @@ qcrypto_block_luks_open(QCryptoBlock *block,
>
> ivhash_name = strchr(ivgen_name, ':');
> if (!ivhash_name) {
> -ivhash = 0;
> +luks->ivgen_hash_alg = 0;
*luks is initialized to 0 anyway, but it doesn’t hurt, of course.
> } else {
> *ivhash_name = '\0';
> ivhash_name++;
>
> -ivhash = qcrypto_block_luks_hash_name_lookup(ivhash_name,
> - _err);
> +luks->ivgen_hash_alg =
> qcrypto_block_luks_hash_name_lookup(ivhash_name,
> +
> _err);
> if (local_err) {
> ret = -ENOTSUP;
> error_propagate(errp, local_err);
> @@ -744,25 +732,27 @@ qcrypto_block_luks_open(QCryptoBlock *block,
[...]
>
> -hash = qcrypto_block_luks_hash_name_lookup(luks->header.hash_spec,
> +luks->hash_alg =
> +qcrypto_block_luks_hash_name_lookup(luks->header.hash_spec,
> _err);
Indentation is off now.
> if (local_err) {
> ret =
[Qemu-devel] [PATCH 02/13] qcrypto-luks: misc refactoring
This is also a preparation for key read/write/erase functions
* use master key len from the header
* prefer to use crypto params in the QCryptoBlockLUKS
over passing them as function arguments
* define QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME
* Add comments to various crypto parameters in the QCryptoBlockLUKS
Signed-off-by: Maxim Levitsky
---
crypto/block-luks.c | 213 ++--
1 file changed, 105 insertions(+), 108 deletions(-)
diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index 409ab50f20..48213abde7 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -70,6 +70,8 @@ typedef struct QCryptoBlockLUKSKeySlot
QCryptoBlockLUKSKeySlot;
#define QCRYPTO_BLOCK_LUKS_SECTOR_SIZE 512LL
+#define QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME 2000
+
static const char qcrypto_block_luks_magic[QCRYPTO_BLOCK_LUKS_MAGIC_LEN] = {
'L', 'U', 'K', 'S', 0xBA, 0xBE
};
@@ -199,13 +201,25 @@ QEMU_BUILD_BUG_ON(sizeof(struct QCryptoBlockLUKSHeader)
!= 592);
struct QCryptoBlockLUKS {
QCryptoBlockLUKSHeader header;
-/* Cache parsed versions of what's in header fields,
- * as we can't rely on QCryptoBlock.cipher being
- * non-NULL */
+/* Main encryption algorithm used for encryption*/
QCryptoCipherAlgorithm cipher_alg;
+
+/* Mode of encryption for the selected encryption algorithm */
QCryptoCipherMode cipher_mode;
+
+/* Initialization vector generation algorithm */
QCryptoIVGenAlgorithm ivgen_alg;
+
+/* Hash algorithm used for IV generation*/
QCryptoHashAlgorithm ivgen_hash_alg;
+
+/*
+ * Encryption algorithm used for IV generation.
+ * Usually the same as main encryption algorithm
+ */
+QCryptoCipherAlgorithm ivgen_cipher_alg;
+
+/* Hash algorithm used in pbkdf2 function */
QCryptoHashAlgorithm hash_alg;
};
@@ -397,6 +411,12 @@ qcrypto_block_luks_essiv_cipher(QCryptoCipherAlgorithm
cipher,
}
}
+static int masterkeylen(QCryptoBlockLUKS *luks)
+{
+return luks->header.key_bytes;
+}
+
+
/*
* Given a key slot, and user password, this will attempt to unlock
* the master encryption key from the key slot.
@@ -410,21 +430,15 @@ qcrypto_block_luks_essiv_cipher(QCryptoCipherAlgorithm
cipher,
*/
static int
qcrypto_block_luks_load_key(QCryptoBlock *block,
-QCryptoBlockLUKSKeySlot *slot,
+uint slot_idx,
const char *password,
-QCryptoCipherAlgorithm cipheralg,
-QCryptoCipherMode ciphermode,
-QCryptoHashAlgorithm hash,
-QCryptoIVGenAlgorithm ivalg,
-QCryptoCipherAlgorithm ivcipheralg,
-QCryptoHashAlgorithm ivhash,
uint8_t *masterkey,
-size_t masterkeylen,
QCryptoBlockReadFunc readfunc,
void *opaque,
Error **errp)
{
QCryptoBlockLUKS *luks = block->opaque;
+QCryptoBlockLUKSKeySlot *slot = >header.key_slots[slot_idx];
uint8_t *splitkey;
size_t splitkeylen;
uint8_t *possiblekey;
@@ -439,9 +453,9 @@ qcrypto_block_luks_load_key(QCryptoBlock *block,
return 0;
}
-splitkeylen = masterkeylen * slot->stripes;
+splitkeylen = masterkeylen(luks) * slot->stripes;
splitkey = g_new0(uint8_t, splitkeylen);
-possiblekey = g_new0(uint8_t, masterkeylen);
+possiblekey = g_new0(uint8_t, masterkeylen(luks));
/*
* The user password is used to generate a (possible)
@@ -450,11 +464,11 @@ qcrypto_block_luks_load_key(QCryptoBlock *block,
* the key is correct and validate the results of
* decryption later.
*/
-if (qcrypto_pbkdf2(hash,
+if (qcrypto_pbkdf2(luks->hash_alg,
(const uint8_t *)password, strlen(password),
slot->salt, QCRYPTO_BLOCK_LUKS_SALT_LEN,
slot->iterations,
- possiblekey, masterkeylen,
+ possiblekey, masterkeylen(luks),
errp) < 0) {
goto cleanup;
}
@@ -478,19 +492,19 @@ qcrypto_block_luks_load_key(QCryptoBlock *block,
/* Setup the cipher/ivgen that we'll use to try to decrypt
* the split master key material */
-cipher = qcrypto_cipher_new(cipheralg, ciphermode,
-possiblekey, masterkeylen,
+cipher = qcrypto_cipher_new(luks->cipher_alg, luks->cipher_mode,
+possiblekey, masterkeylen(luks),
errp);
if (!cipher) {
goto cleanup;
}
-niv = qcrypto_cipher_get_iv_len(cipheralg,
-ciphermode);
-ivgen = qcrypto_ivgen_new(ivalg,
- ivcipheralg,
-
