On Mon, Jun 13, 2016 at 20:27:52 -0400, John Ferlan wrote:
> Add the ability to detect a luks encrypted device.
>
> This also adding new 16 bit big/little endian macros since the
> version of a luks device is stored in a uint16_t.
>
> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
> ---
> src/util/virendian.h | 24 ++++++++++++++++++++++++
> src/util/virstoragefile.c | 38 ++++++++++++++++++++++++++++++++------
> src/util/virstoragefile.h | 1 +
> tests/virendiantest.c | 18 ++++++++++++++++++
> 4 files changed, 75 insertions(+), 6 deletions(-)
>
> diff --git a/src/util/virendian.h b/src/util/virendian.h
> index eefe48c..97940bd 100644
> --- a/src/util/virendian.h
> +++ b/src/util/virendian.h
> @@ -90,4 +90,28 @@
> ((uint32_t)(uint8_t)((buf)[2]) << 16) | \
> ((uint32_t)(uint8_t)((buf)[3]) << 24))
>
> +/**
> + * virReadBufInt16BE:
> + * @buf: byte to start reading at (can be 'char*' or 'unsigned char*');
> + * evaluating buf must not have any side effects
> + *
> + * Read 2 bytes at BUF as a big-endian 16-bit number. Caller is
> + * responsible to avoid reading beyond array bounds.
> + */
> +# define virReadBufInt16BE(buf) \
> + (((uint16_t)(uint8_t)((buf)[0]) << 8) | \
> + (uint16_t)(uint8_t)((buf)[1]))
> +
> +/**
> + * virReadBufInt16LE:
> + * @buf: byte to start reading at (can be 'char*' or 'unsigned char*');
> + * evaluating buf must not have any side effects
> + *
> + * Read 2 bytes at BUF as a little-endian 16-bit number. Caller is
> + * responsible to avoid reading beyond array bounds.
> + */
> +# define virReadBufInt16LE(buf) \
> + ((uint16_t)(uint8_t)((buf)[0]) | \
> + ((uint16_t)(uint8_t)((buf)[1]) << 8))
> +
> #endif /* __VIR_ENDIAN_H__ */
Since you are adding this code and also the tests below it looks like a
job for a separate patch.
> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index 5c2519c..f7a9632 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
[...]
> @@ -193,6 +194,14 @@ qedGetBackingStore(char **, int *, const char *, size_t);
> #define PLOOP_IMAGE_SIZE_OFFSET 36
> #define PLOOP_SIZE_MULTIPLIER 512
>
> +#define LUKS_HDR_MAGIC_LEN 6
> +#define LUKS_HDR_VERSION_LEN 2
> +
> +/* Format described by qemu commit id '3e308f20e' */
> +#define LUKS_HDR_VERSION_OFFSET LUKS_HDR_MAGIC_LEN
> +#define LUKS_HDR_CRYPT_OFFSET LUKS_HDR_MAGIC_LEN + LUKS_HDR_VERSION_LEN
> +
> +
> static struct FileTypeInfo const fileTypeInfo[] = {
> [VIR_STORAGE_FILE_NONE] = { 0, NULL, NULL, LV_LITTLE_ENDIAN,
> -1, {0}, 0, 0, 0, FI_CRYPT_NONE, 0, NULL, NULL },
> @@ -249,6 +258,13 @@ static struct FileTypeInfo const fileTypeInfo[] = {
> PLOOP_SIZE_MULTIPLIER,
> FI_CRYPT_NONE, -1, NULL, NULL },
>
> + /* Magic is 'L','U','K','S', 0xBA, 0xBE
> + * Set sizeOffset = -1 and let hypervisor handle */
> + [VIR_STORAGE_FILE_LUKS] = {
> + 0, "\x4c\x55\x4b\x53\xba\xbe", NULL,
> + LV_BIG_ENDIAN, LUKS_HDR_VERSION_OFFSET, {1},
> + -1, 0, 0, FI_CRYPT_LUKS, LUKS_HDR_CRYPT_OFFSET, NULL, NULL
The encryption header offset is not used here. You are not extracting
the cipher name from the header.
> + },
> /* All formats with a backing store probe below here */
> [VIR_STORAGE_FILE_COW] = {
> 0, "OOOM", NULL,
> @@ -634,7 +650,7 @@ virStorageFileMatchesVersion(int format,
> char *buf,
> size_t buflen)
> {
> - int version;
> + int version = 0;
> size_t i;
>
> /* Validate version number info */
> @@ -648,10 +664,16 @@ virStorageFileMatchesVersion(int format,
> if ((fileTypeInfo[format].versionOffset + 4) > buflen)
> return false;
>
> - if (fileTypeInfo[format].endian == LV_LITTLE_ENDIAN)
> + if (fileTypeInfo[format].endian == LV_LITTLE_ENDIAN) {
> version = virReadBufInt32LE(buf + fileTypeInfo[format].versionOffset);
> - else
> - version = virReadBufInt32BE(buf + fileTypeInfo[format].versionOffset);
> + } else {
> + if (format == VIR_STORAGE_FILE_LUKS)
This should be selected with a different property than the file type.
The idea of the structure above was to avoid having to tweak this code
with individual cases for every single file type. You'll either need to
pass a function pointer for extraction or switch it according to the
passed size.
> + version = virReadBufInt16BE(buf +
> + fileTypeInfo[format].versionOffset);
> + else
> + version = virReadBufInt32BE(buf +
> + fileTypeInfo[format].versionOffset);
> + }
>
> for (i = 0;
> i < FILE_TYPE_VERSIONS_LAST && fileTypeInfo[format].versionNumbers[i];
> @@ -832,6 +854,10 @@ virStorageFileGetMetadataInternal(virStorageSourcePtr meta,
> if (crypt_format && !meta->encryption &&
> VIR_ALLOC(meta->encryption) < 0)
> goto cleanup;
> + } else if (fileTypeInfo[meta->format].cryptType == FI_CRYPT_LUKS) {
> + /* By definition, this is encrypted */
FI_CRYPT_LUKS is implied by by the type which in turn implies
encryption. Also the encryption header offset is rather useless here
since it's not used.
Are you expecting to add extraction of the actual cipher type here?
In such case it would be actually desired to have the switch as you've
added here, but you should change it to VIR_ prefix and perhaps add a
more sane comment than /* style of crypt */ ... I prefer gothic crypts.
> + if (!meta->encryption && VIR_ALLOC(meta->encryption) < 0)
> + goto cleanup;
> }
>
> VIR_FREE(meta->backingStoreRaw);
> diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
> index 9424fed..8d5c45a 100644
> --- a/src/util/virstoragefile.h
> +++ b/src/util/virstoragefile.h
> @@ -74,6 +74,7 @@ typedef enum {
> VIR_STORAGE_FILE_FAT,
> VIR_STORAGE_FILE_VHD,
> VIR_STORAGE_FILE_PLOOP,
> + VIR_STORAGE_FILE_LUKS,
>
> /* Not a format, but a marker: all formats below this point have
> * libvirt support for following a backing chain */
> diff --git a/tests/virendiantest.c b/tests/virendiantest.c
> index 4072507..f858e5c 100644
> --- a/tests/virendiantest.c
> +++ b/tests/virendiantest.c
> @@ -50,6 +50,15 @@ test1(const void *data ATTRIBUTE_UNUSED)
> if (virReadBufInt32LE(array + 9) != 0x8d8c8b8aU)
> goto cleanup;
>
> + if (virReadBufInt16BE(array) != 0x0102U)
> + goto cleanup;
> + if (virReadBufInt16BE(array + 11) != 0x8c8dU)
> + goto cleanup;
> + if (virReadBufInt16LE(array) != 0x0201U)
> + goto cleanup;
> + if (virReadBufInt16LE(array + 11) != 0x8d8cU)
> + goto cleanup;
> +
> ret = 0;
> cleanup:
> return ret;
> @@ -81,6 +90,15 @@ test2(const void *data ATTRIBUTE_UNUSED)
> if (virReadBufInt32LE(array + 9) != 0x8d8c8b8aU)
> goto cleanup;
>
> + if (virReadBufInt16BE(array) != 0x0102U)
> + goto cleanup;
> + if (virReadBufInt16BE(array + 11) != 0x8c8dU)
> + goto cleanup;
> + if (virReadBufInt16LE(array) != 0x0201U)
> + goto cleanup;
> + if (virReadBufInt16LE(array + 11) != 0x8d8cU)
> + goto cleanup;
> +
With these it's definitely stuff for a separate patch.
> ret = 0;
> cleanup:
> return ret;
> --
> 2.5.5
>
> --
> libvir-list mailing list
> libvir-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list