9:22 a.m.
On 06/21/2016 08:41 AM, Peter Krempa wrote:
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(a)redhat.com>
> ---
> 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.
OK - easily done
> 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.
Hmm... I think at one time I had thought it might need to be. I can drop
LUKS_HDR_CRYPT_OFFSET
> + },
> /* 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.
Ewww. I think going with versionSize will be easier, although
versionFunc would be just as entertaining
> + 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?
At one time I thought I might have to get more data, but it seems it
really doesn't matter unless we wanted to display it somehow. As I was
going through the implementation phase, I thought I'd have to know which
crypt algorithm was used, but all that is handled by qemu - all we need
to provide on open/read is the AES secret in order to provide the
password used to encrypt the data.
Since this code was written I've noted Daniel has a qemu patches:
https://lists.gnu.org/archive/html/qemu-devel/2016-06/msg03885.html
which will allow qemu-img display a whole lot more luks information
which I think would be a better option than us needing to keep track of
more header information. When v2 comes along, we'd have more work to do
in order to print out information.
Perhaps a nice addition, but as I've found out not technically necessary
to make things work.
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.
I prefer not to visit the crypt...
John
> + 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(a)redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list