|
[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 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.
> 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.
> 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);
>
>
--
Daniel De Graaf
National Security Agency
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |