|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 11/11] add vtpm support to libxl
On 09/28/2012 11:03 AM, George Dunlap wrote:
> On Thu, Sep 27, 2012 at 6:11 PM, Matthew Fioravante
> <matthew.fioravante@xxxxxxxxxx> 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 <matthew.fioravante@xxxxxxxxxx>
> 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 *aodevs,
>> 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 *aodevs,
>> 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 = domcreate_attach_pci;
>> + dcs->multidev.callback = 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 follow.
>
>> 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 = CONTAINER_OF(multidev, *dcs, multidev);
>> + STATE_AO_GC(dcs->ao);
>> + int domid = dcs->guest_domid;
>> +
>> + libxl_domain_config* const d_config = 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 = 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 *multidev,
>> int ret)
>> {
>> @@ -1111,7 +1147,7 @@ static void domcreate_attach_pci(libxl__egc *egc,
>> libxl__multidev *multidev,
>> libxl_domain_config *const d_config = 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
>> *config_source,
>> }
>> }
>>
>> + if (!xlu_cfg_get_list(config, "vtpm", &vtpms, 0, 0)) {
>> + d_config->num_vtpms = 0;
>> + d_config->vtpms = NULL;
>> + while ((buf = xlu_cfg_get_listitem (vtpms, d_config->num_vtpms)) !=
>> NULL) {
>> + libxl_device_vtpm *vtpm;
>> + char * buf2 = strdup(buf);
>> + char *p, *p2;
>> +
>> + d_config->vtpms = (libxl_device_vtpm *)
>> realloc(d_config->vtpms, sizeof(libxl_device_vtpm) *
>> (d_config->num_vtpms+1));
>> + vtpm = d_config->vtpms + d_config->num_vtpms;
>> + libxl_device_vtpm_init(vtpm);
>> + vtpm->devid = d_config->num_vtpms;
>> +
>> + p = 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=dom0. Considering that were getting rid of the process model, it
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 == ' ')
>> + ++p;
>> + if ((p2 = strchr(p, '=')) == NULL)
>> + break;
>> + *p2 = '\0';
>> + if (!strcmp(p, "backend")) {
>> + if(domain_qualifier_to_domid(p2 + 1,
>> &(vtpm->backend_domid), 0))
>> + {
>> + fprintf(stderr, "Specified backend domain does not
>> exist, defaulting to Dom0\n");
>> + vtpm->backend_domid = 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=[whatever]", it will act like uuid isn't there and
> 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
Attachment:
smime.p7s _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |