| Message ID | 20190814202219.1870-7-mlevitsk@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | RFC: luks/encrypted qcow2 key management | expand |
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 <mlevitsk@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
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 <mlevitsk@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. 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
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 <mlevitsk@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. > > 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
On 8/25/19 11:08 AM, Maxim Levitsky wrote: >>> 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. Actually, for size_t, you need %zu. %lu/size_t will cause warnings on 32-bit platforms.
On Mon, 2019-08-26 at 08:31 -0500, Eric Blake wrote: > On 8/25/19 11:08 AM, Maxim Levitsky wrote: > > > > > 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. > > Actually, for size_t, you need %zu. %lu/size_t will cause warnings on > 32-bit platforms. > > Thank you! I have read something like that on the internet, but I wondered, what actually is the most portable way. Best regards, Maxim Levitsky
On Sun, Aug 25, 2019 at 07:08:00PM +0300, Maxim Levitsky wrote: > 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 <mlevitsk@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. > > > > 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. For size_t, you need %zu to be 32/64-bit agnostic (ssize_t is %zd) Regards, Daniel
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); + + if (slot1->stripes == 0 || + (slot1->active != QCRYPTO_BLOCK_LUKS_KEY_SLOT_DISABLED && + slot1->active != QCRYPTO_BLOCK_LUKS_KEY_SLOT_ENABLED)) { + + error_setg(errp, "Keyslot %i is corrupted", i); + 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++) { + + 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", + i, j); + ret = -EINVAL; + 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 doens't really matter. Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> --- crypto/block-luks.c | 42 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+)