| Message ID | 20110831143623.043146580@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Aug 31, 2011 at 10:36:01AM -0400, Stefan Berger wrote: > This patch adds encryption of the individual state blobs that are written > into the block storage. The 'directory' at the beginnig of the block > storage is not encrypted. Does this mean that there's a new format that we store data in, and that qemu needs to support? If so, this needs an entry under docs documenting the format. > > The encryption support added in this patch would also work if QCoW2 was not > to be used as the (only) image file format to store the TPM's state. What does 'was not to be used' above mean? > > Keys can be passed as a string of hexadecimal digits forming a 256, 192 or > 128 bit AES key. The string can optionally start with '0x'. If the > parser does not recognize it as a hexadecimal number, the string itself is > taken as the AES key, which makes for example 'my_key' a valid AES key > parameter. It is also necessary to provide the encryption scheme. > Currently only 'aes-cbc' is supported. An example for a valid key command > line argument is: > > -tpm builtin,key=aes-cbc:0x1234567890abcdef123456 > > The key passed via command line argument is wiped from the command > line after parsing. If for example key=aes-cbc:0x1234... was passed it will > then be changed to key=------... so that 'ps' does not show the key anymore. > Obviously it cannot be completely prevented that the key is visible during a > very short period of time until qemu gets to the point where the code wiping > the key is reached. > > A byte indicating the encryption type being used is introduced in the > directory structure indicating whether blobs are encrypted and if so, what > encryption type was used, i.e., aes-cbc. > > An additional 'layer' for reading and writing the blobs to the underlying > block storage is added. This layer encrypts the blobs for writing if a key is > available. Similarly it decrypts the blobs after reading. > > Checks are added that test > - whether encryption is supported follwing the revision of the directory > structure (rev >= 2) You never generate rev 1 code, right? So why keep that support around in code? The first version merged into qemu should be revision 0 (or 1, as you like). Don't support legacy with old version of your patch. > - whether a key has been provided although all data are stored in clear-text > - whether a key is missing for decryption. > > In either one of the cases the backend reports an error message to the user > and Qemu terminates. > > -v7: > - cleaned up function parsing key > > -v6: > - changed the format of the key= to take the type of encryption into > account: key=aes-cbc:0x12345... and reworked code for encryption and > decryption of blobs; separate type and data: keytype=aes-cbc,key=0x123 to avoid introducing more option parsing. Also, are people likely to have the key in a file? If yes maybe read a key from there and skip parsing completely? > - modified directory entry to hold a uint_8 describing the encryption > type (none, aes-cbc) being used for the blobs. > - incrementing revision of the directory to '2' indicating encryption > support > > -v5: > - -tpmdev now also gets a key parameter > - add documentation about key parameter > > Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> > > --- > hw/tpm_builtin.c | 285 +++++++++++++++++++++++++++++++++++++++++++++++++++++-- > qemu-config.c | 10 + > qemu-options.hx | 22 +++- > tpm.c | 10 + > 4 files changed, 318 insertions(+), 9 deletions(-) > > Index: qemu-git/hw/tpm_builtin.c > =================================================================== > --- qemu-git.orig/hw/tpm_builtin.c > +++ qemu-git/hw/tpm_builtin.c > @@ -27,6 +27,7 @@ > #include "hw/pc.h" > #include "migration.h" > #include "sysemu.h" > +#include "aes.h" > > #include <libtpms/tpm_library.h> > #include <libtpms/tpm_error.h> > @@ -110,14 +111,27 @@ typedef struct BSDir { > uint16_t rev; > uint32_t checksum; > uint32_t num_entries; > - uint32_t reserved[10]; > + uint8_t enctype; > + uint8_t reserved1[3]; > + uint32_t reserved[8]; > BSEntry entries[BS_DIR_MAX_NUM_ENTRIES]; > } __attribute__((packed)) BSDir; > > > #define BS_DIR_REV1 1 > +/* rev 2 added encryption */ > +#define BS_DIR_REV2 2 > > -#define BS_DIR_REV_CURRENT BS_DIR_REV1 > + > +#define BS_DIR_REV_CURRENT BS_DIR_REV2 > + > +/* above enctype */ > +enum BSEnctype { > + BS_DIR_ENCTYPE_NONE = 0, > + BS_DIR_ENCTYPE_AES_CBC, > + > + BS_DIR_ENCTYPE_LAST, > +}; > > /* local variables */ > > @@ -150,6 +164,11 @@ static const unsigned char tpm_std_fatal > > static char dev_description[80]; > > +static struct enckey { > + uint8_t enctype; > + AES_KEY tpm_enc_key; > + AES_KEY tpm_dec_key; > +} enckey; > > static int tpm_builtin_load_sized_data_from_bs(BlockDriverState *bs, > enum BSEntryType be, > @@ -264,7 +283,7 @@ static uint32_t tpm_builtin_calc_dir_che > > static bool tpm_builtin_is_valid_bsdir(BSDir *dir) > { > - if (dir->rev != BS_DIR_REV_CURRENT || > + if (dir->rev > BS_DIR_REV_CURRENT || > dir->num_entries > BS_DIR_MAX_NUM_ENTRIES) { > return false; > } > @@ -295,6 +314,33 @@ static bool tpm_builtin_has_valid_conten > return rc; > } > > +static bool tpm_builtin_supports_encryption(const BSDir *dir) > +{ > + return (dir->rev >= BS_DIR_REV2); > +} > + > + > +static bool tpm_builtin_has_missing_key(const BSDir *dir) > +{ > + return ((dir->enctype != BS_DIR_ENCTYPE_NONE) && > + (enckey.enctype == BS_DIR_ENCTYPE_NONE)); > +} > + > + > +static bool tpm_builtin_has_unnecessary_key(const BSDir *dir) > +{ > + return (((dir->enctype == BS_DIR_ENCTYPE_NONE) && > + (enckey.enctype != BS_DIR_ENCTYPE_NONE)) || > + ((!tpm_builtin_supports_encryption(dir)) && > + (enckey.enctype != BS_DIR_ENCTYPE_NONE))); > +} > + > + > +static bool tpm_builtin_uses_unsupported_enctype(const BSDir *dir) > +{ > + return (dir->enctype >= BS_DIR_ENCTYPE_LAST); > +} > + > > static int tpm_builtin_create_blank_dir(BlockDriverState *bs) > { > @@ -306,6 +352,7 @@ static int tpm_builtin_create_blank_dir( > dir = (BSDir *)buf; > dir->rev = BS_DIR_REV_CURRENT; > dir->num_entries = 0; > + dir->enctype = enckey.enctype; > > dir->checksum = tpm_builtin_calc_dir_checksum(dir); > > @@ -407,6 +454,38 @@ static int tpm_builtin_startup_bs(BlockD > > tpm_builtin_dir_be_to_cpu(dir); > > + if (tpm_builtin_is_valid_bsdir(dir)) { > + if (tpm_builtin_supports_encryption(dir) && > + tpm_builtin_has_missing_key(dir)) { > + fprintf(stderr, > + "tpm: the data are encrypted but I am missing the key.\n"); > + rc = -EIO; > + goto err_exit; > + } > + if (tpm_builtin_has_unnecessary_key(dir)) { > + fprintf(stderr, > + "tpm: I have a key but the data are not encrypted.\n"); > + rc = -EIO; > + goto err_exit; > + } > + if (tpm_builtin_supports_encryption(dir) && > + tpm_builtin_uses_unsupported_enctype(dir)) { > + fprintf(stderr, > + "tpm: State is encrypted with an unsupported encryption " > + "scheme.\n"); > + rc = -EIO; > + goto err_exit; > + } > + if (tpm_builtin_supports_encryption(dir) && > + (dir->enctype != BS_DIR_ENCTYPE_NONE) && > + !tpm_builtin_has_valid_content(dir)) { > + fprintf(stderr, "tpm: cannot read the data - " > + "is this the wrong key?\n"); > + rc = -EIO; > + goto err_exit; > + } > + } > + > if (!tpm_builtin_is_valid_bsdir(dir) || > !tpm_builtin_has_valid_content(dir)) { > /* if it's encrypted and has something else than null-content, > @@ -569,6 +648,105 @@ static int set_bs_entry_size_crc(BlockDr > } > > > +static int tpm_builtin_blocksize_roundup(uint8_t enctype, int plainsize) > +{ > + switch (enctype) { > + case BS_DIR_ENCTYPE_NONE: > + return plainsize; > + case BS_DIR_ENCTYPE_AES_CBC: > + return ALIGN(plainsize, AES_BLOCK_SIZE); > + default: > + assert(false); > + return 0; > + } > +} > + > + > +static int tpm_builtin_bdrv_pread(BlockDriverState *bs, int64_t offset, > + void *buf, int count, > + enum BSEntryType type) > +{ > + int ret; > + union { > + uint64_t ll[2]; > + uint8_t b[16]; > + } ivec; > + int toread = count; > + > + toread = tpm_builtin_blocksize_roundup(enckey.enctype, count); > + > + ret = bdrv_pread(bs, offset, buf, toread); > + > + if (ret != toread) { > + return ret; > + } > + > + switch (enckey.enctype) { > + case BS_DIR_ENCTYPE_NONE: > + break; > + case BS_DIR_ENCTYPE_AES_CBC: > + ivec.ll[0] = cpu_to_be64(type); > + ivec.ll[1] = 0; > + > + AES_cbc_encrypt(buf, buf, toread, &enckey.tpm_dec_key, ivec.b, 0); > + break; > + default: > + assert(false); > + } > + > + return count; > +} > + > + > +static int tpm_builtin_bdrv_pwrite(BlockDriverState *bs, int64_t offset, > + void *buf, int count, > + enum BSEntryType type) > +{ > + int ret; > + union { > + uint64_t ll[2]; > + uint8_t b[16]; > + } ivec; > + int towrite = count; > + void *out_buf = buf; > + > + switch (enckey.enctype) { > + case BS_DIR_ENCTYPE_NONE: > + break; > + case BS_DIR_ENCTYPE_AES_CBC: > + ivec.ll[0] = cpu_to_be64(type); > + ivec.ll[1] = 0; > + > + towrite = ALIGN(count, AES_BLOCK_SIZE); > + > + if (towrite != count) { > + out_buf = g_malloc(towrite); > + > + if (out_buf == NULL) { > + return -ENOMEM; > + } > + } > + > + AES_cbc_encrypt(buf, out_buf, towrite, &enckey.tpm_enc_key, ivec.b, 1); > + break; > + default: > + assert(false); > + } > + > + ret = bdrv_pwrite(bs, offset, out_buf, towrite); > + > + if (out_buf != buf) { > + g_free(out_buf); > + } > + > + if (ret == towrite) { > + return count; > + } > + > + return ret; > +} > + > + > static int tpm_builtin_load_sized_data_from_bs(BlockDriverState *bs, > enum BSEntryType be, > TPMSizedBuffer *tsb) > @@ -594,7 +772,7 @@ static int tpm_builtin_load_sized_data_f > goto err_exit; > } > > - tsb->buffer = g_malloc(entry.blobsize); > + tsb->buffer = g_malloc(ALIGN(entry.blobsize, AES_BLOCK_SIZE)); > if (!tsb->buffer) { > rc = -ENOMEM; > goto err_exit; > @@ -602,7 +780,8 @@ static int tpm_builtin_load_sized_data_f > > tsb->size = entry.blobsize; > > - if (bdrv_pread(bs, entry.offset, tsb->buffer, tsb->size) != tsb->size) { > + if (tpm_builtin_bdrv_pread(bs, entry.offset, tsb->buffer, tsb->size, be) != > + tsb->size) { > clear_sized_buffer(tsb); > fprintf(stderr, "tpm: Error while reading stored data!\n"); > rc = -EIO; > @@ -667,7 +846,8 @@ static int tpm_builtin_save_sized_data_t > } > > if (data_len > 0) { > - if (bdrv_pwrite(bs, entry.offset, data, data_len) != data_len) { > + if (tpm_builtin_bdrv_pwrite(bs, entry.offset, data, data_len, be) != > + data_len) { > rc = -EIO; > } > } > @@ -1492,11 +1672,77 @@ static const char *tpm_builtin_create_de > } > > > +/* > + * Convert a string of hex digits to its binary representation. > + * The conversion stops once either the maximum size of the binary > + * array has been reached or an non-hex digit was encountered. > + */ Don't we care about non-hex following a valid key? This will silently discard them if length matches a legal value by luck. > +static size_t stream_to_bin(const char *stream, > + unsigned char *bin, size_t bin_size) > +{ > + size_t c = 0; > + unsigned char nib = 0; > + > + while (c < bin_size && stream[c] != 0) { > + if (stream[c] >= '0' && stream[c] <= '9') { > + nib |= stream[c] - '0'; > + } else if (stream[c] >= 'A' && stream[c] <= 'F') { > + nib |= stream[c] - 'A' + 10; > + } else if (stream[c] >= 'a' && stream[c] <= 'f') { > + nib |= stream[c] - 'a' + 10; > + } else { > + break; > + } > + > + if ((c & 1) == 1) { > + bin[c/2] = nib; > + nib = 0; > + } else { > + nib <<= 4; > + bin[c/2] = nib; > + } > + > + c++; > + } > + > + return c; > +} Can't this use something like scanf %x instead? Something like the below seems to work for me, and gives length in bytes and not nibbles. #include <stdio.h> #include <assert.h> int main(int argc, char **argv) { int l = 0, b = 0, s, n; char buf[256 / 8]; for (b = 0; b < sizeof(buf); ++b) { s = sscanf(argv[1] + l, "%2hhx%n", buf + b, &n); if (s == 0) { printf("invalid input. scanned %d bytes, text left: %s\n", b, argv[1] + l); return 1; } assert(s != EOF && n >= 1 && n <= 2); l += n; if (!argv[1][l]) { printf("scanned %d bytes length %d\n", b + 1, l); return 0; } } printf("key too long. scanned %d bytes, text left: %s\n", b, argv[1] + l); return 2; } > + > + Two empty lines in a row :) > +static bool tpm_builtin_parse_as_hexkey(const char *rawkey, > + unsigned char *keyvalue, > + int *keysize) > +{ > + size_t c = 0; > + > + /* skip over leading '0x' */ > + if (!strncmp(rawkey, "0x", 2)) { > + rawkey += 2; > + } > + > + c = stream_to_bin(rawkey, keyvalue, *keysize); > + > + if (c == 256/4) { > + *keysize = 256; > + } else if (c >= 192/4) { > + *keysize = 192; > + } else if (c >= 128/4) { > + *keysize = 128; > + } else { > + return false; Want to tell the user what went wrong? Also, you don't allow skipping leading zeroes? > + } > + > + return true; Always put spaces around /. But where does the /4 come from? 4 bits per character? > +} > + > + > static TPMBackend *tpm_builtin_create(QemuOpts *opts, const char *id, > const char *model) > { > TPMBackend *driver; > const char *value; > + unsigned char keyvalue[256/8]; > + int keysize = sizeof(keyvalue); > > driver = g_malloc(sizeof(TPMBackend)); > if (!driver) { > @@ -1523,6 +1769,33 @@ static TPMBackend *tpm_builtin_create(Qe > goto err_exit; > } > > + value = qemu_opt_get(opts, "key"); > + if (value) { > + if (!strncasecmp(value, "aes-cbc:", 8)) { > + memset(keyvalue, 0x0, sizeof(keyvalue)); > + > + if (!tpm_builtin_parse_as_hexkey(&value[8], keyvalue, &keysize)) { > + keysize = 128; > + strncpy((char *)keyvalue, value, 128/8); > + } > + > + if (AES_set_encrypt_key(keyvalue, keysize, > + &enckey.tpm_enc_key) != 0 || > + AES_set_decrypt_key(keyvalue, keysize, > + &enckey.tpm_dec_key) != 0) { > + fprintf(stderr, "tpm: Error setting AES key.\n"); > + goto err_exit; > + } > + enckey.enctype = BS_DIR_ENCTYPE_AES_CBC; > + } else { > + fprintf(stderr, "tpm: Unknown encryption scheme. Known types are: " > + "aes-cbc.\n"); > + goto err_exit; > + } > + } else { > + enckey.enctype = BS_DIR_ENCTYPE_NONE; > + } > + > return driver; > > err_exit: > Index: qemu-git/qemu-config.c > =================================================================== > --- qemu-git.orig/qemu-config.c > +++ qemu-git/qemu-config.c > @@ -522,6 +522,11 @@ static QemuOptsList qemu_tpmdev_opts = { > .type = QEMU_OPT_STRING, > .help = "Persistent storage for TPM state", > }, > + { > + .name = "key", > + .type = QEMU_OPT_STRING, > + .help = "Data encryption key", > + }, > { /* end of list */ } > }, > }; > @@ -546,6 +551,11 @@ static QemuOptsList qemu_tpm_opts = { > .type = QEMU_OPT_STRING, > .help = "Persistent storage for TPM state", > }, > + { > + .name = "key", > + .type = QEMU_OPT_STRING, > + .help = "Data encryption key", > + }, > { /* end of list */ } > }, > }; > Index: qemu-git/tpm.c > =================================================================== > --- qemu-git.orig/tpm.c > +++ qemu-git/tpm.c > @@ -245,6 +245,7 @@ void tpm_cleanup(void) > void tpm_config_parse(QemuOptsList *opts_list, const char *optarg) > { > QemuOpts *opts; > + char *key; > > if (strcmp("none", optarg) != 0) { > if (*optarg == '?') { > @@ -255,6 +256,15 @@ void tpm_config_parse(QemuOptsList *opts > if (!opts) { > exit(1); > } > + > + /* if a key is provided, wipe it out so no one can see it with 'ps' */ > + key = strstr(optarg, "key="); > + if (key) { > + key += 4; > + while (key[0] && key[0] != ',') { > + *key++ = '-'; > + } > + } > } > } > > Index: qemu-git/qemu-options.hx > =================================================================== > --- qemu-git.orig/qemu-options.hx > +++ qemu-git/qemu-options.hx > @@ -1766,8 +1766,9 @@ DEFHEADING(TPM device options:) > # ifdef CONFIG_TPM > DEF("tpm", HAS_ARG, QEMU_OPTION_tpm, \ > "" \ > - "-tpm builtin,path=<path>[,model=<model>]\n" \ > + "-tpm builtin,path=<path>[,model=<model>][,key=<aes key>]\n" \ > " enable a builtin TPM with state in file in path\n" \ > + " and encrypt the TPM's state with the given AES key\n" \ > "-tpm model=? to list available TPM device models\n" \ > "-tpm ? to list available TPM backend types\n", > QEMU_ARCH_I386) > @@ -1796,13 +1797,22 @@ Use ? to print all available TPM backend > qemu -tpmdev ? > @end example > > -@item -tpmdev builtin ,id=@var{id}, path=@var{path} > +@item -tpmdev builtin ,id=@var{id}, path=@var{path} [,key=@var{key}] > > Creates an instance of the built-in TPM. > > @option{path} specifies the path to the QCoW2 image that will store > the TPM's persistent data. @option{path} is required. > > +@option{key} specifies the AES key to use to encrypt the TPM's persistent > +data. If encryption is to be used, the key must be provided the first > +time a Qemu VM with attached TPM is started and the same key must subsequently > +be used. The format of the key is the type of encryption to use, i.e., > +@code{aes-cbc}, followed by a colon and then the actual key. The key can > +be a hex number with optional leading @code{0x} > +and 32, 48 or 64 hex digits for 128, 192 or 256 bit AES keys respectively. > +@option{key} is optional. > + > To create a built-in TPM use the following two options: > @example > -tpmdev builtin,id=tpm0,path=<path_to_qcow2> -device tpm-tis,tpmdev=tpm0 > @@ -1810,12 +1820,18 @@ To create a built-in TPM use the followi > Not that the @code{-tpmdev} id is @code{tpm0} and is referenced by > @code{tpmdev=tpm0} in the device option. > > + > +To create a built-in TPM whose state is encrypted with a 128 bit AES key > +using AES-CBC encryption scheme supply the following two options: > +@example > +-tpmdev builtin,id=tpm0,path=<path_to_qcow2>,key=aes-cbc:0x1234567890abcdef01234567890abcdef -device tpm-tis,tpmdev=tpm0 > +@end example > @end table > > The short form of a TPM device option is: > @table @option > > -@item -tpm @var{backend-type}, path=@var{path} [,model=@var{model}] > +@item -tpm @var{backend-type}, path=@var{path} [,model=@var{model}] [,key=@var{key}] > @findex -tpm > > @option{model} specifies the device model. The default device model is a >
On 09/01/2011 03:26 PM, Michael S. Tsirkin wrote: > On Wed, Aug 31, 2011 at 10:36:01AM -0400, Stefan Berger wrote: >> This patch adds encryption of the individual state blobs that are written >> into the block storage. The 'directory' at the beginnig of the block >> storage is not encrypted. > Does this mean that there's a new format that we store data > in, and that qemu needs to support? No, this type of encryption is completely handled inside this file. > If so, this needs an entry under docs documenting the format. > >> The encryption support added in this patch would also work if QCoW2 was not >> to be used as the (only) image file format to store the TPM's state. > What does 'was not to be used' above mean? > The only image file format that the libtpms based 'builtin' backend currently accepts is the QCoW2 file format. QCoW2 provides encryption of its own (I find it a bit problematic). From what I know none of the other file formats provides encryption, i.e., 'raw' for sure does not. So if the image type format requirement was to be relaxed to also allow for example 'raw', then the encryption added in this patch would still work. >> Keys can be passed as a string of hexadecimal digits forming a 256, 192 or >> 128 bit AES key. The string can optionally start with '0x'. If the >> parser does not recognize it as a hexadecimal number, the string itself is >> taken as the AES key, which makes for example 'my_key' a valid AES key >> parameter. It is also necessary to provide the encryption scheme. >> Currently only 'aes-cbc' is supported. An example for a valid key command >> line argument is: >> >> -tpm builtin,key=aes-cbc:0x1234567890abcdef123456 >> >> The key passed via command line argument is wiped from the command >> line after parsing. If for example key=aes-cbc:0x1234... was passed it will >> then be changed to key=------... so that 'ps' does not show the key anymore. >> Obviously it cannot be completely prevented that the key is visible during a >> very short period of time until qemu gets to the point where the code wiping >> the key is reached. >> >> A byte indicating the encryption type being used is introduced in the >> directory structure indicating whether blobs are encrypted and if so, what >> encryption type was used, i.e., aes-cbc. >> >> An additional 'layer' for reading and writing the blobs to the underlying >> block storage is added. This layer encrypts the blobs for writing if a key is >> available. Similarly it decrypts the blobs after reading. >> >> Checks are added that test >> - whether encryption is supported follwing the revision of the directory >> structure (rev>= 2) > You never generate rev 1 code, right? I did this in the previous patch that implemented rev 1 that knew nothing about the encryption added in rev 2. > So why keep that support around in code? > The first version merged into qemu should be revision 0 (or 1, as you like). I chose '1'. See patch 9: +#define BS_DIR_REV1 1 + +#define BS_DIR_REV_CURRENT BS_DIR_REV1 + So I think it's the proper thing to do to increase the revision number from 1 to 2 since it's in two separate patches (even if they were to be applied immediately). > Don't support legacy with old version of your patch. > >> - whether a key has been provided although all data are stored in clear-text >> - whether a key is missing for decryption. >> >> In either one of the cases the backend reports an error message to the user >> and Qemu terminates. >> >> -v7: >> - cleaned up function parsing key >> >> -v6: >> - changed the format of the key= to take the type of encryption into >> account: key=aes-cbc:0x12345... and reworked code for encryption and >> decryption of blobs; > separate type and data: > keytype=aes-cbc,key=0x123 to avoid introducing more option parsing. > Also, are people likely to have the key in a file? > If yes maybe read a key from there and skip parsing completely? > I think both choices should probably exist. Now what's a good file format? Would we expect to find a hex number in there or should it always be assumed to be a binary file? >> - modified directory entry to hold a uint_8 describing the encryption >> type (none, aes-cbc) being used for the blobs. >> - incrementing revision of the directory to '2' indicating encryption >> support >> >> -v5: >> - -tpmdev now also gets a key parameter >> - add documentation about key parameter >> >> Signed-off-by: Stefan Berger<stefanb@linux.vnet.ibm.com> >> >> --- >> hw/tpm_builtin.c | 285 +++++++++++++++++++++++++++++++++++++++++++++++++++++-- >> qemu-config.c | 10 + >> qemu-options.hx | 22 +++- >> tpm.c | 10 + >> 4 files changed, 318 insertions(+), 9 deletions(-) >> >> Index: qemu-git/hw/tpm_builtin.c >> =================================================================== >> --- qemu-git.orig/hw/tpm_builtin.c >> +++ qemu-git/hw/tpm_builtin.c >> @@ -27,6 +27,7 @@ >> #include "hw/pc.h" >> #include "migration.h" >> #include "sysemu.h" >> +#include "aes.h" >> >> #include<libtpms/tpm_library.h> >> #include<libtpms/tpm_error.h> >> @@ -110,14 +111,27 @@ typedef struct BSDir { >> uint16_t rev; >> uint32_t checksum; >> uint32_t num_entries; >> - uint32_t reserved[10]; >> + uint8_t enctype; >> + uint8_t reserved1[3]; >> + uint32_t reserved[8]; >> BSEntry entries[BS_DIR_MAX_NUM_ENTRIES]; >> } __attribute__((packed)) BSDir; >> >> >> #define BS_DIR_REV1 1 >> +/* rev 2 added encryption */ >> +#define BS_DIR_REV2 2 >> >> -#define BS_DIR_REV_CURRENT BS_DIR_REV1 >> + >> +#define BS_DIR_REV_CURRENT BS_DIR_REV2 >> + >> +/* above enctype */ >> +enum BSEnctype { >> + BS_DIR_ENCTYPE_NONE = 0, >> + BS_DIR_ENCTYPE_AES_CBC, >> + >> + BS_DIR_ENCTYPE_LAST, >> +}; >> >> /* local variables */ >> >> @@ -150,6 +164,11 @@ static const unsigned char tpm_std_fatal >> >> static char dev_description[80]; >> >> +static struct enckey { >> + uint8_t enctype; >> + AES_KEY tpm_enc_key; >> + AES_KEY tpm_dec_key; >> +} enckey; >> >> static int tpm_builtin_load_sized_data_from_bs(BlockDriverState *bs, >> enum BSEntryType be, >> @@ -264,7 +283,7 @@ static uint32_t tpm_builtin_calc_dir_che >> >> static bool tpm_builtin_is_valid_bsdir(BSDir *dir) >> { >> - if (dir->rev != BS_DIR_REV_CURRENT || >> + if (dir->rev> BS_DIR_REV_CURRENT || >> dir->num_entries> BS_DIR_MAX_NUM_ENTRIES) { >> return false; >> } >> @@ -295,6 +314,33 @@ static bool tpm_builtin_has_valid_conten >> return rc; >> } >> >> +static bool tpm_builtin_supports_encryption(const BSDir *dir) >> +{ >> + return (dir->rev>= BS_DIR_REV2); >> +} >> + >> + >> +static bool tpm_builtin_has_missing_key(const BSDir *dir) >> +{ >> + return ((dir->enctype != BS_DIR_ENCTYPE_NONE)&& >> + (enckey.enctype == BS_DIR_ENCTYPE_NONE)); >> +} >> + >> + >> +static bool tpm_builtin_has_unnecessary_key(const BSDir *dir) >> +{ >> + return (((dir->enctype == BS_DIR_ENCTYPE_NONE)&& >> + (enckey.enctype != BS_DIR_ENCTYPE_NONE)) || >> + ((!tpm_builtin_supports_encryption(dir))&& >> + (enckey.enctype != BS_DIR_ENCTYPE_NONE))); >> +} >> + >> + >> +static bool tpm_builtin_uses_unsupported_enctype(const BSDir *dir) >> +{ >> + return (dir->enctype>= BS_DIR_ENCTYPE_LAST); >> +} >> + >> >> static int tpm_builtin_create_blank_dir(BlockDriverState *bs) >> { >> @@ -306,6 +352,7 @@ static int tpm_builtin_create_blank_dir( >> dir = (BSDir *)buf; >> dir->rev = BS_DIR_REV_CURRENT; >> dir->num_entries = 0; >> + dir->enctype = enckey.enctype; >> >> dir->checksum = tpm_builtin_calc_dir_checksum(dir); >> >> @@ -407,6 +454,38 @@ static int tpm_builtin_startup_bs(BlockD >> >> tpm_builtin_dir_be_to_cpu(dir); >> >> + if (tpm_builtin_is_valid_bsdir(dir)) { >> + if (tpm_builtin_supports_encryption(dir)&& >> + tpm_builtin_has_missing_key(dir)) { >> + fprintf(stderr, >> + "tpm: the data are encrypted but I am missing the key.\n"); >> + rc = -EIO; >> + goto err_exit; >> + } >> + if (tpm_builtin_has_unnecessary_key(dir)) { >> + fprintf(stderr, >> + "tpm: I have a key but the data are not encrypted.\n"); >> + rc = -EIO; >> + goto err_exit; >> + } >> + if (tpm_builtin_supports_encryption(dir)&& >> + tpm_builtin_uses_unsupported_enctype(dir)) { >> + fprintf(stderr, >> + "tpm: State is encrypted with an unsupported encryption " >> + "scheme.\n"); >> + rc = -EIO; >> + goto err_exit; >> + } >> + if (tpm_builtin_supports_encryption(dir)&& >> + (dir->enctype != BS_DIR_ENCTYPE_NONE)&& >> + !tpm_builtin_has_valid_content(dir)) { >> + fprintf(stderr, "tpm: cannot read the data - " >> + "is this the wrong key?\n"); >> + rc = -EIO; >> + goto err_exit; >> + } >> + } >> + >> if (!tpm_builtin_is_valid_bsdir(dir) || >> !tpm_builtin_has_valid_content(dir)) { >> /* if it's encrypted and has something else than null-content, >> @@ -569,6 +648,105 @@ static int set_bs_entry_size_crc(BlockDr >> } >> >> >> +static int tpm_builtin_blocksize_roundup(uint8_t enctype, int plainsize) >> +{ >> + switch (enctype) { >> + case BS_DIR_ENCTYPE_NONE: >> + return plainsize; >> + case BS_DIR_ENCTYPE_AES_CBC: >> + return ALIGN(plainsize, AES_BLOCK_SIZE); >> + default: >> + assert(false); >> + return 0; >> + } >> +} >> + >> + >> +static int tpm_builtin_bdrv_pread(BlockDriverState *bs, int64_t offset, >> + void *buf, int count, >> + enum BSEntryType type) >> +{ >> + int ret; >> + union { >> + uint64_t ll[2]; >> + uint8_t b[16]; >> + } ivec; >> + int toread = count; >> + >> + toread = tpm_builtin_blocksize_roundup(enckey.enctype, count); >> + >> + ret = bdrv_pread(bs, offset, buf, toread); >> + >> + if (ret != toread) { >> + return ret; >> + } >> + >> + switch (enckey.enctype) { >> + case BS_DIR_ENCTYPE_NONE: >> + break; >> + case BS_DIR_ENCTYPE_AES_CBC: >> + ivec.ll[0] = cpu_to_be64(type); >> + ivec.ll[1] = 0; >> + >> + AES_cbc_encrypt(buf, buf, toread,&enckey.tpm_dec_key, ivec.b, 0); >> + break; >> + default: >> + assert(false); >> + } >> + >> + return count; >> +} >> + >> + >> +static int tpm_builtin_bdrv_pwrite(BlockDriverState *bs, int64_t offset, >> + void *buf, int count, >> + enum BSEntryType type) >> +{ >> + int ret; >> + union { >> + uint64_t ll[2]; >> + uint8_t b[16]; >> + } ivec; >> + int towrite = count; >> + void *out_buf = buf; >> + >> + switch (enckey.enctype) { >> + case BS_DIR_ENCTYPE_NONE: >> + break; >> + case BS_DIR_ENCTYPE_AES_CBC: >> + ivec.ll[0] = cpu_to_be64(type); >> + ivec.ll[1] = 0; >> + >> + towrite = ALIGN(count, AES_BLOCK_SIZE); >> + >> + if (towrite != count) { >> + out_buf = g_malloc(towrite); >> + >> + if (out_buf == NULL) { >> + return -ENOMEM; >> + } >> + } >> + >> + AES_cbc_encrypt(buf, out_buf, towrite,&enckey.tpm_enc_key, ivec.b, 1); >> + break; >> + default: >> + assert(false); >> + } >> + >> + ret = bdrv_pwrite(bs, offset, out_buf, towrite); >> + >> + if (out_buf != buf) { >> + g_free(out_buf); >> + } >> + >> + if (ret == towrite) { >> + return count; >> + } >> + >> + return ret; >> +} >> + >> + >> static int tpm_builtin_load_sized_data_from_bs(BlockDriverState *bs, >> enum BSEntryType be, >> TPMSizedBuffer *tsb) >> @@ -594,7 +772,7 @@ static int tpm_builtin_load_sized_data_f >> goto err_exit; >> } >> >> - tsb->buffer = g_malloc(entry.blobsize); >> + tsb->buffer = g_malloc(ALIGN(entry.blobsize, AES_BLOCK_SIZE)); >> if (!tsb->buffer) { >> rc = -ENOMEM; >> goto err_exit; >> @@ -602,7 +780,8 @@ static int tpm_builtin_load_sized_data_f >> >> tsb->size = entry.blobsize; >> >> - if (bdrv_pread(bs, entry.offset, tsb->buffer, tsb->size) != tsb->size) { >> + if (tpm_builtin_bdrv_pread(bs, entry.offset, tsb->buffer, tsb->size, be) != >> + tsb->size) { >> clear_sized_buffer(tsb); >> fprintf(stderr, "tpm: Error while reading stored data!\n"); >> rc = -EIO; >> @@ -667,7 +846,8 @@ static int tpm_builtin_save_sized_data_t >> } >> >> if (data_len> 0) { >> - if (bdrv_pwrite(bs, entry.offset, data, data_len) != data_len) { >> + if (tpm_builtin_bdrv_pwrite(bs, entry.offset, data, data_len, be) != >> + data_len) { >> rc = -EIO; >> } >> } >> @@ -1492,11 +1672,77 @@ static const char *tpm_builtin_create_de >> } >> >> >> +/* >> + * Convert a string of hex digits to its binary representation. >> + * The conversion stops once either the maximum size of the binary >> + * array has been reached or an non-hex digit was encountered. >> + */ > Don't we care about non-hex following a valid key? Do you have an example? This function is meant to convert 0x1234567 to a binary stream. An equivalent would be 0x1234567X, since it would terminate parsing on 'X'. > This will silently discard them if length matches > a legal value by luck. > >> +static size_t stream_to_bin(const char *stream, >> + unsigned char *bin, size_t bin_size) >> +{ >> + size_t c = 0; >> + unsigned char nib = 0; >> + >> + while (c< bin_size&& stream[c] != 0) { >> + if (stream[c]>= '0'&& stream[c]<= '9') { >> + nib |= stream[c] - '0'; >> + } else if (stream[c]>= 'A'&& stream[c]<= 'F') { >> + nib |= stream[c] - 'A' + 10; >> + } else if (stream[c]>= 'a'&& stream[c]<= 'f') { >> + nib |= stream[c] - 'a' + 10; >> + } else { >> + break; >> + } >> + >> + if ((c& 1) == 1) { >> + bin[c/2] = nib; >> + nib = 0; >> + } else { >> + nib<<= 4; >> + bin[c/2] = nib; >> + } >> + >> + c++; >> + } >> + >> + return c; >> +} > Can't this use something like scanf %x instead? Sure it could... > Something like the below seems to work for me, > and gives length in bytes and not nibbles. > > #include<stdio.h> > #include<assert.h> > > int main(int argc, char **argv) > { > int l = 0, b = 0, s, n; > char buf[256 / 8]; > for (b = 0; b< sizeof(buf); ++b) { > s = sscanf(argv[1] + l, "%2hhx%n", buf + b,&n); > if (s == 0) { > printf("invalid input. scanned %d bytes, text left: %s\n", b, argv[1] + l); > return 1; > } > assert(s != EOF&& n>= 1&& n<= 2); > l += n; > if (!argv[1][l]) { > printf("scanned %d bytes length %d\n", b + 1, l); > return 0; > } > } > printf("key too long. scanned %d bytes, text left: %s\n", b, argv[1] + l); > return 2; > } > > > >> + >> + > Two empty lines in a row :) > Probably this is not the only occurrence... Is this a problem? >> +static bool tpm_builtin_parse_as_hexkey(const char *rawkey, >> + unsigned char *keyvalue, >> + int *keysize) >> +{ >> + size_t c = 0; >> + >> + /* skip over leading '0x' */ >> + if (!strncmp(rawkey, "0x", 2)) { >> + rawkey += 2; >> + } >> + >> + c = stream_to_bin(rawkey, keyvalue, *keysize); >> + >> + if (c == 256/4) { >> + *keysize = 256; >> + } else if (c>= 192/4) { >> + *keysize = 192; >> + } else if (c>= 128/4) { >> + *keysize = 128; >> + } else { >> + return false; > Want to tell the user what went wrong? Here's what the key parser handles: - all keys >= 256 bits are truncated to 256 bits - all keys >= 192 bits are truncated to 192 bits - all keys >= 128 bits are truncated to 128 bits - all keys < 128 bits are assumed to not be given as a hexadecimal number but the string itself is the key, i.e. 'HELLOWORLD' becomes a valid key. > Also, you don't allow skipping leading zeroes? An AES key should be allowed to have leading zeros, no? >> + } >> + >> + return true; > Always put spaces around /. > But where does the /4 come from? 4 bits per character? > c is the number of 'nibbles'. 4 bits in a nibble - that's what the /4 comes from. >> +} >> + >> + >> static TPMBackend *tpm_builtin_create(QemuOpts *opts, const char *id, >> const char *model) >> { >> TPMBackend *driver; >> const char *value; >> + unsigned char keyvalue[256/8]; >> + int keysize = sizeof(keyvalue); >> >> driver = g_malloc(sizeof(TPMBackend)); >> if (!driver) { >> @@ -1523,6 +1769,33 @@ static TPMBackend *tpm_builtin_create(Qe >> goto err_exit; >> } >> >> + value = qemu_opt_get(opts, "key"); >> + if (value) { >> + if (!strncasecmp(value, "aes-cbc:", 8)) { >> + memset(keyvalue, 0x0, sizeof(keyvalue)); >> + >> + if (!tpm_builtin_parse_as_hexkey(&value[8], keyvalue,&keysize)) { >> + keysize = 128; >> + strncpy((char *)keyvalue, value, 128/8); Here first the input is attempted to be parsed as hex key and if that fails the input string is taken as the key. It should be &value[8] here -- so that's a bug. Stefan
On Thu, Sep 01, 2011 at 10:23:51PM -0400, Stefan Berger wrote: > >>Checks are added that test > >>- whether encryption is supported follwing the revision of the directory > >> structure (rev>= 2) > >You never generate rev 1 code, right? > I did this in the previous patch that implemented rev 1 that knew > nothing about the encryption added in rev 2. > >So why keep that support around in code? > >The first version merged into qemu should be revision 0 (or 1, as you like). > I chose '1'. See patch 9: > > +#define BS_DIR_REV1 1 > + > +#define BS_DIR_REV_CURRENT BS_DIR_REV1 > + > > So I think it's the proper thing to do to increase the revision > number from 1 to 2 since it's in two separate patches (even if they > were to be applied immediately). Look, the code is not merged yet and you already have legacy with revision history to support? Why create work? > >Don't support legacy with old version of your patch. > > > >>- whether a key has been provided although all data are stored in clear-text > >>- whether a key is missing for decryption. > >> > >>In either one of the cases the backend reports an error message to the user > >>and Qemu terminates. > >> > >>-v7: > >> - cleaned up function parsing key > >> > >>-v6: > >> - changed the format of the key= to take the type of encryption into > >> account: key=aes-cbc:0x12345... and reworked code for encryption and > >> decryption of blobs; > >separate type and data: > >keytype=aes-cbc,key=0x123 to avoid introducing more option parsing. > >Also, are people likely to have the key in a file? > >If yes maybe read a key from there and skip parsing completely? > > > I think both choices should probably exist. Now what's a good file > format? Would we expect to find a hex number in there or should it > always be assumed to be a binary file? binary > >> - modified directory entry to hold a uint_8 describing the encryption > >> type (none, aes-cbc) being used for the blobs. > >> - incrementing revision of the directory to '2' indicating encryption > >> support > >> > >>-v5: > >> - -tpmdev now also gets a key parameter > >> - add documentation about key parameter > >> > >>Signed-off-by: Stefan Berger<stefanb@linux.vnet.ibm.com> > >> > >>--- > >> hw/tpm_builtin.c | 285 +++++++++++++++++++++++++++++++++++++++++++++++++++++-- > >> qemu-config.c | 10 + > >> qemu-options.hx | 22 +++- > >> tpm.c | 10 + > >> 4 files changed, 318 insertions(+), 9 deletions(-) > >> > >>Index: qemu-git/hw/tpm_builtin.c > >>=================================================================== > >>--- qemu-git.orig/hw/tpm_builtin.c > >>+++ qemu-git/hw/tpm_builtin.c > >>@@ -27,6 +27,7 @@ > >> #include "hw/pc.h" > >> #include "migration.h" > >> #include "sysemu.h" > >>+#include "aes.h" > >> > >> #include<libtpms/tpm_library.h> > >> #include<libtpms/tpm_error.h> > >>@@ -110,14 +111,27 @@ typedef struct BSDir { > >> uint16_t rev; > >> uint32_t checksum; > >> uint32_t num_entries; > >>- uint32_t reserved[10]; > >>+ uint8_t enctype; > >>+ uint8_t reserved1[3]; > >>+ uint32_t reserved[8]; > >> BSEntry entries[BS_DIR_MAX_NUM_ENTRIES]; > >> } __attribute__((packed)) BSDir; > >> > >> > >> #define BS_DIR_REV1 1 > >>+/* rev 2 added encryption */ > >>+#define BS_DIR_REV2 2 > >> > >>-#define BS_DIR_REV_CURRENT BS_DIR_REV1 > >>+ > >>+#define BS_DIR_REV_CURRENT BS_DIR_REV2 > >>+ > >>+/* above enctype */ > >>+enum BSEnctype { > >>+ BS_DIR_ENCTYPE_NONE = 0, > >>+ BS_DIR_ENCTYPE_AES_CBC, > >>+ > >>+ BS_DIR_ENCTYPE_LAST, > >>+}; > >> > >> /* local variables */ > >> > >>@@ -150,6 +164,11 @@ static const unsigned char tpm_std_fatal > >> > >> static char dev_description[80]; > >> > >>+static struct enckey { > >>+ uint8_t enctype; > >>+ AES_KEY tpm_enc_key; > >>+ AES_KEY tpm_dec_key; > >>+} enckey; > >> > >> static int tpm_builtin_load_sized_data_from_bs(BlockDriverState *bs, > >> enum BSEntryType be, > >>@@ -264,7 +283,7 @@ static uint32_t tpm_builtin_calc_dir_che > >> > >> static bool tpm_builtin_is_valid_bsdir(BSDir *dir) > >> { > >>- if (dir->rev != BS_DIR_REV_CURRENT || > >>+ if (dir->rev> BS_DIR_REV_CURRENT || > >> dir->num_entries> BS_DIR_MAX_NUM_ENTRIES) { > >> return false; > >> } > >>@@ -295,6 +314,33 @@ static bool tpm_builtin_has_valid_conten > >> return rc; > >> } > >> > >>+static bool tpm_builtin_supports_encryption(const BSDir *dir) > >>+{ > >>+ return (dir->rev>= BS_DIR_REV2); > >>+} > >>+ > >>+ > >>+static bool tpm_builtin_has_missing_key(const BSDir *dir) > >>+{ > >>+ return ((dir->enctype != BS_DIR_ENCTYPE_NONE)&& > >>+ (enckey.enctype == BS_DIR_ENCTYPE_NONE)); > >>+} > >>+ > >>+ > >>+static bool tpm_builtin_has_unnecessary_key(const BSDir *dir) > >>+{ > >>+ return (((dir->enctype == BS_DIR_ENCTYPE_NONE)&& > >>+ (enckey.enctype != BS_DIR_ENCTYPE_NONE)) || > >>+ ((!tpm_builtin_supports_encryption(dir))&& > >>+ (enckey.enctype != BS_DIR_ENCTYPE_NONE))); > >>+} > >>+ > >>+ > >>+static bool tpm_builtin_uses_unsupported_enctype(const BSDir *dir) > >>+{ > >>+ return (dir->enctype>= BS_DIR_ENCTYPE_LAST); > >>+} > >>+ > >> > >> static int tpm_builtin_create_blank_dir(BlockDriverState *bs) > >> { > >>@@ -306,6 +352,7 @@ static int tpm_builtin_create_blank_dir( > >> dir = (BSDir *)buf; > >> dir->rev = BS_DIR_REV_CURRENT; > >> dir->num_entries = 0; > >>+ dir->enctype = enckey.enctype; > >> > >> dir->checksum = tpm_builtin_calc_dir_checksum(dir); > >> > >>@@ -407,6 +454,38 @@ static int tpm_builtin_startup_bs(BlockD > >> > >> tpm_builtin_dir_be_to_cpu(dir); > >> > >>+ if (tpm_builtin_is_valid_bsdir(dir)) { > >>+ if (tpm_builtin_supports_encryption(dir)&& > >>+ tpm_builtin_has_missing_key(dir)) { > >>+ fprintf(stderr, > >>+ "tpm: the data are encrypted but I am missing the key.\n"); > >>+ rc = -EIO; > >>+ goto err_exit; > >>+ } > >>+ if (tpm_builtin_has_unnecessary_key(dir)) { > >>+ fprintf(stderr, > >>+ "tpm: I have a key but the data are not encrypted.\n"); > >>+ rc = -EIO; > >>+ goto err_exit; > >>+ } > >>+ if (tpm_builtin_supports_encryption(dir)&& > >>+ tpm_builtin_uses_unsupported_enctype(dir)) { > >>+ fprintf(stderr, > >>+ "tpm: State is encrypted with an unsupported encryption " > >>+ "scheme.\n"); > >>+ rc = -EIO; > >>+ goto err_exit; > >>+ } > >>+ if (tpm_builtin_supports_encryption(dir)&& > >>+ (dir->enctype != BS_DIR_ENCTYPE_NONE)&& > >>+ !tpm_builtin_has_valid_content(dir)) { > >>+ fprintf(stderr, "tpm: cannot read the data - " > >>+ "is this the wrong key?\n"); > >>+ rc = -EIO; > >>+ goto err_exit; > >>+ } > >>+ } > >>+ > >> if (!tpm_builtin_is_valid_bsdir(dir) || > >> !tpm_builtin_has_valid_content(dir)) { > >> /* if it's encrypted and has something else than null-content, > >>@@ -569,6 +648,105 @@ static int set_bs_entry_size_crc(BlockDr > >> } > >> > >> > >>+static int tpm_builtin_blocksize_roundup(uint8_t enctype, int plainsize) > >>+{ > >>+ switch (enctype) { > >>+ case BS_DIR_ENCTYPE_NONE: > >>+ return plainsize; > >>+ case BS_DIR_ENCTYPE_AES_CBC: > >>+ return ALIGN(plainsize, AES_BLOCK_SIZE); > >>+ default: > >>+ assert(false); > >>+ return 0; > >>+ } > >>+} > >>+ > >>+ > >>+static int tpm_builtin_bdrv_pread(BlockDriverState *bs, int64_t offset, > >>+ void *buf, int count, > >>+ enum BSEntryType type) > >>+{ > >>+ int ret; > >>+ union { > >>+ uint64_t ll[2]; > >>+ uint8_t b[16]; > >>+ } ivec; > >>+ int toread = count; > >>+ > >>+ toread = tpm_builtin_blocksize_roundup(enckey.enctype, count); > >>+ > >>+ ret = bdrv_pread(bs, offset, buf, toread); > >>+ > >>+ if (ret != toread) { > >>+ return ret; > >>+ } > >>+ > >>+ switch (enckey.enctype) { > >>+ case BS_DIR_ENCTYPE_NONE: > >>+ break; > >>+ case BS_DIR_ENCTYPE_AES_CBC: > >>+ ivec.ll[0] = cpu_to_be64(type); > >>+ ivec.ll[1] = 0; > >>+ > >>+ AES_cbc_encrypt(buf, buf, toread,&enckey.tpm_dec_key, ivec.b, 0); > >>+ break; > >>+ default: > >>+ assert(false); > >>+ } > >>+ > >>+ return count; > >>+} > >>+ > >>+ > >>+static int tpm_builtin_bdrv_pwrite(BlockDriverState *bs, int64_t offset, > >>+ void *buf, int count, > >>+ enum BSEntryType type) > >>+{ > >>+ int ret; > >>+ union { > >>+ uint64_t ll[2]; > >>+ uint8_t b[16]; > >>+ } ivec; > >>+ int towrite = count; > >>+ void *out_buf = buf; > >>+ > >>+ switch (enckey.enctype) { > >>+ case BS_DIR_ENCTYPE_NONE: > >>+ break; > >>+ case BS_DIR_ENCTYPE_AES_CBC: > >>+ ivec.ll[0] = cpu_to_be64(type); > >>+ ivec.ll[1] = 0; > >>+ > >>+ towrite = ALIGN(count, AES_BLOCK_SIZE); > >>+ > >>+ if (towrite != count) { > >>+ out_buf = g_malloc(towrite); > >>+ > >>+ if (out_buf == NULL) { > >>+ return -ENOMEM; > >>+ } > >>+ } > >>+ > >>+ AES_cbc_encrypt(buf, out_buf, towrite,&enckey.tpm_enc_key, ivec.b, 1); > >>+ break; > >>+ default: > >>+ assert(false); > >>+ } > >>+ > >>+ ret = bdrv_pwrite(bs, offset, out_buf, towrite); > >>+ > >>+ if (out_buf != buf) { > >>+ g_free(out_buf); > >>+ } > >>+ > >>+ if (ret == towrite) { > >>+ return count; > >>+ } > >>+ > >>+ return ret; > >>+} > >>+ > >>+ > >> static int tpm_builtin_load_sized_data_from_bs(BlockDriverState *bs, > >> enum BSEntryType be, > >> TPMSizedBuffer *tsb) > >>@@ -594,7 +772,7 @@ static int tpm_builtin_load_sized_data_f > >> goto err_exit; > >> } > >> > >>- tsb->buffer = g_malloc(entry.blobsize); > >>+ tsb->buffer = g_malloc(ALIGN(entry.blobsize, AES_BLOCK_SIZE)); > >> if (!tsb->buffer) { > >> rc = -ENOMEM; > >> goto err_exit; > >>@@ -602,7 +780,8 @@ static int tpm_builtin_load_sized_data_f > >> > >> tsb->size = entry.blobsize; > >> > >>- if (bdrv_pread(bs, entry.offset, tsb->buffer, tsb->size) != tsb->size) { > >>+ if (tpm_builtin_bdrv_pread(bs, entry.offset, tsb->buffer, tsb->size, be) != > >>+ tsb->size) { > >> clear_sized_buffer(tsb); > >> fprintf(stderr, "tpm: Error while reading stored data!\n"); > >> rc = -EIO; > >>@@ -667,7 +846,8 @@ static int tpm_builtin_save_sized_data_t > >> } > >> > >> if (data_len> 0) { > >>- if (bdrv_pwrite(bs, entry.offset, data, data_len) != data_len) { > >>+ if (tpm_builtin_bdrv_pwrite(bs, entry.offset, data, data_len, be) != > >>+ data_len) { > >> rc = -EIO; > >> } > >> } > >>@@ -1492,11 +1672,77 @@ static const char *tpm_builtin_create_de > >> } > >> > >> > >>+/* > >>+ * Convert a string of hex digits to its binary representation. > >>+ * The conversion stops once either the maximum size of the binary > >>+ * array has been reached or an non-hex digit was encountered. > >>+ */ > >Don't we care about non-hex following a valid key? > Do you have an example? This function is meant to convert 0x1234567 > to a binary stream. An equivalent would be 0x1234567X, since it > would terminate parsing on 'X'. But than might be a typo. 0x1234567O for example stops parsing at 'O' since this is not a digit but looks like one. > >Two empty lines in a row :) > > > Probably this is not the only occurrence... Is this a problem? Yes. They aren't helpful. checkpatch should complain about these not sure whether it does. > >>+static bool tpm_builtin_parse_as_hexkey(const char *rawkey, > >>+ unsigned char *keyvalue, > >>+ int *keysize) > >>+{ > >>+ size_t c = 0; > >>+ > >>+ /* skip over leading '0x' */ > >>+ if (!strncmp(rawkey, "0x", 2)) { > >>+ rawkey += 2; > >>+ } > >>+ > >>+ c = stream_to_bin(rawkey, keyvalue, *keysize); > >>+ > >>+ if (c == 256/4) { > >>+ *keysize = 256; > >>+ } else if (c>= 192/4) { > >>+ *keysize = 192; > >>+ } else if (c>= 128/4) { > >>+ *keysize = 128; > >>+ } else { > >>+ return false; > >Want to tell the user what went wrong? > Here's what the key parser handles: > - all keys >= 256 bits are truncated to 256 bits > - all keys >= 192 bits are truncated to 192 bits > - all keys >= 128 bits are truncated to 128 bits > - all keys < 128 bits are assumed to not be given as a hexadecimal > number but the string itself is the key, i.e. 'HELLOWORLD' becomes a > valid key. Oh my. I presume this makes sense in some world ... > >Also, you don't allow skipping leading zeroes? > An AES key should be allowed to have leading zeros, no? > >>+ } > >>+ > >>+ return true; > >Always put spaces around /. > >But where does the /4 come from? 4 bits per character? > > > c is the number of 'nibbles'. 4 bits in a nibble - that's what the > /4 comes from. > >>+} > >>+ > >>+ > >> static TPMBackend *tpm_builtin_create(QemuOpts *opts, const char *id, > >> const char *model) > >> { > >> TPMBackend *driver; > >> const char *value; > >>+ unsigned char keyvalue[256/8]; > >>+ int keysize = sizeof(keyvalue); > >> > >> driver = g_malloc(sizeof(TPMBackend)); > >> if (!driver) { > >>@@ -1523,6 +1769,33 @@ static TPMBackend *tpm_builtin_create(Qe > >> goto err_exit; > >> } > >> > >>+ value = qemu_opt_get(opts, "key"); > >>+ if (value) { > >>+ if (!strncasecmp(value, "aes-cbc:", 8)) { > >>+ memset(keyvalue, 0x0, sizeof(keyvalue)); > >>+ > >>+ if (!tpm_builtin_parse_as_hexkey(&value[8], keyvalue,&keysize)) { > >>+ keysize = 128; > >>+ strncpy((char *)keyvalue, value, 128/8); > Here first the input is attempted to be parsed as hex key and if > that fails the input string is taken as the key. It should be > &value[8] here -- so that's a bug. > > Stefan These should be different options. key_format=hex/string etc.
On 09/04/2011 12:58 PM, Michael S. Tsirkin wrote: > On Thu, Sep 01, 2011 at 10:23:51PM -0400, Stefan Berger wrote: >>>> Checks are added that test >>>> - whether encryption is supported follwing the revision of the directory >>>> structure (rev>= 2) >>> You never generate rev 1 code, right? >> I did this in the previous patch that implemented rev 1 that knew >> nothing about the encryption added in rev 2. >>> So why keep that support around in code? >>> The first version merged into qemu should be revision 0 (or 1, as you like). >> I chose '1'. See patch 9: >> >> +#define BS_DIR_REV1 1 >> + >> +#define BS_DIR_REV_CURRENT BS_DIR_REV1 >> + >> >> So I think it's the proper thing to do to increase the revision >> number from 1 to 2 since it's in two separate patches (even if they >> were to be applied immediately). > Look, the code is not merged yet and you already have > legacy with revision history to support? Why create work? > Ok, I won't build the intermediate version then. >>> Don't support legacy with old version of your patch. >>> >>>> - whether a key has been provided although all data are stored in clear-text >>>> - whether a key is missing for decryption. >>>> >>>> In either one of the cases the backend reports an error message to the user >>>> and Qemu terminates. >>>> >>>> -v7: >>>> - cleaned up function parsing key >>>> >>>> -v6: >>>> - changed the format of the key= to take the type of encryption into >>>> account: key=aes-cbc:0x12345... and reworked code for encryption and >>>> decryption of blobs; >>> separate type and data: >>> keytype=aes-cbc,key=0x123 to avoid introducing more option parsing. >>> Also, are people likely to have the key in a file? >>> If yes maybe read a key from there and skip parsing completely? >>> >> I think both choices should probably exist. Now what's a good file >> format? Would we expect to find a hex number in there or should it >> always be assumed to be a binary file? > binary So then I'd keep the possibility to pass the key via command line and add the option to read it from a file as well. >>> Two empty lines in a row :) >>> >> Probably this is not the only occurrence... Is this a problem? > Yes. They aren't helpful. checkpatch should complain about these > not sure whether it does. It so far doesn't. >>>> +static bool tpm_builtin_parse_as_hexkey(const char *rawkey, >>>> + unsigned char *keyvalue, >>>> + int *keysize) >>>> +{ >>>> + size_t c = 0; >>>> + >>>> + /* skip over leading '0x' */ >>>> + if (!strncmp(rawkey, "0x", 2)) { >>>> + rawkey += 2; >>>> + } >>>> + >>>> + c = stream_to_bin(rawkey, keyvalue, *keysize); >>>> + >>>> + if (c == 256/4) { >>>> + *keysize = 256; >>>> + } else if (c>= 192/4) { >>>> + *keysize = 192; >>>> + } else if (c>= 128/4) { >>>> + *keysize = 128; >>>> + } else { >>>> + return false; >>> Want to tell the user what went wrong? >> Here's what the key parser handles: >> - all keys>= 256 bits are truncated to 256 bits >> - all keys>= 192 bits are truncated to 192 bits >> - all keys>= 128 bits are truncated to 128 bits >> - all keys< 128 bits are assumed to not be given as a hexadecimal >> number but the string itself is the key, i.e. 'HELLOWORLD' becomes a >> valid key. > Oh my. I presume this makes sense in some world ... > So then I will require to have the exact size of a 128, 192 or 256 bit key then and maybe allow 'passwords', which really was what the last example above was falling back to. Looking around in the QCoW2 code passwords seem to be also possible there (block/qcow2.c:qcow_set_key()). >>> Also, you don't allow skipping leading zeroes? >> An AES key should be allowed to have leading zeros, no? >>>> + } >>>> + >>>> + return true; >>> Always put spaces around /. >>> But where does the /4 come from? 4 bits per character? >>> >> c is the number of 'nibbles'. 4 bits in a nibble - that's what the >> /4 comes from. >>>> +} >>>> + >>>> + >>>> static TPMBackend *tpm_builtin_create(QemuOpts *opts, const char *id, >>>> const char *model) >>>> { >>>> TPMBackend *driver; >>>> const char *value; >>>> + unsigned char keyvalue[256/8]; >>>> + int keysize = sizeof(keyvalue); >>>> >>>> driver = g_malloc(sizeof(TPMBackend)); >>>> if (!driver) { >>>> @@ -1523,6 +1769,33 @@ static TPMBackend *tpm_builtin_create(Qe >>>> goto err_exit; >>>> } >>>> >>>> + value = qemu_opt_get(opts, "key"); >>>> + if (value) { >>>> + if (!strncasecmp(value, "aes-cbc:", 8)) { >>>> + memset(keyvalue, 0x0, sizeof(keyvalue)); >>>> + >>>> + if (!tpm_builtin_parse_as_hexkey(&value[8], keyvalue,&keysize)) { >>>> + keysize = 128; >>>> + strncpy((char *)keyvalue, value, 128/8); >> Here first the input is attempted to be parsed as hex key and if >> that fails the input string is taken as the key. It should be >> &value[8] here -- so that's a bug. >> >> Stefan > These should be different options. > > key_format=hex/string > > etc. > To summarize it: enc_mode=<aes-cbc> # redundant for now since this is the only supported encryption scheme; so could drop it and assume as default key_format=<hex/binary> # hex for a string hex number; binary would mean the found string is directly converted to a char[] that is then directly used as the AES key (like a password) key=<128, 192, or 256 bit>hex key or string key_file=<path to file containing 128,192 or 256 bit hex key or string> Stefan
On Tue, Sep 06, 2011 at 08:32:41PM -0400, Stefan Berger wrote: > To summarize it: > enc_mode=<aes-cbc> # redundant for now since this is the only > supported encryption scheme; so could drop it and assume as default > > key_format=<hex/binary> # hex for a string hex number; binary would > mean the found string is directly converted to a char[] that is then > directly used as the AES key (like a password) > > key=<128, 192, or 256 bit>hex key or string > > key_file=<path to file containing 128,192 or 256 bit hex key or string> > > Stefan OK
On Thu, Sep 01, 2011 at 10:23:51PM -0400, Stefan Berger wrote: > >>An additional 'layer' for reading and writing the blobs to the underlying > >>block storage is added. This layer encrypts the blobs for writing if a key is > >>available. Similarly it decrypts the blobs after reading. So a couple of further thoughts: 1. Raw storage should work too, and with e.g. NFS migration will be fine, right? So I'd say it's worth supporting. 2. File backed nvram is interesting outside tpm. For example,vpd and chassis number for pci, eeprom emulation for network cards. Using a file per device might be inconvenient though. So please think of a format and API that will allow sections for use by different devices. 3. Home-grown file formats give us enough trouble in migration. Could this use one of the variants of ASN.1? There are portable libraries to read/write that, even.
On 09/07/2011 02:55 PM, Michael S. Tsirkin wrote: > On Thu, Sep 01, 2011 at 10:23:51PM -0400, Stefan Berger wrote: >>>> An additional 'layer' for reading and writing the blobs to the underlying >>>> block storage is added. This layer encrypts the blobs for writing if a key is >>>> available. Similarly it decrypts the blobs after reading. > So a couple of further thoughts: > 1. Raw storage should work too, and with e.g. NFS migration will be fine, right? > So I'd say it's worth supporting. NFS via shared storage, yes, but not migration via Qemu's block migration mechanism. If snapshotting was supposed to be a feature to support then that's only possible via block storage (QCoW2 in particular). Adding plain file support to the TPM code so it can store its 3 blobs into adds quite a bit of complexity to the code. The command line parameter that previously pointed to QCoW2 image file would probably have to point to a directory where files for the 3 blobs can be written into. Besides that, snapshotting would actually have to be prevented maybe through registering a (fake) file of other than QCoW2 type since the plain TPM files won't handle snapshotting correctly, either, and QEMU pretty much would have to be prevented from doing snapshotting at all. Maybe there's an API for this, but I don't know. Though why create this additional complexity? I don't mind relaxing the requirement of using a QCoW2 image and allowing for example RAW images (that then automatically prevent the snapshotting from happening) but the same code I now have would work for writing the blobs into it the single file. > 2. File backed nvram is interesting outside tpm. > For example,vpd and chassis number for pci, eeprom emulation for network cards. > Using a file per device might be inconvenient though. > So please think of a format and API that will allow sections > for use by different devices. Also here 'snapshotting' is the most 'demanding' feature of QEMU I would say. Snapshotting isn't easily supported outside of the block layer from what I understand. Once you are tied to the block layer you end up having to use images and those don't grow quite well. So other devices wanting to use those type of devices would need to know what the worst case sizes are for writing their state into -- unless an image format is created that can grow. As for the format: Ideally all devices could write into one file, right? That would at least prevent too many files besides the VM's image file from floating around which presumably makes image management easier. Following the above, you add up all the worst case sizes the individual devices may need for their blobs and create an image with that capacity. Then you need some form of a (primitive?) directory that lets you write blobs into that storage. Assuming there were well defined names for those devices one could say for example store this blobs under the name 'tpm-permanent-state' and later on load it under that name. The possible size of the directory would have to be considered as well... I do something like that for the TPM where I have up to 3 such blobs that I store. The bad thing about the above is of course the need to know what the sum of all the worst case sizes is. So a growable image format would be quite good to have. I haven't followed the conversations much, but is that something QCoW3 would support? Crazy idea: Is there a filesystem that one could use and mount a filesystem onto (some) sectors of an image? Again, the best format right now is QCoW2 for this (due to snapshotting suport) where one would have to be able to mount a filesystem onto the current snapshot's available sectors. Then at least the handling of blobs would become a lot easier. Though I doubt this would be possible without custom code and lots of development. > 3. Home-grown file formats give us enough trouble in migration. > Could this use one of the variants of ASN.1? > There are portable libraries to read/write that, even. > I am not sure what 'this' refers to. What I am doing with the TPM is writing 3 independent blobs at certain offset into the QCoW2 block file. A directory in the first sector holds the offsets, sizes and crc32's of these (unencrypted) blobs. I am not that familiar with ASN.1 except that from what I have seen it looks like a fairly terrible format needing an object language to create a parser from etc. not to mention the problems I had with snacc trying to compile the ASN.1 object language of an RFC... Stefan
On Wed, Sep 07, 2011 at 08:16:27PM -0400, Stefan Berger wrote: > On 09/07/2011 02:55 PM, Michael S. Tsirkin wrote: > >On Thu, Sep 01, 2011 at 10:23:51PM -0400, Stefan Berger wrote: > >>>>An additional 'layer' for reading and writing the blobs to the underlying > >>>>block storage is added. This layer encrypts the blobs for writing if a key is > >>>>available. Similarly it decrypts the blobs after reading. > >So a couple of further thoughts: > >1. Raw storage should work too, and with e.g. NFS migration will be fine, right? > > So I'd say it's worth supporting. > NFS via shared storage, yes, but not migration via Qemu's block > migration mechanism. If snapshotting was supposed to be a feature to > support then that's only possible via block storage (QCoW2 in > particular). As disk has the same limitation, that sounds fine. Let the user decide whether snapshoting is needed, same as disk. > Adding plain file support to the TPM code so it can store its 3 > blobs into adds quite a bit of complexity to the code. The command > line parameter that previously pointed to QCoW2 image file would > probably have to point to a directory where files for the 3 blobs > can be written into. Besides that, snapshotting would actually have > to be prevented maybe through registering a (fake) file of other > than QCoW2 type since the plain TPM files won't handle snapshotting > correctly, either, and QEMU pretty much would have to be prevented > from doing snapshotting at all. Maybe there's an API for this, but I > don't know. Though why create this additional complexity? I don't > mind relaxing the requirement of using a QCoW2 image and allowing > for example RAW images (that then automatically prevent the > snapshotting from happening) but the same code I now have would work > for writing the blobs into it the single file. Right. Write all blobs into a single files at different offsets, or something. > >2. File backed nvram is interesting outside tpm. > > For example,vpd and chassis number for pci, eeprom emulation for network cards. > > Using a file per device might be inconvenient though. > > So please think of a format and API that will allow sections > > for use by different devices. > Also here 'snapshotting' is the most 'demanding' feature of QEMU I > would say. Snapshotting isn't easily supported outside of the block > layer from what I understand. Once you are tied to the block layer > you end up having to use images and those don't grow quite well. So > other devices wanting to use those type of devices would need to > know what the worst case sizes are for writing their state into -- > unless an image format is created that can grow. > > As for the format: Ideally all devices could write into one file, > right? That would at least prevent too many files besides the VM's > image file from floating around which presumably makes image > management easier. Following the above, you add up all the worst > case sizes the individual devices may need for their blobs and > create an image with that capacity. Then you need some form of a > (primitive?) directory that lets you write blobs into that storage. > Assuming there were well defined names for those devices one could > say for example store this blobs under the name > 'tpm-permanent-state' and later on load it under that name. The > possible size of the directory would have to be considered as > well... I do something like that for the TPM where I have up to 3 > such blobs that I store. > > The bad thing about the above is of course the need to know what the > sum of all the worst case sizes is. A typical usecase I know about has prepared vpd/eeprom content. We'll typically need a tool to get binary blobs and put that into the file image. That tool can do the necessary math. We could also integrate this into qemu-img if we like. > So a growable image format would > be quite good to have. I haven't followed the conversations much, > but is that something QCoW3 would support? I don't follow - does TPM need a growable image format? Why? Hardware typically has fixed amount of memory :) > Crazy idea: Is there a filesystem that one could use and mount a > filesystem onto (some) sectors of an image? Again, the best format > right now is QCoW2 for this (due to snapshotting suport) where one > would have to be able to mount a filesystem onto the current > snapshot's available sectors. Then at least the handling of blobs > would become a lot easier. Though I doubt this would be possible > without custom code and lots of development. Hmm, libguestfs can do all kind of smart stuff. But we don't want qemu to depend on that. > >3. Home-grown file formats give us enough trouble in migration. > > Could this use one of the variants of ASN.1? > > There are portable libraries to read/write that, even. > > > I am not sure what 'this' refers to. What I am doing with the TPM is > writing 3 independent blobs at certain offset into the QCoW2 block > file. A directory in the first sector holds the offsets, sizes and > crc32's of these (unencrypted) blobs. Right. It's the encoding of the directory that is custom, and that bothers me. I'd prefer a format that is self-describing and self-delimiting, give a way to inspect the data using external tools. > I am not that familiar with ASN.1 except that from what I have seen > it looks like a fairly terrible format needing an object language to > create a parser from etc. not to mention the problems I had with > snacc trying to compile the ASN.1 object language of an RFC... > > Stefan Sorry about the confusion, we don't need the notation, I don't mean that. I mean use a subset of the ASN.1 basic encoding http://homepages.dcc.ufmg.br/~coelho/nm/asn.1.intro.pdf So we could have a set of sequences, with an ascii string (a tag) followed by an octet string (content).
On 09/08/2011 06:32 AM, Michael S. Tsirkin wrote: > On Wed, Sep 07, 2011 at 08:16:27PM -0400, Stefan Berger wrote: >> On 09/07/2011 02:55 PM, Michael S. Tsirkin wrote: >>> On Thu, Sep 01, 2011 at 10:23:51PM -0400, Stefan Berger wrote: >>>>>> An additional 'layer' for reading and writing the blobs to the underlying >>>>>> block storage is added. This layer encrypts the blobs for writing if a key is >>>>>> available. Similarly it decrypts the blobs after reading. >>> So a couple of further thoughts: >>> 1. Raw storage should work too, and with e.g. NFS migration will be fine, right? >>> So I'd say it's worth supporting. >> NFS via shared storage, yes, but not migration via Qemu's block >> migration mechanism. If snapshotting was supposed to be a feature to >> support then that's only possible via block storage (QCoW2 in >> particular). > As disk has the same limitation, that sounds fine. > Let the user decide whether snapshoting is needed, > same as disk. > >> Adding plain file support to the TPM code so it can store its 3 >> blobs into adds quite a bit of complexity to the code. The command >> line parameter that previously pointed to QCoW2 image file would >> probably have to point to a directory where files for the 3 blobs >> can be written into. Besides that, snapshotting would actually have >> to be prevented maybe through registering a (fake) file of other >> than QCoW2 type since the plain TPM files won't handle snapshotting >> correctly, either, and QEMU pretty much would have to be prevented >> from doing snapshotting at all. Maybe there's an API for this, but I >> don't know. Though why create this additional complexity? I don't >> mind relaxing the requirement of using a QCoW2 image and allowing >> for example RAW images (that then automatically prevent the >> snapshotting from happening) but the same code I now have would work >> for writing the blobs into it the single file. > Right. Write all blobs into a single files at different > offsets, or something. That's exactly what I am doing already. Just that I am doing this with Qemu's BlockStorage (bdrv) writing to sectors rather than seek()ing in files. To avoid more complexity I'd rather not introduce more code handling plain files but rely on all the image formats that qemu already supports and that give features like encryption (QCoW2 only), snapshotting (QCoW2 only) and block migration (presumably all of them). Plain files offer none of that. Devices that need to write their state to persistent storage really have to aim for doing this through Qemu's bdrv since they will otherwise be the ones killing the snapshot feature. TPM certainly doesn't want to be one of them. If the user doesn't want snapshotting to be supported since his VM image files are not QCoW2 type of files, just create a raw image file for the TPM's persistent state and bdrv will automatically prevent snapshotting. The point is that the TPM code now using the bdrv layer works with any image format already. >>> 2. File backed nvram is interesting outside tpm. >>> For example,vpd and chassis number for pci, eeprom emulation for network cards. >>> Using a file per device might be inconvenient though. >>> So please think of a format and API that will allow sections >>> for use by different devices. >> Also here 'snapshotting' is the most 'demanding' feature of QEMU I >> would say. Snapshotting isn't easily supported outside of the block >> layer from what I understand. Once you are tied to the block layer >> you end up having to use images and those don't grow quite well. So >> other devices wanting to use those type of devices would need to >> know what the worst case sizes are for writing their state into -- >> unless an image format is created that can grow. >> >> As for the format: Ideally all devices could write into one file, >> right? That would at least prevent too many files besides the VM's >> image file from floating around which presumably makes image >> management easier. Following the above, you add up all the worst >> case sizes the individual devices may need for their blobs and >> create an image with that capacity. Then you need some form of a >> (primitive?) directory that lets you write blobs into that storage. >> Assuming there were well defined names for those devices one could >> say for example store this blobs under the name >> 'tpm-permanent-state' and later on load it under that name. The >> possible size of the directory would have to be considered as >> well... I do something like that for the TPM where I have up to 3 >> such blobs that I store. >> >> The bad thing about the above is of course the need to know what the >> sum of all the worst case sizes is. > A typical usecase I know about has prepared vpd/eeprom content. > We'll typically need a tool to get binary blobs and put that into the > file image. That tool can do the necessary math. > We could also integrate this into qemu-img if we like. > >> So a growable image format would >> be quite good to have. I haven't followed the conversations much, >> but is that something QCoW3 would support? > I don't follow - does TPM need a growable image format? Why? > Hardware typically has fixed amount of memory :) Ideally the user wouldn't have to worry about creating the single file for persistent storage for all the devices at all but Qemu could 'somehow' do this. Assume the user starts the VM with a device having an EEPROM. Now that device has the need for 10k of persistent storage. So somehow with the limitations of images that don't grow you have to have created an image of at least 10k a priori. Later the user adds another device to the same VM that needs 40k of persistent storage. What now? Dispose the old image with the EPPROM data and create a new image with at least 50k to hold both their data? Or add another image with just 40k to hold that device's persistent state? I'd rather have the 10k image grow to 50k and accommodate both state blobs... >>> 3. Home-grown file formats give us enough trouble in migration. >>> Could this use one of the variants of ASN.1? >>> There are portable libraries to read/write that, even. >>> >> I am not sure what 'this' refers to. What I am doing with the TPM is >> writing 3 independent blobs at certain offset into the QCoW2 block >> file. A directory in the first sector holds the offsets, sizes and >> crc32's of these (unencrypted) blobs. > Right. It's the encoding of the directory that is custom, > and that bothers me. I'd prefer a format that is self-describing and > self-delimiting, give a way to inspect the data using external tools. Nothing would prevent us from defining a data structure for that directory as long as that data structure accommodates all use cases of today and especially tomorrow :-). >> I am not that familiar with ASN.1 except that from what I have seen >> it looks like a fairly terrible format needing an object language to >> create a parser from etc. not to mention the problems I had with >> snacc trying to compile the ASN.1 object language of an RFC... >> >> Stefan > Sorry about the confusion, we don't need the notation, I don't mean that. > I mean use a subset of the ASN.1 basic encoding > http://homepages.dcc.ufmg.br/~coelho/nm/asn.1.intro.pdf > > So we could have a set of sequences, with an ascii string (a tag) > followed by an octet string (content). > > I think the data layout in the image should be in such format that you don't have to re-write the whole content of the image if a blob is stored. I think a directory at the beginning could solve this. To make it simple one probably would need to know how big the 'directory' could be otherwise one has to get into allocation of sectors so that once the directory was to grow beyond 512 bytes that one would know where its next data are written into. The same is true for the devices' data blobs. If one knows the sizes of all the blobs one can lay them out to start and end at specific offsets in the image. And knowing the size of all the blobs helps in creating the image of correct size. Well, all this is a work-around for not having a 'filesystem'. Stefan
On Thu, Sep 08, 2011 at 08:11:00AM -0400, Stefan Berger wrote: > On 09/08/2011 06:32 AM, Michael S. Tsirkin wrote: > >On Wed, Sep 07, 2011 at 08:16:27PM -0400, Stefan Berger wrote: > >>On 09/07/2011 02:55 PM, Michael S. Tsirkin wrote: > >>>On Thu, Sep 01, 2011 at 10:23:51PM -0400, Stefan Berger wrote: > >>>>>>An additional 'layer' for reading and writing the blobs to the underlying > >>>>>>block storage is added. This layer encrypts the blobs for writing if a key is > >>>>>>available. Similarly it decrypts the blobs after reading. > >>>So a couple of further thoughts: > >>>1. Raw storage should work too, and with e.g. NFS migration will be fine, right? > >>> So I'd say it's worth supporting. > >>NFS via shared storage, yes, but not migration via Qemu's block > >>migration mechanism. If snapshotting was supposed to be a feature to > >>support then that's only possible via block storage (QCoW2 in > >>particular). > >As disk has the same limitation, that sounds fine. > >Let the user decide whether snapshoting is needed, > >same as disk. > > > >>Adding plain file support to the TPM code so it can store its 3 > >>blobs into adds quite a bit of complexity to the code. The command > >>line parameter that previously pointed to QCoW2 image file would > >>probably have to point to a directory where files for the 3 blobs > >>can be written into. Besides that, snapshotting would actually have > >>to be prevented maybe through registering a (fake) file of other > >>than QCoW2 type since the plain TPM files won't handle snapshotting > >>correctly, either, and QEMU pretty much would have to be prevented > >>from doing snapshotting at all. Maybe there's an API for this, but I > >>don't know. Though why create this additional complexity? I don't > >>mind relaxing the requirement of using a QCoW2 image and allowing > >>for example RAW images (that then automatically prevent the > >>snapshotting from happening) but the same code I now have would work > >>for writing the blobs into it the single file. > >Right. Write all blobs into a single files at different > >offsets, or something. > > That's exactly what I am doing already. Just that I am doing this > with Qemu's BlockStorage (bdrv) writing to sectors rather than > seek()ing in files. To avoid more complexity I'd rather not > introduce more code handling plain files but rely on all the image > formats that qemu already supports and that give features like > encryption (QCoW2 only), snapshotting (QCoW2 only) and block > migration (presumably all of them). Plain files offer none of that. > Devices that need to write their state to persistent storage really > have to aim for doing this through Qemu's bdrv since they will > otherwise be the ones killing the snapshot feature. TPM certainly > doesn't want to be one of them. If the user doesn't want > snapshotting to be supported since his VM image files are not QCoW2 > type of files, just create a raw image file for the TPM's persistent > state and bdrv will automatically prevent snapshotting. The point is > that the TPM code now using the bdrv layer works with any image > format already. Ah, that's fine then. I had an impression there was a qcow only limitation, not sure what in code gave me that impression. > >>>2. File backed nvram is interesting outside tpm. > >>> For example,vpd and chassis number for pci, eeprom emulation for network cards. > >>> Using a file per device might be inconvenient though. > >>> So please think of a format and API that will allow sections > >>> for use by different devices. > >>Also here 'snapshotting' is the most 'demanding' feature of QEMU I > >>would say. Snapshotting isn't easily supported outside of the block > >>layer from what I understand. Once you are tied to the block layer > >>you end up having to use images and those don't grow quite well. So > >>other devices wanting to use those type of devices would need to > >>know what the worst case sizes are for writing their state into -- > >>unless an image format is created that can grow. > >> > >>As for the format: Ideally all devices could write into one file, > >>right? That would at least prevent too many files besides the VM's > >>image file from floating around which presumably makes image > >>management easier. Following the above, you add up all the worst > >>case sizes the individual devices may need for their blobs and > >>create an image with that capacity. Then you need some form of a > >>(primitive?) directory that lets you write blobs into that storage. > >>Assuming there were well defined names for those devices one could > >>say for example store this blobs under the name > >>'tpm-permanent-state' and later on load it under that name. The > >>possible size of the directory would have to be considered as > >>well... I do something like that for the TPM where I have up to 3 > >>such blobs that I store. > >> > >>The bad thing about the above is of course the need to know what the > >>sum of all the worst case sizes is. > >A typical usecase I know about has prepared vpd/eeprom content. > >We'll typically need a tool to get binary blobs and put that into the > >file image. That tool can do the necessary math. > >We could also integrate this into qemu-img if we like. > > > >>So a growable image format would > >>be quite good to have. I haven't followed the conversations much, > >>but is that something QCoW3 would support? > >I don't follow - does TPM need a growable image format? Why? > >Hardware typically has fixed amount of memory :) > Ideally the user wouldn't have to worry about creating the single > file for persistent storage for all the devices at all but Qemu > could 'somehow' do this. > Assume the user starts the VM with a device having an EEPROM. Now > that device has the need for 10k of persistent storage. So somehow > with the limitations of images that don't grow you have to have > created an image of at least 10k a priori. Later the user adds > another device to the same VM that needs 40k of persistent storage. > What now? Dispose the old image with the EPPROM data and create a > new image with at least 50k to hold both their data? Or add another > image with just 40k to hold that device's persistent state? I'd > rather have the 10k image grow to 50k and accommodate both state > blobs... I see, yes, might be useful. But even without that, simple users - without hotplug - will be able to have a single file with all data, and advanced users will be able to have a file per device. Not ideal but I think manageable. > >>>3. Home-grown file formats give us enough trouble in migration. > >>> Could this use one of the variants of ASN.1? > >>> There are portable libraries to read/write that, even. > >>> > >>I am not sure what 'this' refers to. What I am doing with the TPM is > >>writing 3 independent blobs at certain offset into the QCoW2 block > >>file. A directory in the first sector holds the offsets, sizes and > >>crc32's of these (unencrypted) blobs. By the way, why do we checksum data? Should be optional? > >Right. It's the encoding of the directory that is custom, > >and that bothers me. I'd prefer a format that is self-describing and > >self-delimiting, give a way to inspect the data using external tools. > Nothing would prevent us from defining a data structure for that > directory as long as that data structure accommodates all use cases > of today and especially tomorrow :-). Right, so please give thought to the proposal of using a subset of BER. > >>I am not that familiar with ASN.1 except that from what I have seen > >>it looks like a fairly terrible format needing an object language to > >>create a parser from etc. not to mention the problems I had with > >>snacc trying to compile the ASN.1 object language of an RFC... > >> > >> Stefan > >Sorry about the confusion, we don't need the notation, I don't mean that. > >I mean use a subset of the ASN.1 basic encoding > >http://homepages.dcc.ufmg.br/~coelho/nm/asn.1.intro.pdf > > > >So we could have a set of sequences, with an ascii string (a tag) > >followed by an octet string (content). > > > > > I think the data layout in the image should be in such format that > you don't have to re-write the whole content of the image if a blob > is stored. With predefined blob size, we can use octet strings and not have to rewrite anything, find the right octet and change it inplace. > I think a directory at the beginning could solve this. It could, but it's not needed for that. > To make it simple one probably would need to know how big the > 'directory' could be otherwise one has to get into allocation of > sectors so that once the directory was to grow beyond 512 bytes that > one would know where its next data are written into. The same is > true for the devices' data blobs. If one knows the sizes of all the > blobs one can lay them out to start and end at specific offsets in > the image. And knowing the size of all the blobs helps in creating > the image of correct size. > Well, all this is a work-around for not having a 'filesystem'. > > Stefan > Sounds like overkill. A sequence with tags in DER format is much easier.
On 09/08/2011 09:16 AM, Michael S. Tsirkin wrote: > On Thu, Sep 08, 2011 at 08:11:00AM -0400, Stefan Berger wrote: >> On 09/08/2011 06:32 AM, Michael S. Tsirkin wrote: >>> On Wed, Sep 07, 2011 at 08:16:27PM -0400, Stefan Berger wrote: >>>> On 09/07/2011 02:55 PM, Michael S. Tsirkin wrote: >>>>> On Thu, Sep 01, 2011 at 10:23:51PM -0400, Stefan Berger wrote: >>>>>>>> An additional 'layer' for reading and writing the blobs to the underlying >>>>>>>> block storage is added. This layer encrypts the blobs for writing if a key is >>>>>>>> available. Similarly it decrypts the blobs after reading. >>>>> So a couple of further thoughts: >>>>> 1. Raw storage should work too, and with e.g. NFS migration will be fine, right? >>>>> So I'd say it's worth supporting. >>>> NFS via shared storage, yes, but not migration via Qemu's block >>>> migration mechanism. If snapshotting was supposed to be a feature to >>>> support then that's only possible via block storage (QCoW2 in >>>> particular). >>> As disk has the same limitation, that sounds fine. >>> Let the user decide whether snapshoting is needed, >>> same as disk. >>> >>>> Adding plain file support to the TPM code so it can store its 3 >>>> blobs into adds quite a bit of complexity to the code. The command >>>> line parameter that previously pointed to QCoW2 image file would >>>> probably have to point to a directory where files for the 3 blobs >>>> can be written into. Besides that, snapshotting would actually have >>>> to be prevented maybe through registering a (fake) file of other >>>> than QCoW2 type since the plain TPM files won't handle snapshotting >>>> correctly, either, and QEMU pretty much would have to be prevented >>> >from doing snapshotting at all. Maybe there's an API for this, but I >>>> don't know. Though why create this additional complexity? I don't >>>> mind relaxing the requirement of using a QCoW2 image and allowing >>>> for example RAW images (that then automatically prevent the >>>> snapshotting from happening) but the same code I now have would work >>>> for writing the blobs into it the single file. >>> Right. Write all blobs into a single files at different >>> offsets, or something. >> That's exactly what I am doing already. Just that I am doing this >> with Qemu's BlockStorage (bdrv) writing to sectors rather than >> seek()ing in files. To avoid more complexity I'd rather not >> introduce more code handling plain files but rely on all the image >> formats that qemu already supports and that give features like >> encryption (QCoW2 only), snapshotting (QCoW2 only) and block >> migration (presumably all of them). Plain files offer none of that. >> Devices that need to write their state to persistent storage really >> have to aim for doing this through Qemu's bdrv since they will >> otherwise be the ones killing the snapshot feature. TPM certainly >> doesn't want to be one of them. If the user doesn't want >> snapshotting to be supported since his VM image files are not QCoW2 >> type of files, just create a raw image file for the TPM's persistent >> state and bdrv will automatically prevent snapshotting. The point is >> that the TPM code now using the bdrv layer works with any image >> format already. > Ah, that's fine then. I had an impression there was a qcow only > limitation, not sure what in code gave me that impression. Hm, currently I force the image to be a QCoW2. bdrv_get_format(bs, buf, sizeof(buf)); if (strcmp(buf, "qcow2")) { fprintf(stderr, "vTPM backing store must be of type qcow2\n"); goto err_exit; } I can remove this and we should be fine. >>>>> 2. File backed nvram is interesting outside tpm. >>>>> For example,vpd and chassis number for pci, eeprom emulation for network cards. >>>>> Using a file per device might be inconvenient though. >>>>> So please think of a format and API that will allow sections >>>>> for use by different devices. >>>> Also here 'snapshotting' is the most 'demanding' feature of QEMU I >>>> would say. Snapshotting isn't easily supported outside of the block >>>> layer from what I understand. Once you are tied to the block layer >>>> you end up having to use images and those don't grow quite well. So >>>> other devices wanting to use those type of devices would need to >>>> know what the worst case sizes are for writing their state into -- >>>> unless an image format is created that can grow. >>>> >>>> As for the format: Ideally all devices could write into one file, >>>> right? That would at least prevent too many files besides the VM's >>>> image file from floating around which presumably makes image >>>> management easier. Following the above, you add up all the worst >>>> case sizes the individual devices may need for their blobs and >>>> create an image with that capacity. Then you need some form of a >>>> (primitive?) directory that lets you write blobs into that storage. >>>> Assuming there were well defined names for those devices one could >>>> say for example store this blobs under the name >>>> 'tpm-permanent-state' and later on load it under that name. The >>>> possible size of the directory would have to be considered as >>>> well... I do something like that for the TPM where I have up to 3 >>>> such blobs that I store. >>>> >>>> The bad thing about the above is of course the need to know what the >>>> sum of all the worst case sizes is. >>> A typical usecase I know about has prepared vpd/eeprom content. >>> We'll typically need a tool to get binary blobs and put that into the >>> file image. That tool can do the necessary math. >>> We could also integrate this into qemu-img if we like. >>> >>>> So a growable image format would >>>> be quite good to have. I haven't followed the conversations much, >>>> but is that something QCoW3 would support? >>> I don't follow - does TPM need a growable image format? Why? >>> Hardware typically has fixed amount of memory :) >> Ideally the user wouldn't have to worry about creating the single >> file for persistent storage for all the devices at all but Qemu >> could 'somehow' do this. >> Assume the user starts the VM with a device having an EEPROM. Now >> that device has the need for 10k of persistent storage. So somehow >> with the limitations of images that don't grow you have to have >> created an image of at least 10k a priori. Later the user adds >> another device to the same VM that needs 40k of persistent storage. >> What now? Dispose the old image with the EPPROM data and create a >> new image with at least 50k to hold both their data? Or add another >> image with just 40k to hold that device's persistent state? I'd >> rather have the 10k image grow to 50k and accommodate both state >> blobs... > I see, yes, might be useful. But even without that, > simple users - without hotplug - will be able to have > a single file with all data, and advanced users will > be able to have a file per device. Not ideal > but I think manageable. > As long as the management software keeps everything in one place (dir with subdirs?) it should be manageable. User just need to remember to pass on several files then for a single VM. >>>>> 3. Home-grown file formats give us enough trouble in migration. >>>>> Could this use one of the variants of ASN.1? >>>>> There are portable libraries to read/write that, even. >>>>> >>>> I am not sure what 'this' refers to. What I am doing with the TPM is >>>> writing 3 independent blobs at certain offset into the QCoW2 block >>>> file. A directory in the first sector holds the offsets, sizes and >>>> crc32's of these (unencrypted) blobs. > By the way, why do we checksum data? Should be optional? > It lets one detect corruption. In case encryption is being used for the individual blobs (that's NOT the QCoW2 native encryption but the one I added and that works for RAW images as well) I can use this crc32 after trying to decrypt the data. If the crc32 doesn't match the expected one it was either the wrong key or the data is corrupted. At least I can react to it and tell the user that the data could not be decrypted or are corrupted and terminate Qemu rather than having the device go into panic/shutdown mode on the bad state. >>>> I am not that familiar with ASN.1 except that from what I have seen >>>> it looks like a fairly terrible format needing an object language to >>>> create a parser from etc. not to mention the problems I had with >>>> snacc trying to compile the ASN.1 object language of an RFC... >>>> >>>> Stefan >>> Sorry about the confusion, we don't need the notation, I don't mean that. >>> I mean use a subset of the ASN.1 basic encoding >>> http://homepages.dcc.ufmg.br/~coelho/nm/asn.1.intro.pdf >>> >>> So we could have a set of sequences, with an ascii string (a tag) >>> followed by an octet string (content). >>> >>> >> I think the data layout in the image should be in such format that >> you don't have to re-write the whole content of the image if a blob >> is stored. > With predefined blob size, we can use octet strings and not > have to rewrite anything, find the right octet and change it inplace. > >> I think a directory at the beginning could solve this. > It could, but it's not needed for that. > >> To make it simple one probably would need to know how big the >> 'directory' could be otherwise one has to get into allocation of >> sectors so that once the directory was to grow beyond 512 bytes that >> one would know where its next data are written into. The same is >> true for the devices' data blobs. If one knows the sizes of all the >> blobs one can lay them out to start and end at specific offsets in >> the image. And knowing the size of all the blobs helps in creating >> the image of correct size. >> Well, all this is a work-around for not having a 'filesystem'. >> >> Stefan >> > Sounds like overkill. A sequence with tags in DER format is much easier. > What data should be encoded using DER? - indication whether encryption was used on all blobs - a sequence of - name of the blob - worst-case size of the blob - actual size of the blob - crc32 of the blob - the actual blob ? Stefan
Index: qemu-git/hw/tpm_builtin.c =================================================================== --- qemu-git.orig/hw/tpm_builtin.c +++ qemu-git/hw/tpm_builtin.c @@ -27,6 +27,7 @@ #include "hw/pc.h" #include "migration.h" #include "sysemu.h" +#include "aes.h" #include <libtpms/tpm_library.h> #include <libtpms/tpm_error.h> @@ -110,14 +111,27 @@ typedef struct BSDir { uint16_t rev; uint32_t checksum; uint32_t num_entries; - uint32_t reserved[10]; + uint8_t enctype; + uint8_t reserved1[3]; + uint32_t reserved[8]; BSEntry entries[BS_DIR_MAX_NUM_ENTRIES]; } __attribute__((packed)) BSDir; #define BS_DIR_REV1 1 +/* rev 2 added encryption */ +#define BS_DIR_REV2 2 -#define BS_DIR_REV_CURRENT BS_DIR_REV1 + +#define BS_DIR_REV_CURRENT BS_DIR_REV2 + +/* above enctype */ +enum BSEnctype { + BS_DIR_ENCTYPE_NONE = 0, + BS_DIR_ENCTYPE_AES_CBC, + + BS_DIR_ENCTYPE_LAST, +}; /* local variables */ @@ -150,6 +164,11 @@ static const unsigned char tpm_std_fatal static char dev_description[80]; +static struct enckey { + uint8_t enctype; + AES_KEY tpm_enc_key; + AES_KEY tpm_dec_key; +} enckey; static int tpm_builtin_load_sized_data_from_bs(BlockDriverState *bs, enum BSEntryType be, @@ -264,7 +283,7 @@ static uint32_t tpm_builtin_calc_dir_che static bool tpm_builtin_is_valid_bsdir(BSDir *dir) { - if (dir->rev != BS_DIR_REV_CURRENT || + if (dir->rev > BS_DIR_REV_CURRENT || dir->num_entries > BS_DIR_MAX_NUM_ENTRIES) { return false; } @@ -295,6 +314,33 @@ static bool tpm_builtin_has_valid_conten return rc; } +static bool tpm_builtin_supports_encryption(const BSDir *dir) +{ + return (dir->rev >= BS_DIR_REV2); +} + + +static bool tpm_builtin_has_missing_key(const BSDir *dir) +{ + return ((dir->enctype != BS_DIR_ENCTYPE_NONE) && + (enckey.enctype == BS_DIR_ENCTYPE_NONE)); +} + + +static bool tpm_builtin_has_unnecessary_key(const BSDir *dir) +{ + return (((dir->enctype == BS_DIR_ENCTYPE_NONE) && + (enckey.enctype != BS_DIR_ENCTYPE_NONE)) || + ((!tpm_builtin_supports_encryption(dir)) && + (enckey.enctype != BS_DIR_ENCTYPE_NONE))); +} + + +static bool tpm_builtin_uses_unsupported_enctype(const BSDir *dir) +{ + return (dir->enctype >= BS_DIR_ENCTYPE_LAST); +} + static int tpm_builtin_create_blank_dir(BlockDriverState *bs) { @@ -306,6 +352,7 @@ static int tpm_builtin_create_blank_dir( dir = (BSDir *)buf; dir->rev = BS_DIR_REV_CURRENT; dir->num_entries = 0; + dir->enctype = enckey.enctype; dir->checksum = tpm_builtin_calc_dir_checksum(dir); @@ -407,6 +454,38 @@ static int tpm_builtin_startup_bs(BlockD tpm_builtin_dir_be_to_cpu(dir); + if (tpm_builtin_is_valid_bsdir(dir)) { + if (tpm_builtin_supports_encryption(dir) && + tpm_builtin_has_missing_key(dir)) { + fprintf(stderr, + "tpm: the data are encrypted but I am missing the key.\n"); + rc = -EIO; + goto err_exit; + } + if (tpm_builtin_has_unnecessary_key(dir)) { + fprintf(stderr, + "tpm: I have a key but the data are not encrypted.\n"); + rc = -EIO; + goto err_exit; + } + if (tpm_builtin_supports_encryption(dir) && + tpm_builtin_uses_unsupported_enctype(dir)) { + fprintf(stderr, + "tpm: State is encrypted with an unsupported encryption " + "scheme.\n"); + rc = -EIO; + goto err_exit; + } + if (tpm_builtin_supports_encryption(dir) && + (dir->enctype != BS_DIR_ENCTYPE_NONE) && + !tpm_builtin_has_valid_content(dir)) { + fprintf(stderr, "tpm: cannot read the data - " + "is this the wrong key?\n"); + rc = -EIO; + goto err_exit; + } + } + if (!tpm_builtin_is_valid_bsdir(dir) || !tpm_builtin_has_valid_content(dir)) { /* if it's encrypted and has something else than null-content, @@ -569,6 +648,105 @@ static int set_bs_entry_size_crc(BlockDr } +static int tpm_builtin_blocksize_roundup(uint8_t enctype, int plainsize) +{ + switch (enctype) { + case BS_DIR_ENCTYPE_NONE: + return plainsize; + case BS_DIR_ENCTYPE_AES_CBC: + return ALIGN(plainsize, AES_BLOCK_SIZE); + default: + assert(false); + return 0; + } +} + + +static int tpm_builtin_bdrv_pread(BlockDriverState *bs, int64_t offset, + void *buf, int count, + enum BSEntryType type) +{ + int ret; + union { + uint64_t ll[2]; + uint8_t b[16]; + } ivec; + int toread = count; + + toread = tpm_builtin_blocksize_roundup(enckey.enctype, count); + + ret = bdrv_pread(bs, offset, buf, toread); + + if (ret != toread) { + return ret; + } + + switch (enckey.enctype) { + case BS_DIR_ENCTYPE_NONE: + break; + case BS_DIR_ENCTYPE_AES_CBC: + ivec.ll[0] = cpu_to_be64(type); + ivec.ll[1] = 0; + + AES_cbc_encrypt(buf, buf, toread, &enckey.tpm_dec_key, ivec.b, 0); + break; + default: + assert(false); + } + + return count; +} + + +static int tpm_builtin_bdrv_pwrite(BlockDriverState *bs, int64_t offset, + void *buf, int count, + enum BSEntryType type) +{ + int ret; + union { + uint64_t ll[2]; + uint8_t b[16]; + } ivec; + int towrite = count; + void *out_buf = buf; + + switch (enckey.enctype) { + case BS_DIR_ENCTYPE_NONE: + break; + case BS_DIR_ENCTYPE_AES_CBC: + ivec.ll[0] = cpu_to_be64(type); + ivec.ll[1] = 0; + + towrite = ALIGN(count, AES_BLOCK_SIZE); + + if (towrite != count) { + out_buf = g_malloc(towrite); + + if (out_buf == NULL) { + return -ENOMEM; + } + } + + AES_cbc_encrypt(buf, out_buf, towrite, &enckey.tpm_enc_key, ivec.b, 1); + break; + default: + assert(false); + } + + ret = bdrv_pwrite(bs, offset, out_buf, towrite); + + if (out_buf != buf) { + g_free(out_buf); + } + + if (ret == towrite) { + return count; + } + + return ret; +} + + static int tpm_builtin_load_sized_data_from_bs(BlockDriverState *bs, enum BSEntryType be, TPMSizedBuffer *tsb) @@ -594,7 +772,7 @@ static int tpm_builtin_load_sized_data_f goto err_exit; } - tsb->buffer = g_malloc(entry.blobsize); + tsb->buffer = g_malloc(ALIGN(entry.blobsize, AES_BLOCK_SIZE)); if (!tsb->buffer) { rc = -ENOMEM; goto err_exit; @@ -602,7 +780,8 @@ static int tpm_builtin_load_sized_data_f tsb->size = entry.blobsize; - if (bdrv_pread(bs, entry.offset, tsb->buffer, tsb->size) != tsb->size) { + if (tpm_builtin_bdrv_pread(bs, entry.offset, tsb->buffer, tsb->size, be) != + tsb->size) { clear_sized_buffer(tsb); fprintf(stderr, "tpm: Error while reading stored data!\n"); rc = -EIO; @@ -667,7 +846,8 @@ static int tpm_builtin_save_sized_data_t } if (data_len > 0) { - if (bdrv_pwrite(bs, entry.offset, data, data_len) != data_len) { + if (tpm_builtin_bdrv_pwrite(bs, entry.offset, data, data_len, be) != + data_len) { rc = -EIO; } } @@ -1492,11 +1672,77 @@ static const char *tpm_builtin_create_de } +/* + * Convert a string of hex digits to its binary representation. + * The conversion stops once either the maximum size of the binary + * array has been reached or an non-hex digit was encountered. + */ +static size_t stream_to_bin(const char *stream, + unsigned char *bin, size_t bin_size) +{ + size_t c = 0; + unsigned char nib = 0; + + while (c < bin_size && stream[c] != 0) { + if (stream[c] >= '0' && stream[c] <= '9') { + nib |= stream[c] - '0'; + } else if (stream[c] >= 'A' && stream[c] <= 'F') { + nib |= stream[c] - 'A' + 10; + } else if (stream[c] >= 'a' && stream[c] <= 'f') { + nib |= stream[c] - 'a' + 10; + } else { + break; + } + + if ((c & 1) == 1) { + bin[c/2] = nib; + nib = 0; + } else { + nib <<= 4; + bin[c/2] = nib; + } + + c++; + } + + return c; +} + + +static bool tpm_builtin_parse_as_hexkey(const char *rawkey, + unsigned char *keyvalue, + int *keysize) +{ + size_t c = 0; + + /* skip over leading '0x' */ + if (!strncmp(rawkey, "0x", 2)) { + rawkey += 2; + } + + c = stream_to_bin(rawkey, keyvalue, *keysize); + + if (c == 256/4) { + *keysize = 256; + } else if (c >= 192/4) { + *keysize = 192; + } else if (c >= 128/4) { + *keysize = 128; + } else { + return false; + } + + return true; +} + + static TPMBackend *tpm_builtin_create(QemuOpts *opts, const char *id, const char *model) { TPMBackend *driver; const char *value; + unsigned char keyvalue[256/8]; + int keysize = sizeof(keyvalue); driver = g_malloc(sizeof(TPMBackend)); if (!driver) { @@ -1523,6 +1769,33 @@ static TPMBackend *tpm_builtin_create(Qe goto err_exit; } + value = qemu_opt_get(opts, "key"); + if (value) { + if (!strncasecmp(value, "aes-cbc:", 8)) { + memset(keyvalue, 0x0, sizeof(keyvalue)); + + if (!tpm_builtin_parse_as_hexkey(&value[8], keyvalue, &keysize)) { + keysize = 128; + strncpy((char *)keyvalue, value, 128/8); + } + + if (AES_set_encrypt_key(keyvalue, keysize, + &enckey.tpm_enc_key) != 0 || + AES_set_decrypt_key(keyvalue, keysize, + &enckey.tpm_dec_key) != 0) { + fprintf(stderr, "tpm: Error setting AES key.\n"); + goto err_exit; + } + enckey.enctype = BS_DIR_ENCTYPE_AES_CBC; + } else { + fprintf(stderr, "tpm: Unknown encryption scheme. Known types are: " + "aes-cbc.\n"); + goto err_exit; + } + } else { + enckey.enctype = BS_DIR_ENCTYPE_NONE; + } + return driver; err_exit: Index: qemu-git/qemu-config.c =================================================================== --- qemu-git.orig/qemu-config.c +++ qemu-git/qemu-config.c @@ -522,6 +522,11 @@ static QemuOptsList qemu_tpmdev_opts = { .type = QEMU_OPT_STRING, .help = "Persistent storage for TPM state", }, + { + .name = "key", + .type = QEMU_OPT_STRING, + .help = "Data encryption key", + }, { /* end of list */ } }, }; @@ -546,6 +551,11 @@ static QemuOptsList qemu_tpm_opts = { .type = QEMU_OPT_STRING, .help = "Persistent storage for TPM state", }, + { + .name = "key", + .type = QEMU_OPT_STRING, + .help = "Data encryption key", + }, { /* end of list */ } }, }; Index: qemu-git/tpm.c =================================================================== --- qemu-git.orig/tpm.c +++ qemu-git/tpm.c @@ -245,6 +245,7 @@ void tpm_cleanup(void) void tpm_config_parse(QemuOptsList *opts_list, const char *optarg) { QemuOpts *opts; + char *key; if (strcmp("none", optarg) != 0) { if (*optarg == '?') { @@ -255,6 +256,15 @@ void tpm_config_parse(QemuOptsList *opts if (!opts) { exit(1); } + + /* if a key is provided, wipe it out so no one can see it with 'ps' */ + key = strstr(optarg, "key="); + if (key) { + key += 4; + while (key[0] && key[0] != ',') { + *key++ = '-'; + } + } } } Index: qemu-git/qemu-options.hx =================================================================== --- qemu-git.orig/qemu-options.hx +++ qemu-git/qemu-options.hx @@ -1766,8 +1766,9 @@ DEFHEADING(TPM device options:) # ifdef CONFIG_TPM DEF("tpm", HAS_ARG, QEMU_OPTION_tpm, \ "" \ - "-tpm builtin,path=<path>[,model=<model>]\n" \ + "-tpm builtin,path=<path>[,model=<model>][,key=<aes key>]\n" \ " enable a builtin TPM with state in file in path\n" \ + " and encrypt the TPM's state with the given AES key\n" \ "-tpm model=? to list available TPM device models\n" \ "-tpm ? to list available TPM backend types\n", QEMU_ARCH_I386) @@ -1796,13 +1797,22 @@ Use ? to print all available TPM backend qemu -tpmdev ? @end example -@item -tpmdev builtin ,id=@var{id}, path=@var{path} +@item -tpmdev builtin ,id=@var{id}, path=@var{path} [,key=@var{key}] Creates an instance of the built-in TPM. @option{path} specifies the path to the QCoW2 image that will store the TPM's persistent data. @option{path} is required. +@option{key} specifies the AES key to use to encrypt the TPM's persistent +data. If encryption is to be used, the key must be provided the first +time a Qemu VM with attached TPM is started and the same key must subsequently +be used. The format of the key is the type of encryption to use, i.e., +@code{aes-cbc}, followed by a colon and then the actual key. The key can +be a hex number with optional leading @code{0x} +and 32, 48 or 64 hex digits for 128, 192 or 256 bit AES keys respectively. +@option{key} is optional. + To create a built-in TPM use the following two options: @example -tpmdev builtin,id=tpm0,path=<path_to_qcow2> -device tpm-tis,tpmdev=tpm0 @@ -1810,12 +1820,18 @@ To create a built-in TPM use the followi Not that the @code{-tpmdev} id is @code{tpm0} and is referenced by @code{tpmdev=tpm0} in the device option. + +To create a built-in TPM whose state is encrypted with a 128 bit AES key +using AES-CBC encryption scheme supply the following two options: +@example +-tpmdev builtin,id=tpm0,path=<path_to_qcow2>,key=aes-cbc:0x1234567890abcdef01234567890abcdef -device tpm-tis,tpmdev=tpm0 +@end example @end table The short form of a TPM device option is: @table @option -@item -tpm @var{backend-type}, path=@var{path} [,model=@var{model}] +@item -tpm @var{backend-type}, path=@var{path} [,model=@var{model}] [,key=@var{key}] @findex -tpm @option{model} specifies the device model. The default device model is a
This patch adds encryption of the individual state blobs that are written into the block storage. The 'directory' at the beginnig of the block storage is not encrypted. The encryption support added in this patch would also work if QCoW2 was not to be used as the (only) image file format to store the TPM's state. Keys can be passed as a string of hexadecimal digits forming a 256, 192 or 128 bit AES key. The string can optionally start with '0x'. If the parser does not recognize it as a hexadecimal number, the string itself is taken as the AES key, which makes for example 'my_key' a valid AES key parameter. It is also necessary to provide the encryption scheme. Currently only 'aes-cbc' is supported. An example for a valid key command line argument is: -tpm builtin,key=aes-cbc:0x1234567890abcdef123456 The key passed via command line argument is wiped from the command line after parsing. If for example key=aes-cbc:0x1234... was passed it will then be changed to key=------... so that 'ps' does not show the key anymore. Obviously it cannot be completely prevented that the key is visible during a very short period of time until qemu gets to the point where the code wiping the key is reached. A byte indicating the encryption type being used is introduced in the directory structure indicating whether blobs are encrypted and if so, what encryption type was used, i.e., aes-cbc. An additional 'layer' for reading and writing the blobs to the underlying block storage is added. This layer encrypts the blobs for writing if a key is available. Similarly it decrypts the blobs after reading. Checks are added that test - whether encryption is supported follwing the revision of the directory structure (rev >= 2) - whether a key has been provided although all data are stored in clear-text - whether a key is missing for decryption. In either one of the cases the backend reports an error message to the user and Qemu terminates. -v7: - cleaned up function parsing key -v6: - changed the format of the key= to take the type of encryption into account: key=aes-cbc:0x12345... and reworked code for encryption and decryption of blobs; - modified directory entry to hold a uint_8 describing the encryption type (none, aes-cbc) being used for the blobs. - incrementing revision of the directory to '2' indicating encryption support -v5: - -tpmdev now also gets a key parameter - add documentation about key parameter Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- hw/tpm_builtin.c | 285 +++++++++++++++++++++++++++++++++++++++++++++++++++++-- qemu-config.c | 10 + qemu-options.hx | 22 +++- tpm.c | 10 + 4 files changed, 318 insertions(+), 9 deletions(-)