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@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.
>
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@xxxxxxxxxx
>> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list