| Message ID | 20190826135103.22410-14-mlevitsk@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | RFC crypto/luks: preparation for encryption key managment | expand |
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. Also how about checking iters >= MIN_SLOT_KEY_ITERS > + > + if (slot1->active != QCRYPTO_BLOCK_LUKS_KEY_SLOT_DISABLED && > + slot1->active != QCRYPTO_BLOCK_LUKS_KEY_SLOT_ENABLED) { Align the two lines with ( > + 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
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
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; + 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; + } + + if (slot1->active != QCRYPTO_BLOCK_LUKS_KEY_SLOT_DISABLED && + slot1->active != QCRYPTO_BLOCK_LUKS_KEY_SLOT_ENABLED) { + 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;
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(-)