From mboxrd@z Thu Jan 1 00:00:00 1970 From: Matthew Fioravante Subject: Re: [PATCH] stubdom/vtpm: make state save operation atomic Date: Thu, 29 Nov 2012 13:07:25 -0500 Message-ID: <50B7A45D.1050607@jhuapl.edu> References: <50B52524.6030707@tycho.nsa.gov> <1354054610-10720-1-git-send-email-dgdegra@tycho.nsa.gov> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1863171024055754067==" Return-path: In-Reply-To: <1354054610-10720-1-git-send-email-dgdegra@tycho.nsa.gov> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Daniel De Graaf Cc: "samuel.thibault@ens-lyon.org" , "Ian.Campbell@citrix.com" , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org This is a cryptographically signed message in MIME format. --===============1863171024055754067== Content-Type: multipart/signed; protocol="application/pkcs7-signature"; micalg=sha1; boundary="------------ms000306090006050103030206" This is a cryptographically signed message in MIME format. --------------ms000306090006050103030206 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable 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 > --- > 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 @@ > =20 > /*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=20 image? You can retrieve the disk size by doing fstat(blkfront_fd). > =20 > static struct blkfront_dev* blkdev =3D NULL; > static int blkfront_fd =3D -1; > @@ -59,15 +61,20 @@ void shutdown_vtpmblk(void) > blkdev =3D NULL; > } > =20 > -int write_vtpmblk_raw(uint8_t *data, size_t data_length) > +static int write_vtpmblk_raw(uint8_t *data, size_t data_length, int sl= ot) > { > int rc; > uint32_t lenbuf; > debug("Begin Write data=3D%p len=3D%u", data, data_length); Should add which slot were writing to and possibly the disk offset as=20 well to the debug print > =20 > + if (data_length > SLOT_SIZE - 4) { > + error("write(data) cannot fit in data slot (%d). Increase SLOT_S= IZE.", data_length); > + return -1; > + } > + > lenbuf =3D cpu_to_be32((uint32_t)data_length); > =20 > - lseek(blkfront_fd, 0, SEEK_SET); > + lseek(blkfront_fd, slot * SLOT_SIZE, SEEK_SET); > if((rc =3D write(blkfront_fd, (uint8_t*)&lenbuf, 4)) !=3D 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_le= ngth) > return 0; > } > =20 > -int read_vtpmblk_raw(uint8_t **data, size_t *data_length) > +static int read_vtpmblk_raw(uint8_t **data, size_t *data_length, int s= lot) > { > int rc; > uint32_t lenbuf; > =20 > - lseek(blkfront_fd, 0, SEEK_SET); > + lseek(blkfront_fd, slot * SLOT_SIZE, SEEK_SET); > if(( rc =3D read(blkfront_fd, (uint8_t*)&lenbuf, 4)) !=3D 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_l= ength) > error("read 0 data_length for NVM"); > return -1; > } > + if(*data_length > SLOT_SIZE - 4) { > + error("read invalid data_length for NVM"); > + return -1; > + } > =20 > *data =3D tpm_malloc(*data_length); > if((rc =3D read(blkfront_fd, *data, *data_length)) !=3D *data_leng= th) { > @@ -221,6 +232,8 @@ egress: > return rc; > } > =20 > +static int active_slot =3D -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 =3D -1 (you have this below) Then later write_vtpmblk() active_slot =3D !active_slot (!-1 =3D 0) The last line happens to work correctly but I don't think its very=20 clear. I think we should initialize to 1 and set it to 1 if=20 read_vtpmblk() fails. Also a minor nitpick, the name is active_slot, but what it really is is=20 the last_slot_written_to because everytime you write you change it first.= > + > int write_vtpmblk(struct tpmfront_dev* tpmfront_dev, uint8_t* data, s= ize_t data_length) { > int rc; > uint8_t* cipher =3D NULL; > @@ -228,12 +241,15 @@ int write_vtpmblk(struct tpmfront_dev* tpmfront_d= ev, uint8_t* data, size_t data_ > uint8_t hashkey[HASHKEYSZ]; > uint8_t* symkey =3D hashkey + HASHSZ; > =20 > + /* Switch to the other slot */ > + active_slot =3D !active_slot; > + > /* Encrypt the data */ > if((rc =3D encrypt_vtpmblk(data, data_length, &cipher, &cipher_len= , symkey))) { > goto abort_egress; > } > /* Write to disk */ > - if((rc =3D write_vtpmblk_raw(cipher, cipher_len))) { > + if((rc =3D 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 =3D 0; > size_t keysize; > uint8_t* hashkey =3D NULL; > - uint8_t hash[HASHSZ]; > + uint8_t hash0[HASHSZ]; > + uint8_t hash1[HASHSZ]; > uint8_t* symkey; > =20 > /* Retreive the hash and the key from the manager */ > @@ -270,14 +287,32 @@ int read_vtpmblk(struct tpmfront_dev* tpmfront_de= v, uint8_t** data, size_t *data > } > symkey =3D hashkey + HASHSZ; > =20 > - /* Read from disk now */ > - if((rc =3D read_vtpmblk_raw(&cipher, &cipher_len))) { > + active_slot =3D 0; > + /* Read slot 0 from disk now */ > + if((rc =3D 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 =3D NULL; > + > + active_slot =3D 1; > + /* Read slot 1 from disk now */ > + if((rc =3D read_vtpmblk_raw(&cipher, &cipher_len, 1))) { > goto abort_egress; > } > =20 > /* 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_de= v, uint8_t** data, size_t *data > printf("%02hhX ", hashkey[i]); > } > printf("\n"); > - printf("Actual: "); > + printf("Slot 0: "); > + for(i =3D 0; i < HASHSZ; ++i) { > + printf("%02hhX ", hash0[i]); > + } > + printf("\n"); > + printf("Slot 1: "); > for(i =3D 0; i < HASHSZ; ++i) { > - printf("%02hhX ", hash[i]); > + printf("%02hhX ", hash1[i]); > } > printf("\n"); > rc =3D -1; > goto abort_egress; > } > +valid: > =20 > /* Decrypt the blob */ > if((rc =3D 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 =3D -1; See my comment above > egress: > free(cipher); > free(hashkey); --------------ms000306090006050103030206 Content-Type: application/pkcs7-signature; name="smime.p7s" Content-Transfer-Encoding: base64 Content-Disposition: attachment; filename="smime.p7s" Content-Description: S/MIME Cryptographic Signature MIAGCSqGSIb3DQEHAqCAMIACAQExCzAJBgUrDgMCGgUAMIAGCSqGSIb3DQEHAQAAoIIDyjCC A8YwggMvoAMCAQICBD/xyf0wDQYJKoZIhvcNAQEFBQAwLzELMAkGA1UEBhMCVVMxDzANBgNV BAoTBkpIVUFQTDEPMA0GA1UECxMGQklTRENBMB4XDTEwMDYxMTE4MjIwNloXDTEzMDYxMTE4 NTIwNlowZjELMAkGA1UEBhMCVVMxDzANBgNVBAoTBkpIVUFQTDEPMA0GA1UECxMGUGVvcGxl MTUwFgYDVQQLEw9WUE5Hcm91cC1CSVNEQ0EwGwYDVQQDExRNYXR0aGV3IEUgRmlvcmF2YW50 ZTCBnzANBgkqhkiG9w0BAQEFAAOBjQAwgYkCgYEAnpbwVSP6o1Nb5lcW7dd3yTo9iBJdi7qz 4nANOMFPK7JOy5npKN1iiousl28U/scUJES55gPwAWYJK3uVyQAsA4adgDKi5DoD1UHDQEwp bY7iHLJeq0NPr4BqYNqnCFPbE6HC8zSJrr4qKn+gVUQT39SIFqdiIPJwZL8FYTRQ/zsCAwEA AaOCAbYwggGyMAsGA1UdDwQEAwIHgDArBgNVHRAEJDAigA8yMDEwMDYxMTE4MjIwNlqBDzIw MTIwNzE3MjI1MjA2WjAbBg0rBgEEAbMlCwMBAQEBBAoWCGZpb3JhbWUxMBsGDSsGAQQBsyUL AwEBAQIEChIIMDAxMDQyNjEwWAYJYIZIAYb6ax4BBEsMSVRoZSBwcml2YXRlIGtleSBjb3Jy ZXNwb25kaW5nIHRvIHRoaXMgY2VydGlmaWNhdGUgbWF5IGhhdmUgYmVlbiBleHBvcnRlZC4w KAYDVR0RBCEwH4EdTWF0dGhldy5GaW9yYXZhbnRlQGpodWFwbC5lZHUwUgYDVR0fBEswSTBH oEWgQ6RBMD8xCzAJBgNVBAYTAlVTMQ8wDQYDVQQKEwZKSFVBUEwxDzANBgNVBAsTBkJJU0RD QTEOMAwGA1UEAxMFQ1JMNTYwHwYDVR0jBBgwFoAUCDUpmxH52EU2CyWmF2EJMB1yqeswHQYD VR0OBBYEFO6LYxg6r9wHZ+zdQtBHn1dZ/YTNMAkGA1UdEwQCMAAwGQYJKoZIhvZ9B0EABAww ChsEVjcuMQMCBLAwDQYJKoZIhvcNAQEFBQADgYEAJO9HQh4YNChVLzuZqK5ARJARD8JoujGZ fdo75quvg2jXFQe2sEjvLnxJZgm/pv8fdZakq48CWwjYHKuvIp7sDjTEsQfo+y7SpN/N2NvJ WU5SqfK1VgYtNLRRoGJUB5Q1aZ+Dg95g3kqpyfpUMISJL8IKVLtJVfN4fggFVUYZ9wwxggGr MIIBpwIBATA3MC8xCzAJBgNVBAYTAlVTMQ8wDQYDVQQKEwZKSFVBUEwxDzANBgNVBAsTBkJJ U0RDQQIEP/HJ/TAJBgUrDgMCGgUAoIHLMBgGCSqGSIb3DQEJAzELBgkqhkiG9w0BBwEwHAYJ KoZIhvcNAQkFMQ8XDTEyMTEyOTE4MDcyNVowIwYJKoZIhvcNAQkEMRYEFHwvosWgCwnRopDG u3htj0yCYz+NMGwGCSqGSIb3DQEJDzFfMF0wCwYJYIZIAWUDBAEqMAsGCWCGSAFlAwQBAjAK BggqhkiG9w0DBzAOBggqhkiG9w0DAgICAIAwDQYIKoZIhvcNAwICAUAwBwYFKw4DAgcwDQYI KoZIhvcNAwICASgwDQYJKoZIhvcNAQEBBQAEgYAH3ImdgCdCtPGu2f78YFo+JSXAmMRzflb5 Ly0a2tqLvvQk5nvSx7C7RDCgMmJBlKTrUGrnYkgOs40Y56z70k0lW3wjWU4qGFQ3BSxosPq+ sdGRNADBnTxwg3WUM5aybJ4IgYV9A3eu4ZiAS2lrUb+y5QFXAzMt3NeLmhxMm17SfQAAAAAA AA== --------------ms000306090006050103030206-- --===============1863171024055754067== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============1863171024055754067==--