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 <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?
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
>
