Patchwork [V8,10/14] Encrypt state blobs using AES CBC encryption

login
register
mail settings
Submitter Stefan Berger
Date Aug. 31, 2011, 2:36 p.m.
Message ID <20110831143623.043146580@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/112573/
State New
Headers show

Comments

Stefan Berger - Aug. 31, 2011, 2:36 p.m.
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(-)
Michael S. Tsirkin - Sept. 1, 2011, 7:26 p.m.
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
>
Stefan Berger - Sept. 2, 2011, 2:23 a.m.
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
Michael S. Tsirkin - Sept. 4, 2011, 4:58 p.m.
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.
Stefan Berger - Sept. 7, 2011, 12:32 a.m.
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
Michael S. Tsirkin - Sept. 7, 2011, 11:59 a.m.
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
Michael S. Tsirkin - Sept. 7, 2011, 6:55 p.m.
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.
Stefan Berger - Sept. 8, 2011, 12:16 a.m.
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
Michael S. Tsirkin - Sept. 8, 2011, 10:32 a.m.
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).
Stefan Berger - Sept. 8, 2011, 12:11 p.m.
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
Michael S. Tsirkin - Sept. 8, 2011, 1:16 p.m.
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.
Stefan Berger - Sept. 8, 2011, 3:27 p.m.
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

Patch

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