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<stef...@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.
--
MST
