Re: [Qemu-devel] [PATCH V9 5/5] Add a TPM Passthrough backend driver implementation

[Michael S. Tsirkin]

On Mon, Sep 26, 2011 at 04:12:19PM -0400, Stefan Berger wrote:
> >It's usually a good idea to allow passing the fd through
> >a unix file descriptor or command line. net has some code
> >for that, that can be generalized. Good for security
> >separation. It does not have to be part of this patch though, just
> >thinking aloud.
> >
> I also wanted to have that but would rather put that in another
> patch.

Yes, that's ok.

> Basically the following code from net.c needs to go into a
> separate function:
> 
>         char *endptr = NULL;
> 
>         fd = strtol(param, &endptr, 10);
>         if (*endptr || (fd == 0 && param == endptr)) {
>             return -1;
>         }
> 
> My proposal would be to put it into
> 
> int qemu_parse_fd(const char *)
> 
> into qemu-char.c ?

Sure.

> 
> >>+    if (tpm_passthrough_test_tpmdev(tb->s.tpm_pt->tpm_fd)) {
> >>+        fprintf(stderr,
> >>+                "'%s' is not a TPM device.\n",
> >>+                tb->s.tpm_pt->tpm_dev);
> >>+        goto err_close_tpmdev;
> >Is this a must? Is it common to have more than one
> >tpm device available on a computer? Maybe there's
> >a good default in case only one tpm exists there ...
> >
> Well, passing /dev/tty48 in the place of /dev/tpm0 ends up in a
> disappointment. So I'd rather check what that device is and refuse
> to start if it is found not to be a TPM.

Sorry, I mean can path= be made optional in case there's a single
/dev/tmpXXX? Is it even common to have any tpms except
/dev/tpm0?

> >>+    }
> >>+
> >>+    return tb;
> >>+
> >>+err_close_tpmdev:
> >>+    close(tb->s.tpm_pt->tpm_fd);
> >>+
> >>+err_exit:
> >>+    g_free(tb->id);
> >>+    g_free(tb->model);
> >>+    g_free(tb->parameters);
> >>+    g_free(tb->s.tpm_pt);
> >>+    g_free(tb);
> >>+    return NULL;
> >>+}
> >>+
> >>+static void tpm_passthrough_destroy(TPMBackend *tb)
> >>+{
> >>+    TPMPassthruState *tpm_pt = tb->s.tpm_pt;
> >>+
> >>+    tpm_passthrough_terminate_tpm_thread(tb);
> >>+
> >>+    close(tpm_pt->tpm_fd);
> >>+
> >>+    g_free(tb->id);
> >>+    g_free(tb->model);
> >>+    g_free(tb->parameters);
> >>+    g_free(tb->s.tpm_pt);
> >>+    g_free(tb);
> >>+}
> >>+
> >>+const TPMDriverOps tpm_passthrough_driver = {
> >>+    .id                       = "passthrough",
> >>+    .desc                     = tpm_passthrough_create_desc,
> >>+    .create                   = tpm_passthrough_create,
> >>+    .destroy                  = tpm_passthrough_destroy,
> >>+    .init                     = tpm_passthrough_init,
> >>+    .startup_tpm              = tpm_passthrough_startup_tpm,
> >>+    .realloc_buffer           = tpm_passthrough_realloc_buffer,
> >>+    .reset                    = tpm_passthrough_reset,
> >>+    .had_startup_error        = tpm_passthrough_get_startup_error,
> >>+    .get_tpm_established_flag = tpm_passthrough_get_tpm_established_flag,
> >>+};
> >>Index: qemu-git.pt/qemu-options.hx
> >>===================================================================
> >>--- qemu-git.pt.orig/qemu-options.hx
> >>+++ qemu-git.pt/qemu-options.hx
> >>@@ -1777,6 +1777,7 @@ The general form of a TPM device option
> >>  @item -tpmdev @var{backend} ,address@hidden [,@var{options}]
> >>  @findex -tpmdev
> >>  Backend type must be:
> >>address@hidden
> >>
> >>  The specific backend type will determine the applicable options.
> >>  The @code{-tpmdev} options requires a @code{-device} option.
> >>@@ -1788,6 +1789,29 @@ Use ? to print all available TPM backend
> >>  qemu -tpmdev ?
> >>  @end example
> >>
> >>address@hidden -tpmdev passthrough, address@hidden, address@hidden
> >>+
> >>+Enables access to the host's TPM using the passthrough driver.
> >>+
> >>address@hidden specifies the path to the host's TPM device, i.e., on
> >>+a Linux host this would be @code{/dev/tpm0}.
> >So how about making this the default on linux?
> >Has passthough code any chance to work on non-linux btw?
> >
> For sure not on Win32. For other Unices I don't know.
> >>+
> >>+Note that the passthrough device must not be used by any application on
> >>+the host. Since the host's firmware has already initialized the TPM,
> >>+the firmware (BIOS) executed by QEMU will not be able to initialize the
> >>+TPM again and behave differently than if it could initialize the TPM.
> >For the benefit of users, could this text clarify what happens with
> >e.g. linux and windows guests in this case?
> >
> 
> I'll try to clarify.
> 
> FYI: The pending SeaBIOS patches would typically display a menu if
> they succeed in sending a (standard) initialization sequence to the
> TPM. But in this case the SeaBIOS patches aren't needed yet and if
> used will not succeed in sending that command sequence since the
> BIOS of the host already initialized the device.

OK. What happens then? It would be helpful
to describe the issue in terms of what the user sees.
I think it's ok to assume a patched seabios,
it will get merged by the time this all ships to users :)

> >>+If TPM ownership is released from within a QEMU VM
> >When does this happen?
> >
> tpm_clearown is a command line tool provided by the TrouSerS tss
> implementation in the tpm-tools package that allows you to release
> ownership of the device. If the VM user clears ownership (using the
> TPM's password), the device will become deactivated and disabled.
> >>then this requires
> >What requires?
> >
> >>+rebooting of the host and entering the host's firmware
> >entering?
> >
> Go into the menu of the host's firmware (BIOS/UEFI).

All good answers shall likely go into the documentation :)

> >>to enable and activate
> >>+the TPM again.
> >>address@hidden is required.
> >>+
> >>+To create a passthrough TPM use the following two options:
> >>address@hidden
> >>+-tpmdev pasthrough,id=tpm0,path=<path to TPM device>  -device 
> >>tpm-tis,tpmdev=tpm0
> >>address@hidden example
> >>+Not that the @code{-tpmdev} id is @code{tpm0} and is referenced by
> >Note?
> >
> :-/
> >>address@hidden in the device option.
> >>+
> >>  @end table
> >>
> >>  ETEXI
> Thanks for the review.
> 
>    Stefan