Re: [Qemu-devel] [PATCH 02/13] qcrypto-luks: misc refactoring

[Max Reitz]

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 <address@hidden>
>>> ---
>>>  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