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

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

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?

> 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. :-)

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
>

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