From mboxrd@z Thu Jan 1 00:00:00 1970 From: Matthew Fioravante Subject: Re: [PATCH 11/11] add vtpm support to libxl Date: Fri, 28 Sep 2012 12:06:01 -0400 Message-ID: <5065CAE9.30602@jhuapl.edu> References: <1348765860-11359-1-git-send-email-matthew.fioravante@jhuapl.edu> <1348765860-11359-3-git-send-email-matthew.fioravante@jhuapl.edu> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============3695380679684712031==" Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: George Dunlap Cc: "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. --===============3695380679684712031== Content-Type: multipart/signed; protocol="application/pkcs7-signature"; micalg=sha1; boundary="------------ms020402040708000707080800" This is a cryptographically signed message in MIME format. --------------ms020402040708000707080800 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable On 09/28/2012 11:03 AM, George Dunlap wrote: > On Thu, Sep 27, 2012 at 6:11 PM, Matthew Fioravante > wrote: >> This patch adds vtpm support to libxl. It adds vtpm parsing to config >> files and 3 new xl commands: >> vtpm-attach >> vtpm-detach >> vtpm-list >> >> Signed-off-by: Matthew Fioravante > Overall looks good to me -- just a few comments below about the config > file handling (see below). > > Thanks for all your work on this. > >> @@ -601,6 +605,8 @@ static void domcreate_bootloader_done(libxl__egc *= egc, >> static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *aod= evs, >> int ret); >> >> +static void domcreate_attach_vtpms(libxl__egc *egc, libxl__multidev *= multidev, >> + int ret); >> static void domcreate_attach_pci(libxl__egc *egc, libxl__multidev *ao= devs, >> int ret); >> >> @@ -1084,13 +1090,13 @@ static void domcreate_devmodel_started(libxl__= egc *egc, >> if (d_config->num_nics > 0) { >> /* Attach nics */ >> libxl__multidev_begin(ao, &dcs->multidev); >> - dcs->multidev.callback =3D domcreate_attach_pci; >> + dcs->multidev.callback =3D domcreate_attach_vtpms; > Wow -- what a weird convention you've had to try to figure out and > modify here. Well done. :-) It was really tricky. Is there no better way to handle asynchronous code? This method seems really error prone and almost impossible to follo= w. > >> libxl__add_nics(egc, ao, domid, d_config, &dcs->multidev); >> libxl__multidev_prepared(egc, &dcs->multidev, 0); >> return; >> } >> >> - domcreate_attach_pci(egc, &dcs->multidev, 0); >> + domcreate_attach_vtpms(egc, &dcs->multidev, 0); >> return; >> >> error_out: >> @@ -1098,6 +1104,36 @@ error_out: >> domcreate_complete(egc, dcs, ret); >> } >> >> +static void domcreate_attach_vtpms(libxl__egc *egc, libxl__multidev *= multidev, int ret) { >> + libxl__domain_create_state *dcs =3D CONTAINER_OF(multidev, *dcs, m= ultidev); >> + STATE_AO_GC(dcs->ao); >> + int domid =3D dcs->guest_domid; >> + >> + libxl_domain_config* const d_config =3D dcs->guest_config; >> + >> + if(ret) { >> + LOG(ERROR, "unable to add nic devices"); >> + goto error_out; >> + } >> + >> + /* Plug vtpm devices */ >> + if (d_config->num_vtpms > 0) { >> + /* Attach vtpms */ >> + libxl__multidev_begin(ao, &dcs->multidev); >> + dcs->multidev.callback =3D domcreate_attach_pci; >> + libxl__add_vtpms(egc, ao, domid, d_config, &dcs->multidev); >> + libxl__multidev_prepared(egc, &dcs->multidev, 0); >> + return; >> + } >> + >> + domcreate_attach_pci(egc, multidev, 0); >> + return; >> + >> +error_out: >> + assert(ret); >> + domcreate_complete(egc, dcs, ret); >> +} >> + >> static void domcreate_attach_pci(libxl__egc *egc, libxl__multidev *mu= ltidev, >> int ret) >> { >> @@ -1111,7 +1147,7 @@ static void domcreate_attach_pci(libxl__egc *egc= , libxl__multidev *multidev, >> libxl_domain_config *const d_config =3D dcs->guest_config; >> >> if (ret) { >> - LOG(ERROR, "unable to add nic devices"); >> + LOG(ERROR, "unable to add vtpm devices"); >> goto error_out; >> } >> > [snip] >> @@ -1045,6 +1045,47 @@ static void parse_config_data(const char *confi= g_source, >> } >> } >> >> + if (!xlu_cfg_get_list(config, "vtpm", &vtpms, 0, 0)) { >> + d_config->num_vtpms =3D 0; >> + d_config->vtpms =3D NULL; >> + while ((buf =3D xlu_cfg_get_listitem (vtpms, d_config->num_vt= pms)) !=3D NULL) { >> + libxl_device_vtpm *vtpm; >> + char * buf2 =3D strdup(buf); >> + char *p, *p2; >> + >> + d_config->vtpms =3D (libxl_device_vtpm *) realloc(d_confi= g->vtpms, sizeof(libxl_device_vtpm) * (d_config->num_vtpms+1)); >> + vtpm =3D d_config->vtpms + d_config->num_vtpms; >> + libxl_device_vtpm_init(vtpm); >> + vtpm->devid =3D d_config->num_vtpms; >> + >> + p =3D strtok(buf2, ","); >> + if (!p) >> + goto skip_vtpm; > Is the purpose of this so that you can have "empty" vtpm slots? > (Since even when skipping, you still increment the num_vtpms counter?) That would make a default vtpm with a randomly generated uuid and backend=3Ddom0. Considering that were getting rid of the process model, i= t probably makes sense to force the user to specify a backend domain id because no vtpm device will ever connect to dom0 anymore. See comments about uuid below. > >> + do { >> + while(*p =3D=3D ' ') >> + ++p; >> + if ((p2 =3D strchr(p, '=3D')) =3D=3D NULL) >> + break; >> + *p2 =3D '\0'; >> + if (!strcmp(p, "backend")) { >> + if(domain_qualifier_to_domid(p2 + 1, &(vtpm->back= end_domid), 0)) >> + { >> + fprintf(stderr, "Specified backend domain doe= s not exist, defaulting to Dom0\n"); >> + vtpm->backend_domid =3D 0; > So wait; if someone specifies a domain, and that turns out not to > work, you just change it to 0? It seems like if the *specified* > domain doesn't exist, the command should fail instead of choosing > something at random. I agree. > >> + } >> + } else if(!strcmp(p, "uuid")) { >> + if( libxl_uuid_from_string(&vtpm->uuid, p2 + 1) )= { >> + fprintf(stderr, "Failed to parse vtpm UUID: %= s\n", p2 + 1); >> + exit(1); >> + } >> + } > If I'm parsing this right, it looks like you will just silently ignore > other arguments -- it seems like throwing an error would be better; > especially to catch things like typos. Otherwise, if someone does > something like "uid=3D[whatever]", it will act like uuid isn't there an= d > create a new one, instead of alerting the user to the fact that he'd > made a typo in the config file. The behavior here is there if the user passes an invalid uuid string it will fail with a parse error, but if the user does not specify a uuid at all, one will be randomly generated. Random generation happens in the vtpm types constructor in the xl type system. This brings up a bigger wart in the vtpm implementation. Its kind of confusing now because the linux guest uses a tpmfront/back pair to talk to the vtpm, and then vtpm uses another tpmfront/back pair to talk to the manager. You have to specify the uuid on the vtpm's tpmfront device because that is the device the manager sees. You do not have to specify one on the linux domains device. I'd argue that now, especially with the process model gone, the uuid should be a parameter of the vtpm itself and not the tpmfront/back communication channels. The problem is that this uuid needs to specified by the "control domain" or dom0. By attaching the uuid to the device, the manager can trust the uuid attached to whoever is trying to connect to him. One idea is to make the uuid a commandline parameter for the mini-os domain and have the vtpm pass the id down to the manager. That means however that any rogue domain could potentially connect to the manager and send him someone elses uuid, and thus being able to access the vtpms stored secrets. However one could argue that no domain would be able to connect to the manager to do this anyway because they would have to create a tpmfront/back device pair and the only way to do that is to break into the "control domain." If one can do this, then one could just as easily set their device uuid to whatever they want. I hope all that made sense. Do you see any flaws in my reasoning? If so I should probably get uuids out of the vtpm devices and simplify this whole thing. > > -George --------------ms020402040708000707080800 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 KoZIhvcNAQkFMQ8XDTEyMDkyODE2MDYwMVowIwYJKoZIhvcNAQkEMRYEFNIfLvKkGMOnk7A6 R71FJwslJK9VMGwGCSqGSIb3DQEJDzFfMF0wCwYJYIZIAWUDBAEqMAsGCWCGSAFlAwQBAjAK BggqhkiG9w0DBzAOBggqhkiG9w0DAgICAIAwDQYIKoZIhvcNAwICAUAwBwYFKw4DAgcwDQYI KoZIhvcNAwICASgwDQYJKoZIhvcNAQEBBQAEgYALhtVVjz0Wn0Pd1hnlS7/uSh86DWsdP40B I1HsTsIVinrgJtB5FAsuFB16y1pLf0mKTIKMwWm/USdVN47bxXGwZlxSaFEpggB3BzdyBs2y xrlz5EhnRo/vQ7PyRosEYOgKZ1v5Xwntz2MameDUcWls/RU4iLZGkksnG9bx+dWwQAAAAAAA AA== --------------ms020402040708000707080800-- --===============3695380679684712031== 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 --===============3695380679684712031==--