[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 06/13] qcrypto-luks: implement more rigorous hea
From: |
Maxim Levitsky |
Subject: |
Re: [Qemu-devel] [PATCH 06/13] qcrypto-luks: implement more rigorous header checking |
Date: |
Sun, 25 Aug 2019 19:08:00 +0300 |
On Sun, 2019-08-25 at 18:40 +0300, Maxim Levitsky wrote:
> 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.
Note that I switched i,j to be size_t since you said that you prefer this,
and to print this I apparently need %lu.
[...]
Best regards,
Maxim Levitsky
Re: [Qemu-devel] [PATCH 03/13] qcrypto-luks: refactoring: extract load/store/check/parse header functions, Daniel P . Berrangé, 2019/08/22
[Qemu-devel] [PATCH 06/13] qcrypto-luks: implement more rigorous header checking, Maxim Levitsky, 2019/08/14
[Qemu-devel] [PATCH 08/13] qcrypto: add the plumbing for encryption management, Maxim Levitsky, 2019/08/14
[Qemu-devel] [PATCH 09/13] qcrypto-luks: implement the encryption key management, Maxim Levitsky, 2019/08/14