Re: [Xen-devel] [PATCH 11/11] add vtpm support to libxl

[Matthew Fioravante <matthew.fioravante@xxxxxxxxxx>]

On 09/28/2012 12:39 PM, George Dunlap wrote:
> On Fri, Sep 28, 2012 at 5:06 PM, Matthew Fioravante
> <matthew.fioravante@xxxxxxxxxx> wrote:
>> 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.
> Well I didn't write it. :-)  I haven't taken the time to figure out
> why it might have been written that way; but at first glance, I tend
> to agree with you.  For about 10 minutes I was convinced you had made
> some kind of weird error, by sprinkling "vtpm" around things that
> obviously were supposed to be about nics and pci devices, until I
> realized you were just following the existing "call chain" convention.
>
>>>> +            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.
> Ah, right.  Either way is OK with me, but a comment would be useful. :-)
>
>>>> +                    }
>>>> +                } 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.
> I think you misunderstood my comment; I'm not actually talking about
> the uuid clause that's there, but the "none of the above" clause
> that's missing.  The code says (in pseudocode):
>
> if("backend")
>  parse backend;
> else if("uuid")
>  parse uuid;
>
> But what if it's neither "backend" or "uuid", but something else --
> say, "uid" or "backedn"?  Then instead of giving an error, it will
> just skip that argument and go on to the next one; and if the user
> *intended* to type "backend" instead of "backedn", it will silently
> use the default, giving her no clue as to what the problem might be.
> I'm proposing adding (again in pseudocode):
>
> else
>   error("Unrecognized argument: %s\n", p);
>
> Does that make sense?
>
Yeah, didn't see that. Good catch.
>> This brings up a bigger wart in the vtpm implementation.
> It's 5:30pm on a Friday, so I'm going to put off grokking the rest of
> this until Monday morning. :-)
Agreed, enjoy your weekend :)
> Have a good weekend,
>  -George
>
>> 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
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@xxxxxxxxxxxxx
>> http://lists.xen.org/xen-devel
>>

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel