[Qemu-devel] [PATCH v2 1/3] nvram: Add TPM NVRAM implementation

2013-06-05 Thread Corey Bryant
Provides TPM NVRAM implementation that enables storing of TPM
NVRAM data in a persistent image file.  The block driver is
used to read/write the drive image.  This will enable, for
example, an encrypted QCOW2 image to be used to store sensitive
keys.

This patch provides APIs that a TPM backend can use to read and
write data.

Signed-off-by: Corey Bryant 
---
v2
 -Use non bit-rotting DPRINTF (stefa...@redhat.com)
 -Use DIV_ROUND_UP (stefa...@redhat.com)
 -Use bdrv_pread/bdrv_pwrite in coroutine - causes global
  sector to byte I/O changes (stefa...@redhat.com, kw...@redhat.com)
 -Add tpm_nvram_required_size_kb() and update tpm_nvram_adjust_size()
 -Drop qemu_aio_wait() from tpm_nvram_do_co_read/write() and move
  any completion code path to coroutines
 -Replace tpm_nvram_rwrequest_free() with g_free(rwr)
 -Remove unneeded arg checks from tpm_nvram_read/write()
  (stefa...@redhat.com)
 -Init rwr->rc to -EINPROGRESS and wrap qemu_cond_wait() with check
  (stefa...@redhat.com)
 -Add init of completion_mutex and completion condition
---
 hw/tpm/Makefile.objs |1 +
 hw/tpm/tpm_nvram.c   |  326 ++
 hw/tpm/tpm_nvram.h   |   25 
 3 files changed, 352 insertions(+), 0 deletions(-)
 create mode 100644 hw/tpm/tpm_nvram.c
 create mode 100644 hw/tpm/tpm_nvram.h

diff --git a/hw/tpm/Makefile.objs b/hw/tpm/Makefile.objs
index 99f5983..49faef4 100644
--- a/hw/tpm/Makefile.objs
+++ b/hw/tpm/Makefile.objs
@@ -1,2 +1,3 @@
 common-obj-$(CONFIG_TPM_TIS) += tpm_tis.o
+common-obj-$(CONFIG_TPM_TIS) += tpm_nvram.o
 common-obj-$(CONFIG_TPM_PASSTHROUGH) += tpm_passthrough.o
