Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

vTPM state storage v0 #4730

Merged
merged 8 commits into from
Jun 17, 2022
Merged

vTPM state storage v0 #4730

merged 8 commits into from
Jun 17, 2022

Conversation

edwintorok
Copy link
Contributor

@edwintorok edwintorok commented Jun 8, 2022 •
edited
Loading

See the description in the commit messages.

@psafont
Copy link
Member

psafont commented Jun 9, 2022

you probably want to cherry-pick 813f8dc (#4720) to fix the build

psafont
psafont reviewed Jun 9, 2022
View reviewed changes
ocaml/xapi/xapi_xenops.ml Outdated Show resolved Hide resolved
@edwintorok edwintorok force-pushed the private/edvint/vtpm-state-merged branch from 91fd976 to 62c491c Compare June 9, 2022 13:03
psafont
psafont reviewed Jun 9, 2022
View reviewed changes
ocaml/xapi-idl/lib/uuidm_rpc_type.ml
@@ -0,0 +1,33 @@
module Uuidm = struct
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend changing the number in the quality gate, maintaining the corresponding mli seems to much of a headache.

psafont
psafont reviewed Jun 9, 2022
View reviewed changes
ocaml/xenopsd/xc/device.ml
Varstore_privileged_client.Client.vtpm_get_contents dbg vtpm_uuid
|> Base64.decode_exn
in
let chroot = Xenops_sandbox.Swtpm_guard.chroot ~domid ~vm_uuid in
Copy link
Member

@psafont psafont Jun 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This happens after creating the sandbox, it looks like the chroot object should be a parameter to this function (called in line 4236) instead of exposing the function to create chroot objects without having to create them in the filesystem.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we probably want a way to check whether a file exists in the sandbox without necessarily creating the sandbox (which would create the file if it doesn't exist). I'll see whether I can plumb the Chroot.t parameter through.

@edwintorok
Copy link
Contributor Author

Note that this assumes that the backend is always xapi-db. Once we have a 'backend' field again then this should be made more generic, to allow any backend (and to move the backend specific code into a module of its own with a clear signature, so it'll be obvious how to implement new backends).

@psafont
Copy link
Member

psafont commented Jun 9, 2022 •
edited
Loading

If I read the code correctly, writing the state filepath into a xenstore key containing the vtpm_uuid is done to support multiple vtpms per VM, but is currently unused.

Is there a reason for using this mechanism instead of, let's say symlinks in the chroot to stablish this relationship off-band?

@edwintorok
Copy link
Contributor Author

Looks like it is used after all: in vtpms_of_domid we read the vtpm uuids from xenstore, this makes the stop code generic and works whether or not the VM has a vTPM attached or not (i.e. for the 0 vs 1 usecase rather than 1 vs 1+ vTPM usecase). xenopsd doesn't have access to XAPI's DB so it needs to retrieve that information from somewhere, however I think it should be set/retrieved in the VM's "xenopsd" configuration, and not xenstore directly. (there is a "DB" module in xenops_server that knows about other devices similarly, and doesn't know about TPMs yet).

Using xenstore here was just a shortcut: this needs a proper VTPM_DB module in lib/xenops_server instead. Thanks for spotting this.

1 similar comment
@edwintorok
Copy link
Contributor Author

Looks like it is used after all: in vtpms_of_domid we read the vtpm uuids from xenstore, this makes the stop code generic and works whether or not the VM has a vTPM attached or not (i.e. for the 0 vs 1 usecase rather than 1 vs 1+ vTPM usecase). xenopsd doesn't have access to XAPI's DB so it needs to retrieve that information from somewhere, however I think it should be set/retrieved in the VM's "xenopsd" configuration, and not xenstore directly. (there is a "DB" module in xenops_server that knows about other devices similarly, and doesn't know about TPMs yet).

Using xenstore here was just a shortcut: this needs a proper VTPM_DB module in lib/xenops_server instead. Thanks for spotting this.

@edwintorok edwintorok force-pushed the private/edvint/vtpm-state-merged branch from ce55c02 to 1898475 Compare June 15, 2022 14:28
@edwintorok edwintorok marked this pull request as ready for review June 15, 2022 14:36
psafont
psafont approved these changes Jun 15, 2022
View reviewed changes
Copy link
Member

@psafont psafont left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the chroot function should not be exposed. I'll fix it once this is merged

lindig
lindig approved these changes Jun 16, 2022
View reviewed changes
ocaml/xapi/xapi_xenops.ml Outdated Show resolved Hide resolved
ocaml/xenopsd/xc/domain.ml
@@ -1745,12 +1755,30 @@ let write_varstored_record task ~xs domid main_fd =
Io.write main_fd varstored_record ;
return ()

let forall f l =
let open Suspend_image.M in
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a monadic iter? In that case I would find forall a bit misleading.

ocaml/xenopsd/xc/xc_resources.ml
@@ -95,3 +95,4 @@ let nonessentials =
@ Resources.hvm_guests
@ Resources.pv_guests
@ Resources.pvinpvh_guests
@ Resources.vtpm_guests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving to List.concat as this gets longer would be good.

psafont and others added 8 commits June 17, 2022 10:31
@psafont
maintenance: post-merge fixes
7ee7e33 
Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
@edwintorok @psafont
xapi-idl: move Uuidm type to separate module
4ff283a 
Needs to be moved to a separate module to avoid cycles in the build
system, about to use this type from another module.

Signed-off-by: Edwin Török <edvin.torok@citrix.com>
@edwintorok @psafont
xapi-idl: add sexp converter to Uuidm type
3833d10 
Signed-off-by: Edwin Török <edvin.torok@citrix.com>
@edwintorok @psafont
vTPM: plumb through Uuid
a8dec3a 
Xenopsd needs to know not just whether a VM has a vTPM or not, but also
its UUID (in case a XAPI DB storage backend is used).

For now only 1 vTPM/VM is supported, as before.

Signed-off-by: Edwin Török <edvin.torok@citrix.com>
@edwintorok @psafont
varstore-guard: add filtering for vTPM.{set,get}_contents API
4e5597e 
Signed-off-by: Edwin Török <edvin.torok@citrix.com>
@edwintorok @psafont
vTPM: add minimal support for saving/restoring state through the XAPI DB
875da36 
On VM start read the vTPM state from the XAPI DB and write it out to a
file that is passed as argument to `swtpm-wrapper`.
On VM stop read the vTPM state from the filesystem and save it back into
the XAPI DB.

Note: any updated to vTPM state inbetween start/stop are lost if the
host running the VM crashes for now. To be addressed by other storage backends.

Signed-off-by: Edwin Török <edvin.torok@citrix.com>
@edwintorok @psafont
vTPM: add minimal migration support
43baa4a 
Write a new swtpm record (similar to varstore record) into the migration
stream that contains the latest vTPM state (from the filesystem).
When vTPM start is called on the other end there will be 2 sources of
vTPM state: the migration state and the xapi DB. The data from the
migration state is restored first and takes precedence: there is a check
to guard against EEXIST when starting the vTPM: if it already exists
then we must've been just migrated and we leave the state alone.

This assumes that state would be stored in the filesystem (through
either the file:// or dir:// backends of swtpm, currently the only 2
existing ones).
If other backends are implemented in the future then we would need to
retrieve the latest vTPM state via another mechanism.

TBC: would qemu attempt to restore the device state as well? But we need
to start swtpm with *some* state during migration...

Signed-off-by: Edwin Török <edvin.torok@citrix.com>
@edwintorok @psafont
vTPM: do not hardcode swtpm-wrapper path
cca5ac7 
Signed-off-by: Edwin Török <edvin.torok@citrix.com>
@psafont psafont force-pushed the private/edvint/vtpm-state-merged branch from 1898475 to cca5ac7 Compare June 17, 2022 09:31
@psafont psafont merged commit cb6161f into xapi-project:feature/vtpm Jun 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lindig lindig lindig approved these changes

@psafont psafont psafont approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@edwintorok @psafont @lindig