[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH] stubdom/vtpm: make state save operation atomic
On 11/29/2012 03:24 PM, Daniel De Graaf wrote:
On 11/29/2012 01:07 PM, Matthew Fioravante wrote:
On 11/27/2012 05:16 PM, Daniel De Graaf wrote:
This changes the save format of the vtpm stubdom to include two copies
of the saved data: one active, and one inactive. When saving the state,
data is written to the inactive slot before updating the key and hash
saved with the TPM Manager, which determines the active slot when the
vTPM starts up.
Signed-off-by: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
---
stubdom/vtpm/vtpmblk.c | 66
+++++++++++++++++++++++++++++++++++++++++---------
1 file changed, 54 insertions(+), 12 deletions(-)
diff --git a/stubdom/vtpm/vtpmblk.c b/stubdom/vtpm/vtpmblk.c
index b343bd8..f988606 100644
--- a/stubdom/vtpm/vtpmblk.c
+++ b/stubdom/vtpm/vtpmblk.c
@@ -23,6 +23,8 @@
/*Encryption key and block sizes */
#define BLKSZ 16
+/* Maximum size of one saved-state slot */
+#define SLOT_SIZE 32768
Instead of statically defining a slot size and stressing over whether or not it
may be too small, why not make it half the size of the disk image? You can
retrieve the disk size by doing fstat(blkfront_fd).
Ah, I didn't see that information was available in mini-os. Actually, the
blkinfo
structure in init_vtpmblk is more convenient, so I'll use that.
static struct blkfront_dev* blkdev = NULL;
static int blkfront_fd = -1;
@@ -59,15 +61,20 @@ void shutdown_vtpmblk(void)
blkdev = NULL;
}
-int write_vtpmblk_raw(uint8_t *data, size_t data_length)
+static int write_vtpmblk_raw(uint8_t *data, size_t data_length, int slot)
{
int rc;
uint32_t lenbuf;
debug("Begin Write data=%p len=%u", data, data_length);
Should add which slot were writing to and possibly the disk offset as well to
the debug print
A number of debug outputs will be improved with slot info.
+ if (data_length > SLOT_SIZE - 4) {
+ error("write(data) cannot fit in data slot (%d). Increase SLOT_SIZE.",
data_length);
+ return -1;
+ }
+
lenbuf = cpu_to_be32((uint32_t)data_length);
- lseek(blkfront_fd, 0, SEEK_SET);
+ lseek(blkfront_fd, slot * SLOT_SIZE, SEEK_SET);
if((rc = write(blkfront_fd, (uint8_t*)&lenbuf, 4)) != 4) {
error("write(length) failed! error was %s", strerror(errno));
return -1;
@@ -82,12 +89,12 @@ int write_vtpmblk_raw(uint8_t *data, size_t data_length)
return 0;
}
-int read_vtpmblk_raw(uint8_t **data, size_t *data_length)
+static int read_vtpmblk_raw(uint8_t **data, size_t *data_length, int slot)
{
int rc;
uint32_t lenbuf;
- lseek(blkfront_fd, 0, SEEK_SET);
+ lseek(blkfront_fd, slot * SLOT_SIZE, SEEK_SET);
if(( rc = read(blkfront_fd, (uint8_t*)&lenbuf, 4)) != 4) {
error("read(length) failed! error was %s", strerror(errno));
return -1;
@@ -97,6 +104,10 @@ int read_vtpmblk_raw(uint8_t **data, size_t *data_length)
error("read 0 data_length for NVM");
return -1;
}
+ if(*data_length > SLOT_SIZE - 4) {
+ error("read invalid data_length for NVM");
+ return -1;
+ }
*data = tpm_malloc(*data_length);
if((rc = read(blkfront_fd, *data, *data_length)) != *data_length) {
@@ -221,6 +232,8 @@ egress:
return rc;
}
+static int active_slot = -1;
I think we should initialize to 0 or 1 because -1 is an invalid state.
For a new vtpm, read_vtpmblk() will fail and I think this is what happens:
read_vtpmblk()
try_fetch_key_from_manager() -> fails
goto abort_egress
active_slot = -1 (you have this below)
Yes, that's intentional. That way, you can tell the difference between slot 1
being active and no slot being active.
Other than possibly for debugging, do we really care if no slot is
active? If there is no logical active slot then either slot 0 or 1 is
valid for the next write.
Then later
write_vtpmblk()
active_slot = !active_slot (!-1 = 0)
The last line happens to work correctly but I don't think its very clear. I
think we should initialize to 1 and set it to 1 if read_vtpmblk() fails.
I think I'll just put a comment at the !active_slot to note that active_slot is
-1 => 0 if no active slots existed.
If we keep the -1, then defiantly a comment is warranted. There should
be a block comment at active_slot's definition to briefly explain all of
this, what the valid states of active_slot are (-1, 0, 1), and what they
mean because at first glance its not intuitive and looks like it could
be a bug. Another self documenting option would be to use an enum or
#define INVALID_SLOT -1 if you don't want to write as much comment text.
Also a minor nitpick, the name is active_slot, but what it really is is the
last_slot_written_to because everytime you write you change it first.
That still makes it the active slot: at the point when it's changed, the value
is
the new active slot (and we're just working to commit that change to disk).
+
int write_vtpmblk(struct tpmfront_dev* tpmfront_dev, uint8_t* data, size_t
data_length) {
int rc;
uint8_t* cipher = NULL;
@@ -228,12 +241,15 @@ int write_vtpmblk(struct tpmfront_dev* tpmfront_dev,
uint8_t* data, size_t data_
uint8_t hashkey[HASHKEYSZ];
uint8_t* symkey = hashkey + HASHSZ;
+ /* Switch to the other slot */
+ active_slot = !active_slot;
+
/* Encrypt the data */
if((rc = encrypt_vtpmblk(data, data_length, &cipher, &cipher_len,
symkey))) {
goto abort_egress;
}
/* Write to disk */
- if((rc = write_vtpmblk_raw(cipher, cipher_len))) {
+ if((rc = write_vtpmblk_raw(cipher, cipher_len, active_slot))) {
goto abort_egress;
}
/* Get sha1 hash of data */
@@ -256,7 +272,8 @@ int read_vtpmblk(struct tpmfront_dev* tpmfront_dev,
uint8_t** data, size_t *data
size_t cipher_len = 0;
size_t keysize;
uint8_t* hashkey = NULL;
- uint8_t hash[HASHSZ];
+ uint8_t hash0[HASHSZ];
+ uint8_t hash1[HASHSZ];
uint8_t* symkey;
/* Retreive the hash and the key from the manager */
@@ -270,14 +287,32 @@ int read_vtpmblk(struct tpmfront_dev* tpmfront_dev,
uint8_t** data, size_t *data
}
symkey = hashkey + HASHSZ;
- /* Read from disk now */
- if((rc = read_vtpmblk_raw(&cipher, &cipher_len))) {
+ active_slot = 0;
+ /* Read slot 0 from disk now */
+ if((rc = read_vtpmblk_raw(&cipher, &cipher_len, 0))) {
+ goto abort_egress;
+ }
+
+ /* Compute the hash of the cipher text and compare */
+ sha1(cipher, cipher_len, hash0);
+ if(!memcmp(hash0, hashkey, HASHSZ))
+ goto valid;
+
+ free(cipher);
+ cipher = NULL;
+
+ active_slot = 1;
+ /* Read slot 1 from disk now */
+ if((rc = read_vtpmblk_raw(&cipher, &cipher_len, 1))) {
goto abort_egress;
}
/* Compute the hash of the cipher text and compare */
- sha1(cipher, cipher_len, hash);
- if(memcmp(hash, hashkey, HASHSZ)) {
+ sha1(cipher, cipher_len, hash1);
+ if(!memcmp(hash1, hashkey, HASHSZ))
+ goto valid;
+
+ {
int i;
error("NVM Storage Checksum failed!");
printf("Expected: ");
@@ -285,14 +320,20 @@ int read_vtpmblk(struct tpmfront_dev* tpmfront_dev,
uint8_t** data, size_t *data
printf("%02hhX ", hashkey[i]);
}
printf("\n");
- printf("Actual: ");
+ printf("Slot 0: ");
+ for(i = 0; i < HASHSZ; ++i) {
+ printf("%02hhX ", hash0[i]);
+ }
+ printf("\n");
+ printf("Slot 1: ");
for(i = 0; i < HASHSZ; ++i) {
- printf("%02hhX ", hash[i]);
+ printf("%02hhX ", hash1[i]);
}
printf("\n");
rc = -1;
goto abort_egress;
}
+valid:
/* Decrypt the blob */
if((rc = decrypt_vtpmblk(cipher, cipher_len, data, data_length, symkey)))
{
@@ -300,6 +341,7 @@ int read_vtpmblk(struct tpmfront_dev* tpmfront_dev,
uint8_t** data, size_t *data
}
goto egress;
abort_egress:
+ active_slot = -1;
See my comment above
egress:
free(cipher);
free(hashkey);
Attachment:
smime.p7s
Description: S/MIME Cryptographic Signature
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
