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