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

[Matthew Fioravante <matthew.fioravante@xxxxxxxxxx>]

On 09/28/2012 12:41 PM, Matthew Fioravante wrote:
> 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.
Actually thinking about it more, uuids have to be attached to the
driver. If 2 vtpms connect to the manager, one could send the uuid of
the other and get access to someone elses secrets.

TL;DR version

uuids must remain part of the driver.
>>>
>>>>  -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