Re: [Qemu-devel] [PATCH 06/13] qcrypto-luks: implement more rigorous header checking

[Daniel P . Berrangé]

On Wed, Aug 14, 2019 at 11:22:12PM +0300, Maxim Levitsky wrote:
> Check that keyslots don't overlap with the data,
> and check that keyslots don't overlap with each other.
> (this is done using naive O(n^2) nested loops,
> but since there are just 8 keyslots, this doens't really matter.
> 
> Signed-off-by: Maxim Levitsky <address@hidden>
> ---
>  crypto/block-luks.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/crypto/block-luks.c b/crypto/block-luks.c
> index 336e633df4..1997e92fe1 100644
> --- a/crypto/block-luks.c
> +++ b/crypto/block-luks.c
> @@ -551,6 +551,8 @@ static int
>  qcrypto_block_luks_check_header(QCryptoBlockLUKS *luks, Error **errp)
>  {
>      int ret;
> +    int i, j;
> +
>  
>      if (memcmp(luks->header.magic, qcrypto_block_luks_magic,
>                 QCRYPTO_BLOCK_LUKS_MAGIC_LEN) != 0) {
> @@ -566,6 +568,46 @@ qcrypto_block_luks_check_header(QCryptoBlockLUKS *luks, 
> Error **errp)
>          goto fail;
>      }
>  
> +    /* Check all keyslots for corruption  */
> +    for (i = 0 ; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS ; i++) {
> +
> +        QCryptoBlockLUKSKeySlot *slot1 = &luks->header.key_slots[i];
> +        uint start1 = slot1->key_offset;
> +        uint len1 = splitkeylen_sectors(luks, slot1->stripes);

Using 'uint' is not normal QEMU style.

Either use 'unsigned int'  or if a specific size is needed
then one of the 'guintNN' types from glib.

This applies elsewhere in this patch series too, but
I'll only comment here & let you find the other cases.

> +
> +        if (slot1->stripes == 0 ||
> +                (slot1->active != QCRYPTO_BLOCK_LUKS_KEY_SLOT_DISABLED &&
> +                slot1->active != QCRYPTO_BLOCK_LUKS_KEY_SLOT_ENABLED)) {
> +

Redundant blank line

> +            error_setg(errp, "Keyslot %i is corrupted", i);

I'd do a separate check for stripes and active fields, and then give a
specific error message for each. That way if this does ever trigger
in practice will immediately understand which check failed.

Also using '%d' rather than '%i' is more common convention


> +            ret = -EINVAL;
> +            goto fail;
> +        }
> +
> +        if (start1 + len1 > luks->header.payload_offset) {
> +            error_setg(errp,
> +                       "Keyslot %i is overlapping with the encrypted 
> payload",
> +                       i);
> +            ret = -EINVAL;
> +            goto fail;
> +        }
> +
> +        for (j = i + 1 ; j < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS ; j++) {
> +

Redundant blank

> +            QCryptoBlockLUKSKeySlot *slot2 = &luks->header.key_slots[j];
> +            uint start2 = slot2->key_offset;
> +            uint len2 = splitkeylen_sectors(luks, slot2->stripes);
> +
> +            if (start1 + len1 > start2 && start2 + len2 > start1) {
> +                error_setg(errp,
> +                           "Keyslots %i and %i are overlapping in the 
> header",

%d

> +                           i, j);
> +                ret = -EINVAL;
> +                goto fail;
> +            }
> +        }
> +
> +    }
>      return 0;
>  fail:
>      return ret;
> -- 
> 2.17.2
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|