On Fri, 2019-09-06 at 14:34 +0100, Daniel P. Berrangé wrote:
> On Mon, Aug 26, 2019 at 04:51:03PM +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 doesn't really matter.
> >
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> > crypto/block-luks.c | 46 +++++++++++++++++++++++++++++++++++++++++++--
> > 1 file changed, 44 insertions(+), 2 deletions(-)
> >
> > diff --git a/crypto/block-luks.c b/crypto/block-luks.c
> > index db0fb764b4..fdf4c41f8a 100644
> > --- a/crypto/block-luks.c
> > +++ b/crypto/block-luks.c
> > @@ -541,12 +541,12 @@ fail:
> > static int
> > qcrypto_block_luks_check_header(const QCryptoBlockLUKS *luks, Error **errp)
> > {
> > - int ret;
> > + int ret = -EINVAL;
>
> As before, no need to use errnos, just return -1 immediately.
>
> > + size_t i, j;
> >
> > if (memcmp(luks->header.magic, qcrypto_block_luks_magic,
> > QCRYPTO_BLOCK_LUKS_MAGIC_LEN) != 0) {
> > error_setg(errp, "Volume is not in LUKS format");
> > - ret = -EINVAL;
> > goto fail;
> > }
> >
> > @@ -557,6 +557,48 @@ qcrypto_block_luks_check_header(const QCryptoBlockLUKS *luks, Error **errp)
> > goto fail;
> > }
> >
> > + /* Check all keyslots for corruption */
> > + for (i = 0 ; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS ; i++) {
> > +
> > + const QCryptoBlockLUKSKeySlot *slot1 = &luks->header.key_slots[i];
> > + unsigned int start1 = slot1->key_offset_sector;
> > + unsigned int len1 =
> > + qcrypto_block_luks_splitkeylen_sectors(luks, slot1->stripes);
> > +
> > + if (slot1->stripes == 0) {
> > + error_setg(errp, "Keyslot %zu is corrupted (stripes == 0)", i);
> > + goto fail;
> > + }
>
> How about checking stripes != QCRYPTO_BLOCK_LUKS_STRIPES because
> AFAIR, you're required to use 4k stripes in luks v1.
I see that spec does allow for user defined number of stripes.
>
> Also how about checking iters >= MIN_SLOT_KEY_ITERS
Also this is only a suggested minimum
>
> > +
> > + if (slot1->active != QCRYPTO_BLOCK_LUKS_KEY_SLOT_DISABLED &&
> > + slot1->active != QCRYPTO_BLOCK_LUKS_KEY_SLOT_ENABLED) {
>
> Align the two lines with (
Done
>
> > + error_setg(errp,
> > + "Keyslot %zu state (active/disable) is corrupted", i);
> > + goto fail;
> > + }
> > +
> > + if (start1 + len1 > luks->header.payload_offset_sector) {
> > + error_setg(errp,
> > + "Keyslot %zu is overlapping with the encrypted payload",
> > + i);
> > + goto fail;
> > + }
> > +
> > + for (j = i + 1 ; j < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS ; j++) {
> > + const QCryptoBlockLUKSKeySlot *slot2 = &luks->header.key_slots[j];
> > + unsigned int start2 = slot2->key_offset_sector;
> > + unsigned int len2 =
> > + qcrypto_block_luks_splitkeylen_sectors(luks, slot2->stripes);
> > +
> > + if (start1 + len1 > start2 && start2 + len2 > start1) {
> > + error_setg(errp,
> > + "Keyslots %zu and %zu are overlapping in the header",
> > + i, j);
> > + goto fail;
> > + }
> > + }
> > +
> > + }
> > return 0;
> > fail:
> > return ret;
> > --
> > 2.17.2
> >
>
> Regards,
> Daniel
Best regards,
Maxim Levitsky