diff --git a/hw/tpm/tpm_nvram.c b/hw/tpm/tpm_nvram.c
new file mode 100644
index 000..01f0dee
--- /dev/null
+++ b/hw/tpm/tpm_nvram.c
@@ -0,0 +1,326 @@
+/*
+ * TPM NVRAM - enables storage of persistent NVRAM data on an image file
+ *
+ * Copyright (C) 2013 IBM Corporation
+ *
+ * Authors:
+ *  Stefan Berger
+ *  Corey Bryant 
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "tpm_nvram.h"
+#include "block/block_int.h"
+#include "qemu/thread.h"
+#include "sysemu/sysemu.h"
+
+#define TPM_NVRAM_DEBUG 0
+#define DPRINTF(fmt, ...) \
+do { \
+if (TPM_NVRAM_DEBUG) { \
+fprintf(stderr, fmt, ## __VA_ARGS__); \
+} \
+} while (0)
+
+/* Read/write request data */
+typedef struct TPMNvramRWRequest {
+BlockDriverState *bdrv;
+bool is_write;
+uint64_t offset;
+uint8_t **blob_r;
+uint8_t *blob_w;
+uint32_t size;
+int rc;
+
+QemuMutex completion_mutex;
+QemuCond completion;
+
+QSIMPLEQ_ENTRY(TPMNvramRWRequest) list;
+} TPMNvramRWRequest;
+
+/* Mutex protected queue of read/write requests */
+static QemuMutex tpm_nvram_rwrequests_mutex;
+static QSIMPLEQ_HEAD(, TPMNvramRWRequest) tpm_nvram_rwrequests =
+QSIMPLEQ_HEAD_INITIALIZER(tpm_nvram_rwrequests);
+
+static QEMUBH *tpm_nvram_bh;
+
+/*
+ * Get the disk size in kilobytes needed to store a blob (rounded up to next 
kb)
+ */
+static uint64_t tpm_nvram_required_size_kb(uint64_t offset, uint32_t size)
+{
+uint64_t required_size = offset + size;
+return DIV_ROUND_UP(required_size, 1024);
+}
+
+/*
+ * Increase the drive size if it's too small to store the blob
+ */
+static int tpm_nvram_adjust_size(BlockDriverState *bdrv, uint64_t offset,
+ uint32_t size)
+{
+int rc = 0;
+int64_t drive_size, required_size;
+
+rc = bdrv_getlength(bdrv);
+if (rc < 0) {
+DPRINTF("%s: Unable to determine TPM NVRAM drive size\n", __func__);
+return rc;
+}
+
+drive_size = rc;
+required_size = tpm_nvram_required_size_kb(offset, size) * 1024;
+
+if (drive_size < required_size) {
+rc = bdrv_truncate(bdrv, required_size);
+if (rc < 0) {
+DPRINTF("%s: TPM NVRAM drive too small\n", __func__);
+}
+}
+
+return rc;
+}
+
+/*
+ * Coroutine that reads a blob from the drive asynchronously
+ */
+static void coroutine_fn tpm_nvram_co_read(void *opaque)
+{
+TPMNvramRWRequest *rwr = opaque;
+
+*rwr->blob_r = g_malloc(rwr->size);
+
+rwr->rc = bdrv_pread(rwr->bdrv,
+ rwr->offset,
+ *rwr->blob_r,
+ rwr->size);
+if (rwr->rc != rwr->size) {
+g_free(*rwr->blob_r);
+*rwr->blob_r = NULL;
+}
+
+qemu_mutex_lock(&rwr->completion_mutex);
+qemu_cond_signal(&rwr->completion);
+qemu_mutex_unlock(&rwr->completion_mutex);
+}
+
+/*
+ * Coroutine that writes a blob to the drive asynchronously
+ */
+static void coroutine_fn tpm_nvram_co_write(void *opaque)
+{
+TPMNvramRWRequest *rwr = opaque;
+
+rwr->rc = bdrv_pwrite(rwr->bdrv,
+  rwr->offset,
+  rwr->blob_w,
+  rwr->size);
+
+qemu_mutex_lock(&rwr->completion_mutex);
+

Re: [Qemu-devel] [PATCH v2 1/3] nvram: Add TPM NVRAM implementation

2013-06-06 Thread Stefan Hajnoczi
On Wed, Jun 05, 2013 at 04:47:59PM -0400, Corey Bryant wrote:
> +/*
> + * Coroutine that reads a blob from the drive asynchronously
> + */
> +static void coroutine_fn tpm_nvram_co_read(void *opaque)
> +{
> +TPMNvramRWRequest *rwr = opaque;
> +
> +*rwr->blob_r = g_malloc(rwr->size);
> +
> +rwr->rc = bdrv_pread(rwr->bdrv,
> + rwr->offset,
> + *rwr->blob_r,
> + rwr->size);
> +if (rwr->rc != rwr->size) {
> +g_free(*rwr->blob_r);
> +*rwr->blob_r = NULL;
> +}
> +
> +qemu_mutex_lock(&rwr->completion_mutex);

Race condition: we must only store rwr->rc while holding
->completion_mutex.  Otherwise the other thread may see ->rc and
g_free(rwr) before we leave this function, causing us to operate on
freed memory.

I suggest storing rc into a local variable first and then assigning to
rwr->rc here.

> +/*
> + * Enter a coroutine to write a blob to the drive
> + */
> +static void tpm_nvram_do_co_write(TPMNvramRWRequest *rwr)
> +{
> +int rc;
> +Coroutine *co;
> +
> +rc = tpm_nvram_adjust_size(rwr->bdrv, rwr->offset, rwr->size);
> +if (rc < 0) {
> +rwr->rc = rc;
> +return;
> +}

Do this inside the tpm_nvram_co_write() coroutine so error return still
signals the condvar.  Right now the other thread may miss completion and
deadlock.



Re: [Qemu-devel] [PATCH v2 1/3] nvram: Add TPM NVRAM implementation

2013-06-06 Thread Corey Bryant



On 06/06/2013 05:22 AM, Stefan Hajnoczi wrote:

On Wed, Jun 05, 2013 at 04:47:59PM -0400, Corey Bryant wrote:

+/*
+ * Coroutine that reads a blob from the drive asynchronously
+ */
+static void coroutine_fn tpm_nvram_co_read(void *opaque)
+{
+TPMNvramRWRequest *rwr = opaque;
+
+*rwr->blob_r = g_malloc(rwr->size);
+
+rwr->rc = bdrv_pread(rwr->bdrv,
+ rwr->offset,
+ *rwr->blob_r,
+ rwr->size);
+if (rwr->rc != rwr->size) {
+g_free(*rwr->blob_r);
+*rwr->blob_r = NULL;
+}
+
+qemu_mutex_lock(&rwr->completion_mutex);


Race condition: we must only store rwr->rc while holding
->completion_mutex.  Otherwise the other thread may see ->rc and
g_free(rwr) before we leave this function, causing us to operate on
freed memory.

I suggest storing rc into a local variable first and then assigning to
rwr->rc here.


+/*
+ * Enter a coroutine to write a blob to the drive
+ */
+static void tpm_nvram_do_co_write(TPMNvramRWRequest *rwr)
+{
+int rc;
+Coroutine *co;
+
+rc = tpm_nvram_adjust_size(rwr->bdrv, rwr->offset, rwr->size);
+if (rc < 0) {
+rwr->rc = rc;
+return;
+}


Do this inside the tpm_nvram_co_write() coroutine so error return still
signals the condvar.  Right now the other thread may miss completion and
deadlock.




Thanks for the review.  Sending a v3 out to the list now.

--
Regards,
Corey Bryant