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 <mlevi...@redhat.com> > --- > 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 :|