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

[Maxim Levitsky]

On Thu, 2019-08-22 at 12:04 +0100, Daniel P. Berrangé wrote:
> 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.

Fixed. Sorry for the noise.

> 
> > +
> > +        if (slot1->stripes == 0 ||
> > +                (slot1->active != QCRYPTO_BLOCK_LUKS_KEY_SLOT_DISABLED &&
> > +                slot1->active != QCRYPTO_BLOCK_LUKS_KEY_SLOT_ENABLED)) {
> > +
> 
> Redundant blank line
Fixed
> 
> > +            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
Done.
> 
> 
> > +            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
Fixed.
> 
> > +                           i, j);
> > +                ret = -EINVAL;
> > +                goto fail;
> > +            }
> > +        }
> > +
> > +    }
> >      return 0;
> >  fail:
> >      return ret;
> > -- 
> > 2.17.2
> > 
> 
> Regards,
> Daniel


Best regards,
        Maxim Levitsky