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);
+
+ 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;
--
2.17.2
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
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
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. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
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
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
© 2016 - 2026 Red Hat, Inc.