On 04/25/2018 01:13 PM, John Ferlan wrote:

On 04/10/2018 10:50 PM, Stefan Berger wrote:

...

Enable the TPM CRB interface added in QEMU 2.12. the TPM CRB interface is a simpler interface than the TPM TIS and is only available for TPM 2.

Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- docs/formatdomain.html.in | 2 ++ docs/schemas/domaincommon.rng | 5 +++- src/conf/domain_conf.c | 5 ++-- src/conf/domain_conf.h | 1 + src/qemu/qemu_capabilities.c | 5 ++++ src/qemu/qemu_capabilities.h | 1 + tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml | 1 + tests/qemuxml2argvdata/tpm-passthrough-crb.args | 24 +++++++++++++++ tests/qemuxml2argvdata/tpm-passthrough-crb.xml | 32 ++++++++++++++++++++ tests/qemuxml2argvtest.c | 3 ++ tests/qemuxml2xmloutdata/tpm-passthrough-crb.xml | 36 +++++++++++++++++++++++ 11 files changed, 111 insertions(+), 4 deletions(-) create mode 100644 tests/qemuxml2argvdata/tpm-passthrough-crb.args create mode 100644 tests/qemuxml2argvdata/tpm-passthrough-crb.xml create mode 100644 tests/qemuxml2xmloutdata/tpm-passthrough-crb.xml

TPM is not in my wheelhouse of knowledge, but I'm willing to learn... Still this patch seems to be more about a new model...

The TPM has different hardware interface. One is called the TIS (TPM Interface Specification) and the more recent one, typically only found with a TPM 2 underneath, is the CRB (Command Response Buffer) Interface. Both are MMIO interfaces, just work a little different. TIS was already used with a TPM 1.2 but can also be the interface of a TPM 2.

Fist off - while somewhat painful, separating the XML from the capabilities and capabilities from the qemu specific changes is generally preferred. That way the XML can be agreed upon and it shouldn't interfere with pure XML processing or xml2xml testing. Since capabilities go through periods of flux and extreme change - so separating it makes lagged reviews possible.

Thus it seems this patch gets split in at least 2 parts and perhaps a 3rd patch to alter qemu_command for TPM_CRB specific changes needs to be added.

You will also need to alter qemuxml2xmltest in order to really test that xml2xmloutdata file.

Using DO_TEST_CAPS_LATEST is the "new" methodology behind testing capability requirements for xml2argv - so you'll have some file name differences in your .args output.

Finally since there's quite a few capabilities adjustments since you posted, I cannot git am -3 apply the series at all. So this review is just "by sight". Hazards of not reviewing promptly as of late I'm afraid. Hopefully things won't be as crazy with capabilities adjustments for the next few months.

...

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 08dc74b..16fc7db 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -7628,6 +7628,8 @@ qemu-kvm -net nic,model=? /dev/null The <code>model</code> attribute specifies what device model QEMU provides to the guest. If no model name is provided, <code>tpm-tis</code> will automatically be chosen. + Another available choice is the <code>tpm-crb</code>, which + should only be used when the backend is a TPM 2. You know what this means, but does the "general" reader know what this means? I have no idea what a "TPM 2" is or means...

Now that of course depends on how much background we want to give on this page. What would it make clearer for you considering what I wrote above ?

Also needs a <since... /> type tag which will be at least 4.4.0 as we're close to locking 4.3.0 down...

I will add it.

...

</p> </dd> <dt><code>backend</code></dt> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 8165e69..be5c628 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -4112,7 +4112,10 @@ <element name="tpm"> <optional> <attribute name="model"> - <value>tpm-tis</value> + <choice> + <value>tpm-tis</value> + <value>tpm-crb</value> + </choice> </attribute> </optional> <ref name="tpm-backend"/> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ae7c0d9..232174a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -858,7 +858,8 @@ VIR_ENUM_IMPL(virDomainRNGBackend, "egd");

VIR_ENUM_IMPL(virDomainTPMModel, VIR_DOMAIN_TPM_MODEL_LAST, - "tpm-tis") + "tpm-tis", + "tpm-crb")

VIR_ENUM_IMPL(virDomainTPMBackend, VIR_DOMAIN_TPM_TYPE_LAST, "passthrough") @@ -12549,8 +12550,6 @@ virDomainTPMDefParseXML(virDomainXMLOptionPtr xmlopt, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Unknown TPM frontend model '%s'"), model); goto error; - } else { - def->model = VIR_DOMAIN_TPM_MODEL_TIS; }

Should be OK since the default is TIS (e.g. model = 0 by default).

...

ctxt->node = node; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 61379e5..1724340 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1277,6 +1277,7 @@ struct _virDomainHubDef {

typedef enum { VIR_DOMAIN_TPM_MODEL_TIS, + VIR_DOMAIN_TPM_MODEL_CRB,

VIR_DOMAIN_TPM_MODEL_LAST } virDomainTPMModel;

So model=0 is TIS and is the default...

...

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index e54dde6..0952663 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -466,6 +466,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, /* 285 */ "virtio-mouse-ccw", "virtio-tablet-ccw", + "tpm-crb", );

@@ -3104,6 +3105,10 @@ const struct tpmTypeToCaps virQEMUCapsTPMModelsToCaps[] = { .type = VIR_DOMAIN_TPM_MODEL_TIS, .caps = QEMU_CAPS_DEVICE_TPM_TIS, }, + { + .type = VIR_DOMAIN_TPM_MODEL_CRB, + .caps = QEMU_CAPS_DEVICE_TPM_CRB, + }, };

static int diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 3f3c29f..604525a 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -450,6 +450,7 @@ typedef enum { /* 285 */ QEMU_CAPS_DEVICE_VIRTIO_MOUSE_CCW, /* -device virtio-mouse-ccw */ QEMU_CAPS_DEVICE_VIRTIO_TABLET_CCW, /* -device virtio-tablet-ccw */ + QEMU_CAPS_DEVICE_TPM_CRB, /* -device tpm-crb */

QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml index 334296e..39ee4f4 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml @@ -225,6 +225,7 @@ <flag name='iscsi.password-secret'/> <flag name='isa-serial'/> <flag name='dump-completed'/> + <flag name='tpm-crb'/> <version>2011090</version> <kvmVersion>0</kvmVersion> <microcodeVersion>390060</microcodeVersion> diff --git a/tests/qemuxml2argvdata/tpm-passthrough-crb.args b/tests/qemuxml2argvdata/tpm-passthrough-crb.args new file mode 100644 index 0000000..ae052b4 --- /dev/null +++ b/tests/qemuxml2argvdata/tpm-passthrough-crb.args @@ -0,0 +1,24 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-x86_64 \ +-name TPM-VM \ +-S \ +-M pc-0.12 \ +-m 2048 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid 11d7cd22-da89-3094-6212-079a48a309a1 \ +-nographic \ +-nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-TPM-VM/monitor.sock,\ +server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=readline \ +-boot c \ +-usb \ +-tpmdev passthrough,id=tpm-tpm0,path=/dev/tpm0,\ +cancel-path=/sys/class/misc/tpm0/device/cancel \ +-device tpm-crb,tpmdev=tpm-tpm0,id=tpm0 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 FWIW: Since there was no alteration to qemu_command.c this "worked" when QEMU_CAPS_DEVICE_TPM_TIS was configured due to how qemuBuildTPMDevStr is coded.

...

diff --git a/tests/qemuxml2argvdata/tpm-passthrough-crb.xml b/tests/qemuxml2argvdata/tpm-passthrough-crb.xml new file mode 100644 index 0000000..d4f3873 --- /dev/null +++ b/tests/qemuxml2argvdata/tpm-passthrough-crb.xml @@ -0,0 +1,32 @@ +<domain type='qemu'> + <name>TPM-VM</name> + <uuid>11d7cd22-da89-3094-6212-079a48a309a1</uuid> + <memory unit='KiB'>2097152</memory> + <currentMemory unit='KiB'>512288</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc-0.12'>hvm</type> !! That's an old machine type !! Hazards of copying old file ;-) /me wonders if that'd even work w/ 2.12!

It would work with QEMU 2.12. Now the question is what to write there. When I write pc-q35-2.12 I get the following error: 568) QEMU XML-2-ARGV tpm-passthrough-crb ... libvirt: QEMU Driver error : unsupported configuration: The 'i82801b11-bridge' device is not supported by this QEMU binary FAILED I am not sure what to change so this DMI to PCI bridge doesn't get used, although '-device \?' on this executable does show this device.

...

+ <boot dev='hd'/> + <bootmenu enable='yes'/> + </os> + <features> + <acpi/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='usb' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <tpm model='tpm-crb'> + <backend type='passthrough'> + <device path='/dev/tpm0'/> + </backend> + </tpm> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 308d71f..2992197 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -2134,6 +2134,9 @@ mymain(void)

DO_TEST("tpm-passthrough", QEMU_CAPS_DEVICE_TPM_PASSTHROUGH, QEMU_CAPS_DEVICE_TPM_TIS); + DO_TEST("tpm-passthrough-crb", + QEMU_CAPS_DEVICE_TPM_PASSTHROUGH, QEMU_CAPS_DEVICE_TPM_TIS, IOW: This would have failed if you didn't add the TPM_TIS here...

Yes. In that regard I was wondering how to go about these attributes. We don't want to start QEMU but only create its command line. Can we expect that someone running the tests has the latest QEMU on the system ? I can remove it when I copy /usr/local/bin/qemu-system... to /usr/bin/.

...

+ QEMU_CAPS_DEVICE_TPM_CRB); DO_TEST_PARSE_ERROR("tpm-no-backend-invalid", QEMU_CAPS_DEVICE_TPM_PASSTHROUGH, QEMU_CAPS_DEVICE_TPM_TIS);

diff --git a/tests/qemuxml2xmloutdata/tpm-passthrough-crb.xml b/tests/qemuxml2xmloutdata/tpm-passthrough-crb.xml new file mode 100644 index 0000000..ad094a4 --- /dev/null +++ b/tests/qemuxml2xmloutdata/tpm-passthrough-crb.xml @@ -0,0 +1,36 @@ +<domain type='qemu'> + <name>TPM-VM</name> + <uuid>11d7cd22-da89-3094-6212-079a48a309a1</uuid> + <memory unit='KiB'>2097152</memory> + <currentMemory unit='KiB'>512288</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc-0.12'>hvm</type> Perhaps a more recent machine type value?

And that creates some issues with that bridge error above.

...

+ <boot dev='hd'/> + <bootmenu enable='yes'/> + </os> + <features> + <acpi/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <tpm model='tpm-crb'> + <backend type='passthrough'> + <device path='/dev/tpm0'/> + </backend> + </tpm> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </memballoon> + </devices> +</domain>

So patch 1 has:

docs/* src/conf/* tests/qemuxml2xmltest.c tests/qemuxml2argvdata/tpm-passthrough-crb.xml tests/qemuxml2xmloutdata/tpm-passthrough-crb.xml

and patch 2 has:

src/qemu/qemu_capabilities.c src/qemu/qemu_capabilities.h tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xm

and patch 3 has:

src/qemu/qemu_command.c tests/qemuxml2argvtest.c tests/qemuxml2argvdata/tpm-passthrough-crb.x86_64-latest.args

Ok. I will split it like that and post it as a separate series. Thanks for looking at it. Stefan

John

On 04/25/2018 03:13 PM, John Ferlan wrote:

On 04/25/2018 02:24 PM, Stefan Berger wrote:

...

On 04/25/2018 01:13 PM, John Ferlan wrote:

...

On 04/10/2018 10:50 PM, Stefan Berger wrote:

...

Enable the TPM CRB interface added in QEMU 2.12. the TPM CRB interface is a simpler interface than the TPM TIS and is only available for TPM 2.

Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- docs/formatdomain.html.in | 2 ++ docs/schemas/domaincommon.rng | 5 +++- src/conf/domain_conf.c | 5 ++-- src/conf/domain_conf.h | 1 + src/qemu/qemu_capabilities.c | 5 ++++ src/qemu/qemu_capabilities.h | 1 + tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml | 1 + tests/qemuxml2argvdata/tpm-passthrough-crb.args | 24 +++++++++++++++ tests/qemuxml2argvdata/tpm-passthrough-crb.xml | 32 ++++++++++++++++++++ tests/qemuxml2argvtest.c | 3 ++ tests/qemuxml2xmloutdata/tpm-passthrough-crb.xml | 36 +++++++++++++++++++++++ 11 files changed, 111 insertions(+), 4 deletions(-) create mode 100644 tests/qemuxml2argvdata/tpm-passthrough-crb.args create mode 100644 tests/qemuxml2argvdata/tpm-passthrough-crb.xml create mode 100644 tests/qemuxml2xmloutdata/tpm-passthrough-crb.xml

TPM is not in my wheelhouse of knowledge, but I'm willing to learn... Still this patch seems to be more about a new model... The TPM has different hardware interface. One is called the TIS (TPM Interface Specification) and the more recent one, typically only found with a TPM 2 underneath, is the CRB (Command Response Buffer) Interface. Both are MMIO interfaces, just work a little different. TIS was already used with a TPM 1.2 but can also be the interface of a TPM 2.

...

Fist off - while somewhat painful, separating the XML from the capabilities and capabilities from the qemu specific changes is generally preferred. That way the XML can be agreed upon and it shouldn't interfere with pure XML processing or xml2xml testing. Since capabilities go through periods of flux and extreme change - so separating it makes lagged reviews possible.

Thus it seems this patch gets split in at least 2 parts and perhaps a 3rd patch to alter qemu_command for TPM_CRB specific changes needs to be added.

You will also need to alter qemuxml2xmltest in order to really test that xml2xmloutdata file.

Using DO_TEST_CAPS_LATEST is the "new" methodology behind testing capability requirements for xml2argv - so you'll have some file name differences in your .args output.

Finally since there's quite a few capabilities adjustments since you posted, I cannot git am -3 apply the series at all. So this review is just "by sight". Hazards of not reviewing promptly as of late I'm afraid. Hopefully things won't be as crazy with capabilities adjustments for the next few months.

...

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 08dc74b..16fc7db 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -7628,6 +7628,8 @@ qemu-kvm -net nic,model=? /dev/null The <code>model</code> attribute specifies what device model QEMU provides to the guest. If no model name is provided, <code>tpm-tis</code> will automatically be chosen. + Another available choice is the <code>tpm-crb</code>, which + should only be used when the backend is a TPM 2. You know what this means, but does the "general" reader know what this means? I have no idea what a "TPM 2" is or means... Now that of course depends on how much background we want to give on this page. What would it make clearer for you considering what I wrote above ?

Ah so the "2" is a version number where it can be 1.5 or 2 and CRB is only supported on 2, while TIS is supported on both (whether the next set of patches are required for TPM 2 and TIS isn't clear to me).

To a degree I suspect this is only used by someone who knows what they are doing, but since I didn't know I figured I'd ask. Not sure if there's a happy medium. Perhaps what you wrote above is "enough" to add to the general description for TPM device in this section of the docs. Something that would go after "TPM functionality" in the first line. Enough to give a glimmer of understanding. Of course the next set of patches w/ emulator will make this even more "interesting" to describe.

...

...

Also needs a <since... /> type tag which will be at least 4.4.0 as we're close to locking 4.3.0 down... I will add it.

...

...

</p> </dd> <dt><code>backend</code></dt> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 8165e69..be5c628 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -4112,7 +4112,10 @@ <element name="tpm"> <optional> <attribute name="model"> - <value>tpm-tis</value> + <choice> + <value>tpm-tis</value> + <value>tpm-crb</value> + </choice> </attribute> </optional> <ref name="tpm-backend"/> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ae7c0d9..232174a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -858,7 +858,8 @@ VIR_ENUM_IMPL(virDomainRNGBackend, "egd"); VIR_ENUM_IMPL(virDomainTPMModel, VIR_DOMAIN_TPM_MODEL_LAST, - "tpm-tis") + "tpm-tis", + "tpm-crb") VIR_ENUM_IMPL(virDomainTPMBackend, VIR_DOMAIN_TPM_TYPE_LAST, "passthrough") @@ -12549,8 +12550,6 @@ virDomainTPMDefParseXML(virDomainXMLOptionPtr xmlopt, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Unknown TPM frontend model '%s'"), model); goto error; - } else { - def->model = VIR_DOMAIN_TPM_MODEL_TIS; }

Should be OK since the default is TIS (e.g. model = 0 by default).

...

ctxt->node = node; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 61379e5..1724340 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1277,6 +1277,7 @@ struct _virDomainHubDef { typedef enum { VIR_DOMAIN_TPM_MODEL_TIS, + VIR_DOMAIN_TPM_MODEL_CRB, VIR_DOMAIN_TPM_MODEL_LAST } virDomainTPMModel;

So model=0 is TIS and is the default...

...

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index e54dde6..0952663 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -466,6 +466,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, /* 285 */ "virtio-mouse-ccw", "virtio-tablet-ccw", + "tpm-crb", ); @@ -3104,6 +3105,10 @@ const struct tpmTypeToCaps virQEMUCapsTPMModelsToCaps[] = { .type = VIR_DOMAIN_TPM_MODEL_TIS, .caps = QEMU_CAPS_DEVICE_TPM_TIS, }, + { + .type = VIR_DOMAIN_TPM_MODEL_CRB, + .caps = QEMU_CAPS_DEVICE_TPM_CRB, + }, }; static int diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 3f3c29f..604525a 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -450,6 +450,7 @@ typedef enum { /* 285 */ QEMU_CAPS_DEVICE_VIRTIO_MOUSE_CCW, /* -device virtio-mouse-ccw */ QEMU_CAPS_DEVICE_VIRTIO_TABLET_CCW, /* -device virtio-tablet-ccw */ + QEMU_CAPS_DEVICE_TPM_CRB, /* -device tpm-crb */ QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml index 334296e..39ee4f4 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml @@ -225,6 +225,7 @@ <flag name='iscsi.password-secret'/> <flag name='isa-serial'/> <flag name='dump-completed'/> + <flag name='tpm-crb'/> <version>2011090</version> <kvmVersion>0</kvmVersion> <microcodeVersion>390060</microcodeVersion> diff --git a/tests/qemuxml2argvdata/tpm-passthrough-crb.args b/tests/qemuxml2argvdata/tpm-passthrough-crb.args new file mode 100644 index 0000000..ae052b4 --- /dev/null +++ b/tests/qemuxml2argvdata/tpm-passthrough-crb.args @@ -0,0 +1,24 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-x86_64 \ +-name TPM-VM \ +-S \ +-M pc-0.12 \ +-m 2048 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid 11d7cd22-da89-3094-6212-079a48a309a1 \ +-nographic \ +-nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-TPM-VM/monitor.sock,\ +server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=readline \ +-boot c \ +-usb \ +-tpmdev passthrough,id=tpm-tpm0,path=/dev/tpm0,\ +cancel-path=/sys/class/misc/tpm0/device/cancel \ +-device tpm-crb,tpmdev=tpm-tpm0,id=tpm0 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 FWIW: Since there was no alteration to qemu_command.c this "worked" when QEMU_CAPS_DEVICE_TPM_TIS was configured due to how qemuBuildTPMDevStr is coded.

...

diff --git a/tests/qemuxml2argvdata/tpm-passthrough-crb.xml b/tests/qemuxml2argvdata/tpm-passthrough-crb.xml new file mode 100644 index 0000000..d4f3873 --- /dev/null +++ b/tests/qemuxml2argvdata/tpm-passthrough-crb.xml @@ -0,0 +1,32 @@ +<domain type='qemu'> + <name>TPM-VM</name> + <uuid>11d7cd22-da89-3094-6212-079a48a309a1</uuid> + <memory unit='KiB'>2097152</memory> + <currentMemory unit='KiB'>512288</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc-0.12'>hvm</type> !! That's an old machine type !! Hazards of copying old file ;-) /me wonders if that'd even work w/ 2.12! It would work with QEMU 2.12. Now the question is what to write there. When I write pc-q35-2.12 I get the following error:

568) QEMU XML-2-ARGV tpm-passthrough-crb ... libvirt: QEMU Driver error : unsupported configuration: The 'i82801b11-bridge' device is not supported by this QEMU binary FAILED

I am not sure what to change so this DMI to PCI bridge doesn't get used, although '-device \?' on this executable does show this device.

I mainly noted it because it looked odd... The whole machine thing is a big black box and incurs seemingly endless discussions.

I rebased and am now using ' <type arch='x86_64' machine='pc-i440fx-2.12'>hvm</type>'. This works fine now.

...

...

...

+ <boot dev='hd'/> + <bootmenu enable='yes'/> + </os> + <features> + <acpi/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='usb' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <tpm model='tpm-crb'> + <backend type='passthrough'> + <device path='/dev/tpm0'/> + </backend> + </tpm> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 308d71f..2992197 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -2134,6 +2134,9 @@ mymain(void) DO_TEST("tpm-passthrough", QEMU_CAPS_DEVICE_TPM_PASSTHROUGH, QEMU_CAPS_DEVICE_TPM_TIS); + DO_TEST("tpm-passthrough-crb", + QEMU_CAPS_DEVICE_TPM_PASSTHROUGH, QEMU_CAPS_DEVICE_TPM_TIS, IOW: This would have failed if you didn't add the TPM_TIS here... Yes. In that regard I was wondering how to go about these attributes. We don't want to start QEMU but only create its command line. Can we expect that someone running the tests has the latest QEMU on the system ? I can remove it when I copy /usr/local/bin/qemu-system... to /usr/bin/.

Check out the DO_TEST_CAPS_LATEST macro... Note the only consumer is 'disk-drive-write-cache' and it uses 'pc-i440fx-2.6' for a machine type - hopefully this helps with the other quandary.

I see that test case now that I rebased. Do I really need the DO_TEST_CAPS_LATEST? It works without it. Maybe due to some recent changes in libvirt it doesn't seem to require this capability in the local QEMU executable? Stefan

On 04/25/2018 01:13 PM, John Ferlan wrote:

On 04/10/2018 10:50 PM, Stefan Berger wrote:

...

This patch adds support for an external swtpm TPM emulator. The XML for this type of TPM looks as follows:

<tpm model='tpm-tis'> <backend type='emulator'/> </tpm>

The XML will currently only start a TPM 1.2.

Upon first start, libvirt will run `swtpm_setup`, which will simulate the manufacturing of a TPM and create certificates for it and write them into NVRAM locations of the emulated TPM.

After that libvirt starts the swtpm TPM emulator using the `swtpm` executable.

Once the VM terminates, libvirt uses the swtpm_ioctl executable to gracefully shut down the `swtpm` in case it is still running (QEMU did not send shutdown) or clean up the socket file.

The above mentioned executables must be found in the PATH.

The executables can either be run as root or started as root and switch to the tss user. The requirement for the tss user comes through 'tcsd', which is used for the simulation of the manufacturing. Which user is used can be configured through qemu.conf. By default 'tss' is used.

The swtpm writes out state into files. The state is kept in /var/lib/libvirt/swtpm:

[root@localhost libvirt]# ls -lZ | grep swtpm

drwx--x--x. 7 root root unconfined_u:object_r:virt_var_lib_t:s0 4096 Apr 5 16:22 swtpm

The directory /var/lib/libvirt/swtpm maintains per-TPM state directories.

[root@localhost swtpm]# ls -lZ total 4 drwx------. 2 tss tss system_u:object_r:virt_var_lib_t:s0 4096 Apr 5 16:46 testvm

[root@localhost testvm]# ls -lZ total 4 drwx------. 2 tss tss system_u:object_r:virt_var_lib_t:s0 4096 Apr 10 21:34 tpm1.2

[root@localhost tpm1.2]# ls -lZ total 8 -rw-r--r--. 1 tss tss system_u:object_r:virt_var_lib_t:s0 3648 Apr 5 16:46 tpm-00.permall

The directory /var/run/libvirt/qemu/swtpm/domain-1-testvm hosts the swtpm.sock that QEMU uses to communicate with the swtpm:

root@localhost domain-1-testvm]# ls -lZ total 0 srw-------. 1 qemu qemu system_u:object_r:svirt_image_t:s0:c597,c632 0 Apr 6 10:24 swtpm.sock

The logfile for the swtpm is in /var/log/swtpm/libvirt/qemu:

[root@localhost-3 qemu]# ls -lZ total 4 -rw-------. 1 tss tss unconfined_u:object_r:var_log_t:s0 2199 Apr 6 14:01 testvm-swtpm.log

The processes are labeled as follows:

[root@localhost 485d0004-a48f-436a-8457-8a3b73e28567]# ps auxZ | grep swtpm | grep socket | grep -v grep system_u:system_r:virtd_t:s0-s0:c0.c1023 tss 18697 0.0 0.0 28172 3892 ? Ss 16:46 0:00 /usr/bin/swtpm socket --daemon --ctrl type=unixio,path=/var/run/libvirt/qemu/swtpm/testvm-swtpm.sock,mode=0660 --tpmstate dir=/var/lib/libvirt/swtpm/testvm/tpm1.2 --log file=/var/log/swtpm/libvirt/qemu/testvm-swtpm.log

[root@localhost 485d0004-a48f-436a-8457-8a3b73e28567]# ps auxZ | grep qemu | grep tpm | grep -v grep system_u:system_r:svirt_t:s0:c413,c430 qemu 18702 2.5 0.0 3036052 48676 ? Sl 16:46 0:08 /bin/qemu-system-x86_64 [...]

Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- docs/formatdomain.html.in | 30 +++ docs/schemas/domaincommon.rng | 5 + src/conf/domain_audit.c | 2 + src/conf/domain_conf.c | 49 +++- src/conf/domain_conf.h | 7 + src/libvirt_private.syms | 7 + src/qemu/Makefile.inc.am | 2 + src/qemu/libvirtd_qemu.aug | 3 + src/qemu/qemu.conf | 7 + src/qemu/qemu_capabilities.c | 5 + src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_cgroup.c | 1 + src/qemu/qemu_command.c | 52 +++- src/qemu/qemu_conf.c | 35 ++- src/qemu/qemu_conf.h | 5 + src/qemu/qemu_domain.c | 4 + src/qemu/qemu_driver.c | 7 + src/qemu/qemu_extdevice.c | 264 ++++++++++++++++++++ src/qemu/qemu_extdevice.h | 44 ++++ src/qemu/qemu_process.c | 12 + src/qemu/test_libvirtd_qemu.aug.in | 1 + src/security/security_dac.c | 6 + src/security/security_selinux.c | 7 + src/util/virfile.c | 60 +++++ src/util/virfile.h | 2 + src/util/virtpm.c | 493 ++++++++++++++++++++++++++++++++++++- src/util/virtpm.h | 25 +- 27 files changed, 1121 insertions(+), 15 deletions(-) create mode 100644 src/qemu/qemu_extdevice.c create mode 100644 src/qemu/qemu_extdevice.h

Wow that's a lot of stuff changing... Again, you should alter XML, then add the capability, and then alter qemu...

I split it up now. I'll pick up as many changes from below as possible. Thanks again.

Perhaps the util/virtpm.{c,h} files could be their own patch assuming they compile separately. The pieces of qemu code that would support the eventual ability to launch qemu with this emulator model should be in place before it's possible to launch - IOW conf, security, and cgroup changes before qemu_command can adds support.

I think this and the next 4 patches needs some rearranging. It seems that this alteration should be in a separate patch series from the CRB series.

I'll scan through the rest of this patch, but nothing to in depth.

...

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 16fc7db..bd6fedc 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -7621,6 +7621,26 @@ qemu-kvm -net nic,model=? /dev/null </devices> ... </pre> + + <p> + The emulator device type gives access to a TPM emulator providing + TPM functionlity for each VM. QEMU talks to it over a UnixIO socket. With functionality

I'm having a brain lock right now "UnixIO socket" - probably could just be a Unix socket I think. Thankfully unlike the persistent reservations work also on list waiting to be reviewed there isn't a configurable socket path option!

Changed to 'Unix socket'.

...

+ the emulator device type each guest gets its own private TPM. + <span class="since">'emulator' since 4.x.y</span> + </p> + <p> + Example: usage of the TPM Emulator + </p> +<pre> + ... + <devices> + <tpm model='tpm-tis'> + <backend type='emulator'> + </backend> + </tpm> + </devices> + ... +</pre> <dl> <dt><code>model</code></dt> <dd> @@ -7653,6 +7673,16 @@ qemu-kvm -net nic,model=? /dev/null </p> </dd> </dl> + <dl> + <dt><code>emulator</code></dt> + <dd> + <p> + For this backend type the 'swtpm' TPM Emulator must be installed on the + host. Libvirt will automatically start an independent TPM emulator + for each QEMU guest requesting access to it. + </p> + </dd> + </dl> </dd> </dl>

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index be5c628..d628444 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -4134,6 +4134,11 @@ </attribute> <ref name="tpm-passthrough-device"/> </group> + <group> + <attribute name="type"> + <value>emulator</value> + </attribute> + </group> </choice> </element> </define> diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c index 82868bc..25cccdd 100644 --- a/src/conf/domain_audit.c +++ b/src/conf/domain_audit.c @@ -586,6 +586,8 @@ virDomainAuditTPM(virDomainObjPtr vm, virDomainTPMDefPtr tpm, "virt=%s resrc=dev reason=%s %s uuid=%s %s", virt, reason, vmname, uuidstr, device); break; + case VIR_DOMAIN_TPM_TYPE_EMULATOR: + break; case VIR_DOMAIN_TPM_TYPE_LAST: default: break; diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 232174a..b5f1c3f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -862,7 +862,8 @@ VIR_ENUM_IMPL(virDomainTPMModel, VIR_DOMAIN_TPM_MODEL_LAST, "tpm-crb")

VIR_ENUM_IMPL(virDomainTPMBackend, VIR_DOMAIN_TPM_TYPE_LAST, - "passthrough") + "passthrough", + "emulator")

VIR_ENUM_IMPL(virDomainIOMMUModel, VIR_DOMAIN_IOMMU_MODEL_LAST, "intel") @@ -2588,6 +2589,24 @@ void virDomainHostdevDefClear(virDomainHostdevDefPtr def) } }

Two blank lines between new functions and we like Free instead of Delete unless of course this is something more specific...

This is something more specifc. The TPM emulator writes state into files in a dedicated directory. That state would normally be written into NVRAM of a TPM so that, after it is powered up again, it has that old state again (keys etc.) . We only delete this state and the per-VM directory tree when the domain is undefined.

...

+void virDomainTPMDelete(virDomainDefPtr def) use:

void virDomainTPMFree(virDomainDefPtr def)

...

+{ + virDomainTPMDefPtr tpm = def->tpm; + + if (!tpm) + return; + + switch (tpm->type) { + case VIR_DOMAIN_TPM_TYPE_EMULATOR: + virTPMDeleteEmulatorStorage(tpm); I would expect the def->tpm to also be deleted, but all this is doing is freeing the storagepath tree which would say to me perhaps the callers of this should just call virTPMDeleteEmulatorStorage

This function may be called from other places that should do the switch on the tpm->type as is done above.

...

+ break; + case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: + case VIR_DOMAIN_TPM_TYPE_LAST: + /* nothing to do */ + break; + } +} + void virDomainTPMDefFree(virDomainTPMDefPtr def) Yes, I see this doesn't follow the model outlined above. I'm fine with it changing as part of this.

...

{ if (!def) @@ -2597,6 +2616,11 @@ void virDomainTPMDefFree(virDomainTPMDefPtr def) case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: VIR_FREE(def->data.passthrough.source.data.file.path); break; + case VIR_DOMAIN_TPM_TYPE_EMULATOR: and this perhaps is where I'd think virTPMDeleteEmulatorStorage would be called.

The virDomainTPMDefFree() may be called for other reasons that I don't think should include the deletion of the directory tree structure where the TPM emulator stores its state. I think this may become clearer once I post the separated patches.

...

+ VIR_FREE(def->data.emulator.source.data.nix.path); + VIR_FREE(def->data.emulator.storagepath); + VIR_FREE(def->data.emulator.logfile); + break; case VIR_DOMAIN_TPM_TYPE_LAST: break; } @@ -12525,6 +12549,11 @@ virDomainSmartcardDefParseXML(virDomainXMLOptionPtr xmlopt, * </backend> * </tpm> * + * or like this: + * + * <tpm model='tpm-tis'> + * <backend type='emulator'/> + * </tpm> */ static virDomainTPMDefPtr virDomainTPMDefParseXML(virDomainXMLOptionPtr xmlopt, @@ -12591,6 +12620,8 @@ virDomainTPMDefParseXML(virDomainXMLOptionPtr xmlopt, def->data.passthrough.source.type = VIR_DOMAIN_CHR_TYPE_DEV; path = NULL; break; + case VIR_DOMAIN_TPM_TYPE_EMULATOR: + break; case VIR_DOMAIN_TPM_TYPE_LAST: goto error; } @@ -24760,24 +24791,32 @@ virDomainTPMDefFormat(virBufferPtr buf, virDomainTPMDefPtr def, unsigned int flags) { + bool did_nl = false; + virBufferAsprintf(buf, "<tpm model='%s'>\n", virDomainTPMModelTypeToString(def->model)); virBufferAdjustIndent(buf, 2); - virBufferAsprintf(buf, "<backend type='%s'>\n", + virBufferAsprintf(buf, "<backend type='%s'", virDomainTPMBackendTypeToString(def->type)); virBufferAdjustIndent(buf, 2);

switch (def->type) { case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: + virBufferAddLit(buf, ">\n"); + did_nl = true; virBufferEscapeString(buf, "<device path='%s'/>\n", def->data.passthrough.source.data.file.path); break; + case VIR_DOMAIN_TPM_TYPE_EMULATOR: case VIR_DOMAIN_TPM_TYPE_LAST: break; }

virBufferAdjustIndent(buf, -2); - virBufferAddLit(buf, "</backend>\n"); + if (did_nl) + virBufferAddLit(buf, "</backend>\n"); + else + virBufferAddLit(buf, "/>\n"); Eww... hard to read/follow

I know. The good thing here are the test cases...

Maybe the def->type switch should handle the <backend> printing completely for which ever type this is...

I'll move it up.

...

virDomainDeviceInfoFormat(buf, &def->info, flags);

@@ -27548,6 +27587,10 @@ virDomainDeleteConfig(const char *configDir, goto cleanup; }

+ /* in case domain is NOT running, remove any TPM storage */ + if (!dom->persistent)

Well this has less to do with running and more to do with persistence vs. transient

Iirc this comes from the following: I ran into this case where I had a running domain and undefined it. The domain keeps running of course, the XML definition is gone. In this case we cannot just delete the TPM emulator's storage while it is running but we have to do it later once that VM terminates.

...

+ virDomainTPMDelete(dom->def); + ret = 0;

cleanup: diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 1724340..f632184 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1284,6 +1284,7 @@ typedef enum {

typedef enum { VIR_DOMAIN_TPM_TYPE_PASSTHROUGH, + VIR_DOMAIN_TPM_TYPE_EMULATOR,

VIR_DOMAIN_TPM_TYPE_LAST } virDomainTPMBackendType; @@ -1298,6 +1299,11 @@ struct _virDomainTPMDef { struct { virDomainChrSourceDef source; } passthrough; + struct { + virDomainChrSourceDef source; + char *storagepath; + char *logfile; + } emulator; } data; }; Perhaps separate the above hunk and deal with all the switches that need a case in one patch with dummy code that just accepts it. Then subsequent patches add "real code" for dummy pieces. Just a thought - makes it easier to find places later on that care about the model type.

...

@@ -2810,6 +2816,7 @@ int virDomainDeviceAddressIsValid(virDomainDeviceInfoPtr info, int type); virDomainDeviceInfoPtr virDomainDeviceGetInfo(virDomainDeviceDefPtr device); void virDomainTPMDefFree(virDomainTPMDefPtr def); +void virDomainTPMDelete(virDomainDefPtr def);

typedef int (*virDomainDeviceInfoCallback)(virDomainDefPtr def, virDomainDeviceDefPtr dev, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 03fe3b3..935ffcc 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -556,6 +556,7 @@ virDomainTimerTrackTypeToString; virDomainTPMBackendTypeFromString; virDomainTPMBackendTypeToString; virDomainTPMDefFree; +virDomainTPMDelete; virDomainTPMModelTypeFromString; virDomainTPMModelTypeToString; virDomainUSBDeviceDefForeach; @@ -1745,6 +1746,7 @@ saferead; safewrite; safezero; virBuildPathInternal; +virDirChownFiles;

This feels separable too

Did that, yes.

...

virDirClose; virDirCreate; virDirOpen; @@ -2971,6 +2973,11 @@ virTimeStringThenRaw;

# util/virtpm.h virTPMCreateCancelPath; +virTPMDeleteEmulatorStorage; +virTPMEmulatorBuildCommand; +virTPMEmulatorInitPaths; +virTPMEmulatorPrepareHost; +virTPMEmulatorStop;

# util/virtypedparam.h diff --git a/src/qemu/Makefile.inc.am b/src/qemu/Makefile.inc.am index 8ef290a..6c8daf8 100644 --- a/src/qemu/Makefile.inc.am +++ b/src/qemu/Makefile.inc.am @@ -19,6 +19,8 @@ QEMU_DRIVER_SOURCES = \ qemu/qemu_domain_address.h \ qemu/qemu_cgroup.c \ qemu/qemu_cgroup.h \ + qemu/qemu_extdevice.c \ + qemu/qemu_extdevice.h \ qemu/qemu_hostdev.c \ qemu/qemu_hostdev.h \ qemu/qemu_hotplug.c \ diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug index c19bf3a..cc5d657 100644 --- a/src/qemu/libvirtd_qemu.aug +++ b/src/qemu/libvirtd_qemu.aug @@ -118,6 +118,8 @@ module Libvirtd_qemu = let vxhs_entry = bool_entry "vxhs_tls" | str_entry "vxhs_tls_x509_cert_dir"

+ let swtpm_entry = str_entry "swtpm_user" + (* Each entry in the config is one of the following ... *) let entry = default_tls_entry | vnc_entry @@ -137,6 +139,7 @@ module Libvirtd_qemu = | gluster_debug_level_entry | memory_entry | vxhs_entry + | swtpm_entry

let comment = [ label "#comment" . del /#[ \t]*/ "# " . store /([^ \t\n][^\n]*)?/ . del /\n/ "\n" ] let empty = [ label "#empty" . eol ] diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index 43dd561..f64ae68 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -775,3 +775,10 @@ # This directory is used for memoryBacking source if configured as file. # NOTE: big files will be stored here #memory_backing_dir = "/var/lib/libvirt/qemu/ram" + +# User for the swtpm TPM Emulator +# +# Default is 'tss'; this is the same user that tcsd (TrouSerS) installs +# and uses; alternative is 'root' +# +#swtpm_user = "tss" All the qemu.conf related stuff should be its own patch.

also did that

...

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 0952663..ce4db62 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -467,6 +467,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "virtio-mouse-ccw", "virtio-tablet-ccw", "tpm-crb", + "tpm-emulator", );

@@ -3098,6 +3099,10 @@ static const struct tpmTypeToCaps virQEMUCapsTPMTypesToCaps[] = { .type = VIR_DOMAIN_TPM_TYPE_PASSTHROUGH, .caps = QEMU_CAPS_DEVICE_TPM_PASSTHROUGH, }, + { + .type = VIR_DOMAIN_TPM_TYPE_EMULATOR, + .caps = QEMU_CAPS_DEVICE_TPM_EMULATOR, + }, };

const struct tpmTypeToCaps virQEMUCapsTPMModelsToCaps[] = { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 604525a..0cc2882 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -451,6 +451,7 @@ typedef enum { QEMU_CAPS_DEVICE_VIRTIO_MOUSE_CCW, /* -device virtio-mouse-ccw */ QEMU_CAPS_DEVICE_VIRTIO_TABLET_CCW, /* -device virtio-tablet-ccw */ QEMU_CAPS_DEVICE_TPM_CRB, /* -device tpm-crb */ + QEMU_CAPS_DEVICE_TPM_EMULATOR, /* -tpmdev emulator */

QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; Capabilities changes in their own patch which also seems to pull in parts of patch3 (the caps_*.xml changes)

...

diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index b604edb..bd4859c 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -238,6 +238,7 @@ qemuSetupTPMCgroup(virDomainObjPtr vm) case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: ret = qemuSetupChrSourceCgroup(vm, &dev->data.passthrough.source); break; + case VIR_DOMAIN_TPM_TYPE_EMULATOR: case VIR_DOMAIN_TPM_TYPE_LAST: break; } diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 89fd08b..878a147 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -9614,21 +9614,33 @@ qemuBuildTPMDevStr(const virDomainDef *def,

static char * -qemuBuildTPMBackendStr(const virDomainDef *def, +qemuBuildTPMBackendStr(virDomainDef *def, + virQEMUDriverPtr driver, If all we need @driver for is @cfg, then why not pass @cfg into qemuBuildTPMCommandLine and then into here? That way this code doesn't manage yet another reference to @cfg

Good point. Done.

...

virCommandPtr cmd, virQEMUCapsPtr qemuCaps, int *tpmfd, - int *cancelfd) + int *cancelfd, + char **chardev) { - const virDomainTPMDef *tpm = def->tpm; + virDomainTPMDef *tpm = def->tpm; virBuffer buf = VIR_BUFFER_INITIALIZER; - const char *type = virDomainTPMBackendTypeToString(tpm->type); + const char *type = NULL; char *cancel_path = NULL, *devset = NULL; const char *tpmdev; + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);

*tpmfd = -1; *cancelfd = -1;

+ switch (tpm->type) { + case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: + case VIR_DOMAIN_TPM_TYPE_EMULATOR: + type = virDomainTPMBackendTypeToString(tpm->type); + break; + case VIR_DOMAIN_TPM_TYPE_LAST: + goto error; + } + virBufferAsprintf(&buf, "%s,id=tpm-%s", type, tpm->info.alias);

switch (tpm->type) { @@ -9679,6 +9691,17 @@ qemuBuildTPMBackendStr(const virDomainDef *def, VIR_FREE(cancel_path);

break; + case VIR_DOMAIN_TPM_TYPE_EMULATOR: + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_TPM_EMULATOR)) + goto no_support; + + virBufferAddLit(&buf, ",chardev=chrtpm"); + + if (virAsprintf(chardev, "socket,id=chrtpm,path=%s", + tpm->data.emulator.source.data.nix.path) < 0) + goto error; + + break; case VIR_DOMAIN_TPM_TYPE_LAST: goto error; } @@ -9686,6 +9709,8 @@ qemuBuildTPMBackendStr(const virDomainDef *def, if (virBufferCheckError(&buf) < 0) goto error;

+ virObjectUnref(cfg); + return virBufferContentAndReset(&buf);

no_support: @@ -9699,16 +9724,19 @@ qemuBuildTPMBackendStr(const virDomainDef *def, VIR_FREE(cancel_path);

virBufferFreeAndReset(&buf); + virObjectUnref(cfg); return NULL; }

static int -qemuBuildTPMCommandLine(virCommandPtr cmd, - const virDomainDef *def, +qemuBuildTPMCommandLine(virQEMUDriverPtr driver, + virCommandPtr cmd, + virDomainDef *def, virQEMUCapsPtr qemuCaps) { char *optstr; + char *chardev = NULL; int tpmfd = -1; int cancelfd = -1; char *fdset; @@ -9716,13 +9744,19 @@ qemuBuildTPMCommandLine(virCommandPtr cmd, if (!def->tpm) return 0;

- if (!(optstr = qemuBuildTPMBackendStr(def, cmd, qemuCaps, - &tpmfd, &cancelfd))) + if (!(optstr = qemuBuildTPMBackendStr(def, driver, cmd, qemuCaps, + &tpmfd, &cancelfd, + &chardev))) return -1;

virCommandAddArgList(cmd, "-tpmdev", optstr, NULL); VIR_FREE(optstr);

+ if (chardev) { + virCommandAddArgList(cmd, "-chardev", chardev, NULL); + VIR_FREE(chardev); + } + if (tpmfd >= 0) { fdset = qemuVirCommandGetFDSet(cmd, tpmfd); if (!fdset) @@ -10151,7 +10185,7 @@ qemuBuildCommandLine(virQEMUDriverPtr driver, chardevStdioLogd) < 0) goto error;

- if (qemuBuildTPMCommandLine(cmd, def, qemuCaps) < 0) + if (qemuBuildTPMCommandLine(driver, cmd, def, qemuCaps) < 0)

Passing @cfg from here should be fine just like other callers do... "...cmd, cfg, ..."

Done :-)

...

goto error;

if (qemuBuildInputCommandLine(cmd, def, qemuCaps) < 0) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 36cf3a2..486b314 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -164,6 +164,10 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) "%s/log/libvirt/qemu", LOCALSTATEDIR) < 0) goto error;

+ if (virAsprintf(&cfg->swtpmLogDir, + "%s/log/swtpm/libvirt/qemu", LOCALSTATEDIR) < 0) + goto error; + if (VIR_STRDUP(cfg->configBaseDir, SYSCONFDIR "/libvirt") < 0) goto error;

@@ -171,6 +175,10 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) "%s/run/libvirt/qemu", LOCALSTATEDIR) < 0) goto error;

+ if (virAsprintf(&cfg->swtpmStateDir, + "%s/run/libvirt/qemu/swtpm", LOCALSTATEDIR) < 0) + goto error; + if (virAsprintf(&cfg->cacheDir, "%s/cache/libvirt/qemu", LOCALSTATEDIR) < 0) goto error; @@ -191,6 +199,9 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) goto error; if (virAsprintf(&cfg->memoryBackingDir, "%s/ram", cfg->libDir) < 0) goto error; + if (virAsprintf(&cfg->swtpmStorageDir, "%s/lib/libvirt/swtpm", + LOCALSTATEDIR) < 0) + goto error; } else { char *rundir; char *cachedir; @@ -204,6 +215,11 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) VIR_FREE(cachedir); goto error; } + if (virAsprintf(&cfg->swtpmLogDir, + "%s/qemu/log", cachedir) < 0) { + VIR_FREE(cachedir); + goto error; + } if (virAsprintf(&cfg->cacheDir, "%s/qemu/cache", cachedir) < 0) { VIR_FREE(cachedir); goto error; @@ -219,6 +235,9 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) } VIR_FREE(rundir);

+ if (virAsprintf(&cfg->swtpmStateDir, "%s/qemu/run/swtpm", rundir) < 0) + goto error; + if (!(cfg->configBaseDir = virGetUserConfigDirectory())) goto error;

@@ -238,6 +257,8 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) goto error; if (virAsprintf(&cfg->memoryBackingDir, "%s/qemu/ram", cfg->configBaseDir) < 0) goto error; + if (virAsprintf(&cfg->swtpmStorageDir, "%s/qemu/swtpm", cfg->configBaseDir) < 0) + goto error; }

if (virAsprintf(&cfg->configDir, "%s/qemu", cfg->configBaseDir) < 0) @@ -336,6 +357,9 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) &cfg->nfirmwares) < 0) goto error;

+ if (virGetUserID("tss", &cfg->swtpm_user) < 0) + cfg->swtpm_user = 0; /* root */ +

Something doesn't feel right here especially for unprivileged mode.

I haven't tried it in unprivileged mode. Not sure how to invoke it even...

If there isn't a "tss" user, then are things configured correctly?

There are only two choice, one is tss and the other one is root.

Should we follow the other callers to virGetUserID and set to -1 instead?

Set swtpm_user = -1; ? What happens when we need to actually use this variable then ? Report an error ?

Still 0 is the default for @cfg members anyway.

...

return cfg;

error: @@ -356,7 +380,9 @@ static void virQEMUDriverConfigDispose(void *obj) VIR_FREE(cfg->configDir); VIR_FREE(cfg->autostartDir); VIR_FREE(cfg->logDir); + VIR_FREE(cfg->swtpmLogDir); VIR_FREE(cfg->stateDir); + VIR_FREE(cfg->swtpmStateDir);

VIR_FREE(cfg->libDir); VIR_FREE(cfg->cacheDir); @@ -405,6 +431,7 @@ static void virQEMUDriverConfigDispose(void *obj) virFirmwareFreeList(cfg->firmwares, cfg->nfirmwares);

VIR_FREE(cfg->memoryBackingDir); + VIR_FREE(cfg->swtpmStorageDir); }

@@ -475,7 +502,7 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, int rv; size_t i, j; char *stdioHandler = NULL; - char *user = NULL, *group = NULL; + char *user = NULL, *group = NULL, *swtpm_user = NULL; char **controllers = NULL; char **hugetlbfs = NULL; char **nvram = NULL; @@ -912,6 +939,11 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, if (virConfGetValueString(conf, "memory_backing_dir", &cfg->memoryBackingDir) < 0) goto cleanup;

+ if (virConfGetValueString(conf, "swtpm_user", &swtpm_user) < 0) + goto cleanup; + if (swtpm_user && virGetUserID(swtpm_user, &cfg->swtpm_user) < 0) + goto cleanup; + ret = 0;

cleanup: @@ -922,6 +954,7 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, VIR_FREE(corestr); VIR_FREE(user); VIR_FREE(group); + VIR_FREE(swtpm_user); virConfFree(conf); return ret; } diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index e1ad546..93d3c65 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -102,7 +102,9 @@ struct _virQEMUDriverConfig { char *configDir; char *autostartDir; char *logDir; + char *swtpmLogDir; char *stateDir; + char *swtpmStateDir; /* These two directories are ones QEMU processes use (so must match * the QEMU user/group */ char *libDir; @@ -111,6 +113,7 @@ struct _virQEMUDriverConfig { char *snapshotDir; char *channelTargetDir; char *nvramDir; + char *swtpmStorageDir;

char *defaultTLSx509certdir; bool checkdefaultTLSx509certdir; @@ -206,6 +209,8 @@ struct _virQEMUDriverConfig {

bool vxhsTLS; char *vxhsTLSx509certdir; + + uid_t swtpm_user; };

/* Main driver state */ diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 580e0f8..542b67b 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -33,6 +33,7 @@ #include "qemu_capabilities.h" #include "qemu_migration.h" #include "qemu_security.h" +#include "qemu_extdevice.h" #include "viralloc.h" #include "virlog.h" #include "virerror.h" @@ -7088,6 +7089,8 @@ qemuDomainRemoveInactive(virQEMUDriverPtr driver, VIR_WARN("unable to remove snapshot directory %s", snapDir); VIR_FREE(snapDir); } + if (!qemuExtDevicesInitPaths(driver, vm->def)) + virDomainTPMDelete(vm->def);

qemuExtDevicesInitPaths returns an int not a bool...

It would return -1 on failure to initialize the paths and 0 when it succeeds...

Still strange to see on a Remove/Inactive path

The storage path is not part of the domain XML and needs to be set before we can remove an inactive domain. I think this part here gets invoked when an undefined VM terminates.

...

virObjectRef(vm);

@@ -10280,6 +10283,7 @@ qemuDomainSetupTPM(virQEMUDriverConfigPtr cfg ATTRIBUTE_UNUSED, return -1; break;

+ case VIR_DOMAIN_TPM_TYPE_EMULATOR: case VIR_DOMAIN_TPM_TYPE_LAST: /* nada */ break; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 7bcc493..066aa4a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -59,6 +59,7 @@ #include "qemu_migration.h" #include "qemu_blockjob.h" #include "qemu_security.h" +#include "qemu_extdevice.h"

#include "virerror.h" #include "virlog.h" @@ -7365,6 +7366,9 @@ qemuDomainCreateWithFlags(virDomainPtr dom, unsigned int flags) goto endjob; }

+ if (qemuExtDevicesInitPaths(driver, vm->def) < 0) + goto endjob; +

This one I get - it's a creation path...

The other one is necessary when one terminated libvirt and restarts it. In that case these paths need to be set again.

...

if (qemuDomainObjStart(dom->conn, driver, vm, flags, QEMU_ASYNC_JOB_START) < 0) goto endjob; @@ -7510,6 +7514,9 @@ qemuDomainUndefineFlags(virDomainPtr dom, if (!(vm = qemuDomObjFromDomain(dom))) return -1;

+ if (qemuExtDevicesInitPaths(driver, vm->def) < 0) + return -1;

Again, confused over failure in an undefine path? Or is this just git diff fooling me?

This is for a domain that's being undefined but was never started (which would have caused the path initialization) since libvirtd was restarted. Again, we need to initialize the paths. If there was a better place to put these paths, I'd be curious where that is.

...

+ cfg = virQEMUDriverGetConfig(driver);

if (virDomainUndefineFlagsEnsureACL(dom->conn, vm->def) < 0) diff --git a/src/qemu/qemu_extdevice.c b/src/qemu/qemu_extdevice.c new file mode 100644 index 0000000..be3df7c --- /dev/null +++ b/src/qemu/qemu_extdevice.c @@ -0,0 +1,264 @@ +/* + * qemu_extdevice.c: QEMU external devices support + * + * Copyright (C) 2014, 2018 IBM Corporation + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Author: Stefan Berger <stefanb@linux.vnet.ibm.com> + */ + +#include <config.h> + +#include "qemu_extdevice.h" +#include "qemu_domain.h" + +#include "viralloc.h" +#include "virlog.h" +#include "virstring.h" +#include "virtime.h" +#include "virtpm.h" + +#define VIR_FROM_THIS VIR_FROM_QEMU + +VIR_LOG_INIT("qemu.qemu_extdevice") + +static int +qemuExtDeviceLogCommand(qemuDomainLogContextPtr logCtxt, + virCommandPtr cmd, + const char *info) +{ + int ret = -1; + char *timestamp = NULL; + char *logline = NULL; + int logFD; + + logFD = qemuDomainLogContextGetWriteFD(logCtxt); + + if ((timestamp = virTimeStringNow()) == NULL) + goto cleanup; + + if (virAsprintf(&logline, "%s: Starting external device: %s\n", + timestamp, info) < 0) + goto cleanup; + + if (safewrite(logFD, logline, strlen(logline)) < 0) + goto cleanup; + + virCommandWriteArgLog(cmd, logFD); + + ret = 0; + + cleanup: + VIR_FREE(timestamp); + VIR_FREE(logline); + + return ret; +} More recently we prefer two blank lines between functions.

...

+ +static int qemuExtTPMInitPaths(virQEMUDriverPtr driver, + virDomainDefPtr def) You're mixing function entry point formats, use:

static int qemuExtTPMInitPaths(virQEMUDriverPtr driver, virDomainDefPtr def)

This repeats throughout.

Fixed.

...

+{ + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + int ret = 0; + + switch (def->tpm->type) { + case VIR_DOMAIN_TPM_TYPE_EMULATOR: + ret = virTPMEmulatorInitPaths(def->tpm, cfg->swtpmStorageDir, def->name); + break; + case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: + case VIR_DOMAIN_TPM_TYPE_LAST: + break; + } + You leak @cfg here.

Added virObjectUnref(cfg) a couple of times now.

...

+ return ret; +} + +static int qemuExtTPMPrepareHost(virQEMUDriverPtr driver, + virDomainDefPtr def) +{ + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + int ret = 0; + + switch (def->tpm->type) { + case VIR_DOMAIN_TPM_TYPE_EMULATOR: + ret = virTPMEmulatorPrepareHost(def->tpm, cfg->swtpmLogDir, + def->name, cfg->swtpm_user, + cfg->swtpmStateDir, cfg->user); + break; + case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: + case VIR_DOMAIN_TPM_TYPE_LAST: + break; + } + again @cfg is leaked.

...

+ return ret; +} + +/* + * qemuExtTPMStartEmulator: + * + * @driver: QEMU driver + * @def: domain definition + * @logCtxt: log context + * + * Start the external TPM Emulator: + * - have the command line built + * - start the external TPM Emulator and sync with it before QEMU start + */ +static int +qemuExtTPMStartEmulator(virQEMUDriverPtr driver, + virDomainDefPtr def, + qemuDomainLogContextPtr logCtxt) +{ + int ret = -1; + virCommandPtr cmd = NULL; + int exitstatus; + char *errbuf = NULL; + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + virDomainTPMDefPtr tpm = def->tpm; + + /* stop any left-over TPM emulator for this VM */ + virTPMEmulatorStop(cfg->swtpmStateDir, def->name); + + if (!(cmd = virTPMEmulatorBuildCommand(tpm, def->name, def->uuid, + cfg->swtpm_user))) + goto cleanup; + + if (qemuExtDeviceLogCommand(logCtxt, cmd, "TPM Emulator") < 0) + goto cleanup; + + virCommandSetErrorBuffer(cmd, &errbuf); + + if (virCommandRun(cmd, &exitstatus) < 0 || exitstatus != 0) { + VIR_ERROR("Could not start 'swtpm'. exitstatus: %d\n" + "stderr: %s\n", exitstatus, errbuf); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not start 'swtpm'. exitstatus: %d, " + "error: %s"), exitstatus, errbuf); + goto error; + } + + ret = 0; + + cleanup: + VIR_FREE(errbuf); + virCommandFree(cmd); + + virObjectUnref(cfg); + + return ret; + + error: + virTPMEmulatorStop(cfg->swtpmStateDir, def->name); Wouldn't be running right?

Yes, removed.

...

+ VIR_FREE(tpm->data.emulator.source.data.nix.path); + + goto cleanup; +} + +static int +qemuExtTPMStart(virQEMUDriverPtr driver, + virDomainDefPtr def, + qemuDomainLogContextPtr logCtxt) +{ + int ret = 0; + virDomainTPMDefPtr tpm = def->tpm; + + switch (tpm->type) { + case VIR_DOMAIN_TPM_TYPE_EMULATOR: + ret = qemuExtTPMStartEmulator(driver, def, logCtxt); + break; + case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: + case VIR_DOMAIN_TPM_TYPE_LAST: + break; + } + + return ret; +} + +static void +qemuExtTPMStop(virQEMUDriverPtr driver, virDomainDefPtr def) +{ + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + + switch (def->tpm->type) { + case VIR_DOMAIN_TPM_TYPE_EMULATOR: + virTPMEmulatorStop(cfg->swtpmStateDir, def->name); + break; + case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: + case VIR_DOMAIN_TPM_TYPE_LAST: + break; + } Leaking @cfg

...

+} + +/* + * qemuExtDevicesInitPaths: + * + * @driver: QEMU driver + * @def: domain definition + * + * Initialize paths of external devices so that it is known where state is + * stored and we can remove directories and files in case of domain XML + * changes. + */ +int qemuExtDevicesInitPaths(virQEMUDriverPtr driver, + virDomainDefPtr def) +{ + int ret = 0; + + if (def->tpm) + ret = qemuExtTPMInitPaths(driver, def); + + return ret; +} + +/* + * qemuExtDevicesPrepareHost: + * + * @driver: QEMU driver + * @def: domain definition + * + * Prepare host storage paths for external devices. + */ +int qemuExtDevicesPrepareHost(virQEMUDriverPtr driver, + virDomainDefPtr def) +{ + int ret = 0; + + if (def->tpm) + ret = qemuExtTPMPrepareHost(driver, def); + + return ret; +} + +int +qemuExtDevicesStart(virQEMUDriverPtr driver, + virDomainDefPtr def, + qemuDomainLogContextPtr logCtxt) +{ + int ret = 0; + + if (def->tpm) + ret = qemuExtTPMStart(driver, def, logCtxt); Why a separate qemuExtTPMStart?

This is mostly driven by the naming of the functions that go from higher level 'ExtDevices' to lower level 'ExtTPM'. I am not saying that other devices will appear, but for the Foo device we would then call 'ExtFoo'. Just 'pattern'....

Why not:

if (@def->tpm) return 0;

<guts of qemuExtTPMStart>

...

+ + return ret; +} + +void +qemuExtDevicesStop(virQEMUDriverPtr driver, + virDomainDefPtr def) +{ + if (def->tpm) + qemuExtTPMStop(driver, def); Similar... don't see the need for the extra indirection.

In fact the whole module only really cares about TPM and less about any sort of external device. Could be qemu_tpm.c and qemuTPM* API's too.

...

+} diff --git a/src/qemu/qemu_extdevice.h b/src/qemu/qemu_extdevice.h new file mode 100644 index 0000000..0bc7735 --- /dev/null +++ b/src/qemu/qemu_extdevice.h @@ -0,0 +1,44 @@ +/* + * qemu_extdevice.h: QEMU external devices support + * + * Copyright (C) 2014, 2018 IBM Corporation + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Author: Stefan Berger <stefanb@linux.vnet.ibm.com> + */ +#ifndef __QEMU_EXTDEVICE_H__ +# define __QEMU_EXTDEVICE_H__ + +# include "qemu_conf.h" +# include "qemu_domain.h" + +int qemuExtDevicesInitPaths(virQEMUDriverPtr driver, + virDomainDefPtr def) + ATTRIBUTE_RETURN_CHECK; + +int qemuExtDevicesPrepareHost(virQEMUDriverPtr driver, + virDomainDefPtr def) + ATTRIBUTE_RETURN_CHECK; + +int qemuExtDevicesStart(virQEMUDriverPtr driver, + virDomainDefPtr def, + qemuDomainLogContextPtr logCtxt) + ATTRIBUTE_RETURN_CHECK; + +void qemuExtDevicesStop(virQEMUDriverPtr driver, virDomainDefPtr def); + +#endif /* __QEMU_EXTDEVICE_H__ */ + diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 1afb71f..7bf90a4 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -47,6 +47,7 @@ #include "qemu_migration.h" #include "qemu_interface.h" #include "qemu_security.h" +#include "qemu_extdevice.h"

#include "cpu/cpu.h" #include "datatypes.h" @@ -5869,6 +5870,10 @@ qemuProcessPrepareHost(virQEMUDriverPtr driver, if (qemuProcessPrepareHostStorage(driver, vm, flags) < 0) goto cleanup;

+ VIR_DEBUG("Preparing external devices"); + if (qemuExtDevicesPrepareHost(driver, vm->def) < 0) + goto cleanup; + ret = 0; cleanup: virObjectUnref(cfg); @@ -5952,6 +5957,9 @@ qemuProcessLaunch(virConnectPtr conn, goto cleanup; logfile = qemuDomainLogContextGetWriteFD(logCtxt);

+ if (qemuExtDevicesStart(driver, vm->def, logCtxt) < 0) + goto cleanup; + VIR_DEBUG("Building emulator command line"); if (!(cmd = qemuBuildCommandLine(driver, qemuDomainLogContextGetManager(logCtxt), @@ -6191,6 +6199,8 @@ qemuProcessLaunch(virConnectPtr conn, ret = 0;

cleanup: + if (ret) + qemuExtDevicesStop(driver, vm->def); qemuDomainSecretDestroy(vm); virCommandFree(cmd); virObjectUnref(logCtxt); @@ -6557,6 +6567,8 @@ void qemuProcessStop(virQEMUDriverPtr driver, /* Clear network bandwidth */ virDomainClearNetBandwidth(vm);

+ qemuExtDevicesStop(driver, vm->def); + virDomainConfVMNWFilterTeardown(vm);

if (cfg->macFilter) { diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in index 688e5b9..03bef74 100644 --- a/src/qemu/test_libvirtd_qemu.aug.in +++ b/src/qemu/test_libvirtd_qemu.aug.in @@ -100,3 +100,4 @@ module Test_libvirtd_qemu = { "1" = "mount" } } { "memory_backing_dir" = "/var/lib/libvirt/qemu/ram" } +{ "swtpm_user" = "tss" }

security changes should be their own patch after we have a TPM_TYPE_EMULATOR

Yes, split that off as well.

...

diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 663c8c9..351f6f4 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -1372,6 +1372,11 @@ virSecurityDACSetTPMFileLabel(virSecurityManagerPtr mgr, &tpm->data.passthrough.source, false); break; + case VIR_DOMAIN_TPM_TYPE_EMULATOR: + ret = virSecurityDACSetChardevLabel(mgr, def, + &tpm->data.emulator.source, + false); + break; case VIR_DOMAIN_TPM_TYPE_LAST: break; } @@ -1393,6 +1398,7 @@ virSecurityDACRestoreTPMFileLabel(virSecurityManagerPtr mgr, &tpm->data.passthrough.source, false); break; + case VIR_DOMAIN_TPM_TYPE_EMULATOR: case VIR_DOMAIN_TPM_TYPE_LAST: break; } diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index c26cdac..17bc07a 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1472,6 +1472,12 @@ virSecuritySELinuxSetTPMFileLabel(virSecurityManagerPtr mgr, return -1; } break; + case VIR_DOMAIN_TPM_TYPE_EMULATOR: + tpmdev = tpm->data.emulator.source.data.nix.path; + rc = virSecuritySELinuxSetFilecon(mgr, tpmdev, seclabel->imagelabel); + if (rc < 0) + return -1; + break; case VIR_DOMAIN_TPM_TYPE_LAST: break; } @@ -1505,6 +1511,7 @@ virSecuritySELinuxRestoreTPMFileLabelInt(virSecurityManagerPtr mgr, VIR_FREE(cancel_path); } break; + case VIR_DOMAIN_TPM_TYPE_EMULATOR: case VIR_DOMAIN_TPM_TYPE_LAST: break; } The following is definitely separable into its own patch "util: Introduce ..."

Also did that.

...

diff --git a/src/util/virfile.c b/src/util/virfile.c index 5e9bd20..aaedb7a 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -38,6 +38,7 @@ #include <unistd.h> #include <dirent.h> #include <dirname.h> +#include <ftw.h> #if defined HAVE_MNTENT_H && defined HAVE_GETMNTENT_R # include <mntent.h> #endif @@ -2933,6 +2934,54 @@ void virDirClose(DIR **dirp) *dirp = NULL; }

+/* + * virDirChownFiles: + * @name: name of the directory + * @uid: uid + * @gid: gid + * + * Change ownership of all regular files in a directory. + * + * Returns -1 on error, with error already reported, 0 on success. + */ +int virDirChownFiles(const char *name, uid_t uid, gid_t gid) virFileChownFiles ?

Changed.

...

+{ + struct dirent *ent; + int ret; + DIR *dir; + char *path; + + if (virDirOpen(&dir, name) < 0) + return -1; Hope that @name isn't a file path on an NFS volume - there's some other

That would depend on the configuration of the host

consumers of chown in this module that you may want to have a look at.

The other consumers stat the file before and check whether chown() is necessary. Hm, is that the recommended way. I just try to change all of them without looking at the state.

...

+ + while ((ret = virDirRead(dir, &ent, name)) > 0) { + if (ent->d_type != DT_REG) + continue; + + if (virAsprintf(&path, "%s/%s", name, ent->d_name) < 0) { + ret = -1; + break; + } + if (chown(path, uid, gid) < 0) { If uid/gid change from non root user X to user Y, then wouldn't this just fail anyway?

Why would it fail if root does that? libvirtd runs as root.

Still using the -1, -1 logic like other chown/chmod consumers may actually be a benefit here at least w/r/t not failing.

Not sure what you mean. uid = -1 passed to chown() means, don't change it. Same for gid = -1.

...

+ ret = -1; + virReportSystemError(errno, + _("cannot chown '%s' to (%u, %u)"), + ent->d_name, (unsigned int) uid, + (unsigned int) gid); + } + VIR_FREE(path); + if (ret < 0) + break; + } + + virDirClose(&dir); + + if (ret < 0) + return -1; + + return 0; +} + static int virFileMakePathHelper(char *path, mode_t mode) { @@ -3031,6 +3080,17 @@ virFileMakeParentPath(const char *path) return ret; }

+static int +_virFileDeletePathCB(const char *fpath, const struct stat *sb ATTRIBUTE_UNUSED, + int typeflag ATTRIBUTE_UNUSED, struct FTW *ftwbuf ATTRIBUTE_UNUSED) +{ + return remove(fpath);> +} + +int virFileDeletePath(const char *path) +{ + return nftw(path, _virFileDeletePathCB, 64, FTW_DEPTH | FTW_PHYS); +} We could use virFileDeleteTree instead.... Using virFileRemove instead of remove... Be wary of adding some new call or something specific to Linux as we have non-Linux build variants...

I removed that today actually.

...

/* Build up a fully qualified path for a config file to be * associated with a persistent guest or network */ diff --git a/src/util/virfile.h b/src/util/virfile.h index cd2a386..5cc2299 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -253,11 +253,13 @@ int virDirRead(DIR *dirp, struct dirent **ent, const char *dirname) void virDirClose(DIR **dirp) ATTRIBUTE_NONNULL(1); # define VIR_DIR_CLOSE(dir) virDirClose(&(dir)) +int virDirChownFiles(const char *name, uid_t uid, gid_t gid);

int virFileMakePath(const char *path) ATTRIBUTE_RETURN_CHECK; int virFileMakePathWithMode(const char *path, mode_t mode) ATTRIBUTE_RETURN_CHECK; int virFileMakeParentPath(const char *path) ATTRIBUTE_RETURN_CHECK; +int virFileDeletePath(const char *path) ATTRIBUTE_RETURN_CHECK;

char *virFileBuildPath(const char *dir, const char *name,

Separable I would think

Yes, comes towards the end of the split of this patch.

...

diff --git a/src/util/virtpm.c b/src/util/virtpm.c index d5c10da..649153e 100644 --- a/src/util/virtpm.c +++ b/src/util/virtpm.c @@ -1,7 +1,7 @@ /* * virtpm.c: TPM support * - * Copyright (C) 2013 IBM Corporation + * Copyright (C) 2013,2018 IBM Corporation * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -22,16 +22,36 @@

#include <config.h>

+#include <sys/types.h> #include <sys/stat.h> +#include <unistd.h> +#include <fcntl.h> +#include <cap-ng.h>

+#include "conf/domain_conf.h" +#include "viralloc.h" +#include "vircommand.h" #include "virstring.h" #include "virerror.h" #include "viralloc.h" #include "virfile.h" +#include "virkmod.h" +#include "virlog.h" #include "virtpm.h" +#include "virutil.h" +#include "configmake.h"

#define VIR_FROM_THIS VIR_FROM_NONE

+VIR_LOG_INIT("util.tpm") + +/* + * executables for the swtpm; to be found on the host + */ +static char *swtpm_path; +static char *swtpm_setup; +static char *swtpm_ioctl; + /** * virTPMCreateCancelPath: * @devpath: Path to the TPM device @@ -74,3 +94,474 @@ virTPMCreateCancelPath(const char *devpath) cleanup: return path; } + +/* + * virTPMEmulatorInit + * + * Initialize the Emulator functions by searching for necessary + * executables that we will use to start and setup the swtpm + */ +static int +virTPMEmulatorInit(void) +{ + if (!swtpm_path) { + swtpm_path = virFindFileInPath("swtpm"); + if (!swtpm_path) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Could not find swtpm 'swtpm' in PATH")); + return -1; + } + if (!virFileIsExecutable(swtpm_path)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("TPM emulator %s is not an executable"), + swtpm_path); + VIR_FREE(swtpm_path); + return -1; + } + } + + if (!swtpm_setup) { + swtpm_setup = virFindFileInPath("swtpm_setup"); + if (!swtpm_setup) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Could not find 'swtpm_setup' in PATH")); + return -1; + } + if (!virFileIsExecutable(swtpm_setup)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("'%s' is not an executable"), + swtpm_setup); + VIR_FREE(swtpm_setup); + return -1; + } + } + + if (!swtpm_ioctl) { + swtpm_ioctl = virFindFileInPath("swtpm_ioctl"); + if (!swtpm_ioctl) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Could not find swtpm_ioctl in PATH")); + return -1; + } + if (!virFileIsExecutable(swtpm_ioctl)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("swtpm_ioctl program %s is not an executable"), + swtpm_ioctl); + VIR_FREE(swtpm_ioctl); + return -1; + } + } + + return 0; +} + +/* + * virTPMCreateEmulatorStoragePath + * + * @swtpmStorageDir: directory for swtpm persistent state + * @vmname: The name of the VM for which to create the storage + * + * Create the swtpm's storage path + */ +static char * +virTPMCreateEmulatorStoragePath(const char *swtpmStorageDir, + const char *vmname) +{ + char *path = NULL; + + ignore_value(virAsprintf(&path, "%s/%s/tpm1.2", swtpmStorageDir, vmname)); + + return path; +} + +/* + * virtTPMGetTPMStorageDir: + * + * @storagepath: directory for swtpm's pesistent state + * + * Derive the 'TPMStorageDir' from the storagepath by searching + * for the last '/'. + */ +static char * +virTPMGetTPMStorageDir(const char *storagepath) +{ + const char *tail = strrchr(storagepath, '/'); similar to virFileRemoveLastComponent - although that would clear out the '/'

...

+ char *path = NULL; + + if (!tail) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not get tail of storagedir %s"), + storagepath); + return NULL; + } + ignore_value(VIR_STRNDUP(path, storagepath, tail - storagepath)); + + return path; +} + +/* + * virTPMEmulatorInitStorage + * + * Initialize the TPM Emulator storage by creating its root directory, + * which is typically found in /var/lib/libvirt/tpm. + * + */ +static int +virTPMEmulatorInitStorage(const char *swtpmStorageDir) +{ + int rc = 0; + + /* allow others to cd into this dir */ + if (virFileMakePathWithMode(swtpmStorageDir, 0711) < 0) { + virReportSystemError(errno, + _("Could not create TPM directory %s"), + swtpmStorageDir); + rc = -1; + } + + return rc; +} + +/* + * virTPMCreateEmulatorStorage + * + * @storagepath: directory for swtpm's pesistent state + * @vmname: The name of the VM + * @created: a pointer to a bool that will be set to true if the + * storage was created because it did not exist yet + * @userid: The userid that needs to be able to access the directory + * + * Unless the storage path for the swtpm for the given VM + * already exists, create it and make it accessible for the given userid. + * Adapt ownership of the directory and all swtpm's state files there. + */ +static int +virTPMCreateEmulatorStorage(const char *storagepath, + bool *created, + uid_t swtpm_user) +{ + int ret = -1; + char *swtpmStorageDir = virTPMGetTPMStorageDir(storagepath); + + if (!swtpmStorageDir) + return -1; + + if (virTPMEmulatorInitStorage(swtpmStorageDir) < 0) + return -1; + + *created = false; + + if (!virFileExists(storagepath)) + *created = true;> + + if (virDirCreate(storagepath, 0700, swtpm_user, swtpm_user, IIRC: We only checked if swtpm_user was a user - we did not check if it was also a gid... You cannot use the same value for both, can you?

You are right. I need a swtpm_group, eh ? On my system tss happens to be mapped to 59 for uid and gid.

Still may want to consider using the -1, -1 because it is handled via the File API's.

-1 uid and -1 gid passed to chown() would mean don't change it. One problem is that a user may edit /etc/libvirt/qemu.conf and change this line here from root to tss: swtpm_user = "tss" In that case all file ownerships have to be adjusted so that the swtpm can access the files. That's why I am always adjusting the ownerships before swtpm or the other tools are started.

...

+ VIR_DIR_CREATE_ALLOW_EXIST) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not create directory %s as uid %u"), + storagepath, swtpm_user); + goto cleanup; + } + + if (virDirChownFiles(storagepath, swtpm_user, swtpm_user) < 0) If we've created the directory properly then wouldn't this possibly change the mode on any existing file in the directory? We really shouldn't need to do this should we? This would essentially cause a failure to chown something we're not allowed to chown and I believe the DirCreate would have failed anyway since

See the explanation above with changing libvirtd's qemu.conf.

...

+ goto cleanup; + + ret = 0; + + cleanup: + VIR_FREE(swtpmStorageDir); + + return ret; +} + +void +virTPMDeleteEmulatorStorage(virDomainTPMDefPtr tpm) +{ + char *path = virTPMGetTPMStorageDir(tpm->data.emulator.storagepath); + if (path) { + ignore_value(virFileDeletePath(path)); + VIR_FREE(path); + } +} + +/* + * virTPMCreateEmulatorSocket: + * + * @swtpmStateDir: the directory where to create the socket in + * + * Create the vTPM device name from the given parameters + */ +static char * +virTPMCreateEmulatorSocket(const char *swtpmStateDir, const char *vmname) +{ + char *path = NULL; + + ignore_value(virAsprintf(&path, "%s/%s-swtpm.sock", swtpmStateDir, + vmname)); See virDomainDefGetShortName and it's consumers. Don't be stuck in the quagmire that caused nightmares for mkletzan... In fact - anywhere that I may have already missed or will miss subsequently that uses vmname should use the short name.

Oh good, I didn't know about this API call. All occurrences of def->name replaced...

...

+ + return path; +} + +/* + * virTPMEmulatorInitPaths: + * + * @tpm: TPM definition for an emulator type + * @swtpmStorageDir: the general swtpm storage dir which is used as a base + * directory for creating VM specific directories + * @vmname: the name of the VM + */ +int virTPMEmulatorInitPaths(virDomainTPMDefPtr tpm, + const char *swtpmStorageDir, + const char *vmname) +{ + if (!tpm->data.emulator.storagepath && + !(tpm->data.emulator.storagepath = + virTPMCreateEmulatorStoragePath(swtpmStorageDir, vmname))) + return -1; + + return 0; +} + +/* + * virTPMEmulatorPrepareHost: + * + * @tpm: tpm definition + * @logDir: directory where swtpm writes its logs into + * @vmname: name of the VM + * @swtpm_user: uid to run the swtpm with + * @swtpmStateDir: directory for swtpm's persistent state + * @qemu_user: uid that qemu will run with; we share the socket file with it + * + * Prepare the log directory for the swtpm and adjust ownership of it and the + * log file we will be using. Prepare the state directory where we will share + * the socket between tss and qemu users. + */ +int virTPMEmulatorPrepareHost(virDomainTPMDefPtr tpm, + const char *logDir, const char *vmname, + uid_t swtpm_user, const char *swtpmStateDir, + uid_t qemu_user) +{ + int ret = -1; + + if (virTPMEmulatorInit() < 0) + return -1; + + /* create log dir ... */ + if (virFileMakePathWithMode(logDir, 0771) < 0) Anyone in the same group (that hasn't been defined yet) can write? Or should they just Read, Execute?

For sure reading and changing into that directory. Maybe not writing.

And the world can Read?

I am changing this to 0730.

...

+ goto cleanup; + + /* ... and adjust ownership */ + if (virDirCreate(logDir, 0771, swtpm_user, swtpm_user, + VIR_DIR_CREATE_ALLOW_EXIST) < 0) + goto cleanup; + + /* create logfile name ... */ + if (virAsprintf(&tpm->data.emulator.logfile, "%s/%s-swtpm.log", + logDir, vmname) < 0) + goto cleanup; + + /* ... and make sure it can be accessed by swtpm_user */ + if (virFileExists(tpm->data.emulator.logfile) && + chown(tpm->data.emulator.logfile, swtpm_user, swtpm_user) < 0) { + virReportSystemError(errno, + _("Could not chown on swtpm logfile %s"), + tpm->data.emulator.logfile); + goto cleanup; + } + + /* create our swtpm state dir ... */ + if (virDirCreate(swtpmStateDir, 0771, qemu_user, swtpm_user, + VIR_DIR_CREATE_ALLOW_EXIST) < 0) + goto cleanup; + + /* create the socket filename */ + if (!(tpm->data.emulator.source.data.nix.path = + virTPMCreateEmulatorSocket(swtpmStateDir, vmname))) + goto cleanup; + tpm->data.emulator.source.type = VIR_DOMAIN_CHR_TYPE_UNIX; + + ret = 0; + + cleanup: + if (ret) + VIR_FREE(tpm->data.emulator.logfile); + + return ret; +} + +/* + * virTPMEmulatorRunSetup + * + * @storagepath: path to the directory for TPM state + * @vmname: the name of the VM + * @vmuuid: the UUID of the VM + * @swtpm_user: The userid to switch to when setting up the TPM; + * typically this should be the uid of 'tss' or 'root' + * @logfile: The file to write the log into; it must be writable + * for the user given by userid or 'tss' + * + * Setup the external swtpm + */ +static int +virTPMEmulatorRunSetup(const char *storagepath, const char *vmname, + const unsigned char *vmuuid, + uid_t swtpm_user, const char *logfile) +{ + virCommandPtr cmd = NULL; + int exitstatus; + int rc = 0; + char uuid[VIR_UUID_STRING_BUFLEN]; + char *vmid = NULL; + + cmd = virCommandNew(swtpm_setup); + if (!cmd) { + rc = -1; + goto cleanup; + } + + virUUIDFormat(vmuuid, uuid); + if (virAsprintf(&vmid, "%s:%s", vmname, uuid) < 0) + goto cleanup; Again, may want to consider a short name...

...

+ + virCommandSetUID(cmd, swtpm_user); + virCommandSetGID(cmd, swtpm_user); + + virCommandAddArgList(cmd, + "--tpm-state", storagepath, + "--vmid", vmid, + "--logfile", logfile, + "--createek", + "--create-ek-cert", + "--create-platform-cert", + "--lock-nvram", + "--not-overwrite", + NULL); + + virCommandClearCaps(cmd); + + if (virCommandRun(cmd, &exitstatus) < 0 || exitstatus != 0) { + char *buffer = NULL; + ignore_value(virFileReadAllQuiet(logfile, 10240, &buffer)); + + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not run '%s'. exitstatus: %d;\n" + "%s"), + swtpm_setup, exitstatus, buffer); + VIR_FREE(buffer); + rc = -1; + } + + cleanup: + VIR_FREE(vmid); + virCommandFree(cmd); + + return rc; +} + +/* + * virTPMEmulatorBuildCommand: + * + * @tpm: TPM definition + * @vmname: The name of the VM + * @vmuuid: The UUID of the VM + * @swtpm_user: The uid for the swtpm to run as (drop privileges to from root) + * + * Create the virCommand use for starting the emulator + * Do some initializations on the way, such as creation of storage + * and emulator setup. + */ +virCommandPtr +virTPMEmulatorBuildCommand(virDomainTPMDefPtr tpm, const char *vmname, + const unsigned char *vmuuid, uid_t swtpm_user) +{ + virCommandPtr cmd = NULL; + bool created = false; + + if (virTPMCreateEmulatorStorage(tpm->data.emulator.storagepath, + &created, swtpm_user) < 0) + return NULL; + + if (created && + virTPMEmulatorRunSetup(tpm->data.emulator.storagepath, vmname, vmuuid, + swtpm_user, tpm->data.emulator.logfile) < 0) + goto error; + + unlink(tpm->data.emulator.source.data.nix.path); + + cmd = virCommandNew(swtpm_path); + if (!cmd) + goto error; + + virCommandClearCaps(cmd); + + virCommandAddArgList(cmd, "socket", "--daemon", "--ctrl", NULL); + virCommandAddArgFormat(cmd, "type=unixio,path=%s,mode=0660", + tpm->data.emulator.source.data.nix.path); So this makes sense 660 to allow RW for owner/group...

...

+ + virCommandAddArg(cmd, "--tpmstate"); + virCommandAddArgFormat(cmd, "dir=%s,mode=0640", + tpm->data.emulator.storagepath); And this differs w/ mode, doesn't it?

Will do 0600 here.

Although I'll admit - I've lost a bit more context now...

...

+ + virCommandAddArg(cmd, "--log"); + virCommandAddArgFormat(cmd, "file=%s", tpm->data.emulator.logfile); + + virCommandSetUID(cmd, swtpm_user); + virCommandSetGID(cmd, swtpm_user); But user != gid

Yes, there will be a swtpm_group variable.

...

+ + return cmd; + + error: + if (created) + virTPMDeleteEmulatorStorage(tpm); + + VIR_FREE(tpm->data.emulator.source.data.nix.path); + VIR_FREE(tpm->data.emulator.storagepath); ^^^ This would seem to need to be some sort of generic TPM.*Free type function...

...

+ + virCommandFree(cmd); + + return NULL; +} + +/* + * virTPMEmulatorStop + * @swtpmStateDir: A directory where the socket is located + * @vmname: name of the VM + * + * Gracefully stop the swptm + */ +void +virTPMEmulatorStop(const char *swtpmStateDir, const char *vmname) +{ + virCommandPtr cmd; If you set this to NULL, then you can move the virCommandFree after cleanup: (doesn't really matter, just an option).

...

+ char *pathname; + char *errbuf = NULL; + + if (virTPMEmulatorInit() < 0) + return; + + if (!(pathname = virTPMCreateEmulatorSocket(swtpmStateDir, vmname))) + return; + + if (!virFileExists(pathname)) + goto cleanup; + + cmd = virCommandNew(swtpm_ioctl); + if (!cmd) { + VIR_FREE(pathname); + return; goto cleanup..

right.

...

+ } + + virCommandAddArgList(cmd, "--unix", pathname, "-s", NULL); + + virCommandSetErrorBuffer(cmd, &errbuf); + + ignore_value(virCommandRun(cmd, NULL)); + + virCommandFree(cmd); + + /* clean up the socket */ + unlink(pathname); + + cleanup: + VIR_FREE(pathname); + VIR_FREE(errbuf); +} diff --git a/src/util/virtpm.h b/src/util/virtpm.h index b21fc05..8afd606 100644 --- a/src/util/virtpm.h +++ b/src/util/virtpm.h @@ -1,7 +1,7 @@ /* * virtpm.h: TPM support * - * Copyright (C) 2013 IBM Corporation + * Copyright (C) 2013,2018 IBM Corporation * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -22,6 +22,29 @@ #ifndef __VIR_TPM_H__ # define __VIR_TPM_H__

+# include "vircommand.h" + +typedef struct _virDomainTPMDef virDomainTPMDef; +typedef virDomainTPMDef *virDomainTPMDefPtr; + char *virTPMCreateCancelPath(const char *devpath) ATTRIBUTE_NOINLINE;

+int virTPMEmulatorInitPaths(virDomainTPMDefPtr tpm, + const char *swtpmStorageDir, + const char *vmname) + ATTRIBUTE_RETURN_CHECK; +int virTPMEmulatorPrepareHost(virDomainTPMDefPtr tpm, + const char *logDir, const char *vmname, + uid_t swtpm_user, const char *swtpmStateDir, + uid_t qemu_user) + ATTRIBUTE_RETURN_CHECK; +virCommandPtr virTPMEmulatorBuildCommand(virDomainTPMDefPtr tpm, + const char *vmname, + const unsigned char *vmuuid, + uid_t swtpm_user) + ATTRIBUTE_RETURN_CHECK; +void virTPMEmulatorStop(const char *swtpmStateDir, + const char *vmname); +void virTPMDeleteEmulatorStorage(virDomainTPMDefPtr tpm); + #endif /* __VIR_TPM_H__ */

I'm sure I missed a few things - this was all done by sight...

Quite a few patches could be generated out of this single patch I would think.. It would allow at least you to make some progress to get some ACK's/R-b's and possibly pushes rather than continually looking at the same things.

I broke this patch up into several patches. Thanks for looking at it. Stefan

John

On 04/26/2018 08:07 PM, John Ferlan wrote:

[...]

...

...

...

Two blank lines between new functions and we like Free instead of Delete unless of course this is something more specific... This is something more specifc. The TPM emulator writes state into files in a dedicated directory. That state would normally be written into NVRAM of a TPM so that, after it is powered up again, it has that old state again (keys etc.) . We only delete this state and the per-VM directory tree when the domain is undefined.

Probably just an API naming thing as I was reading forward... Lot of context to plow through on the first pass for me!

...

...

[...]

...

...

...

@@ -27548,6 +27587,10 @@ virDomainDeleteConfig(const char *configDir, goto cleanup; } + /* in case domain is NOT running, remove any TPM storage */ + if (!dom->persistent) Well this has less to do with running and more to do with persistence vs. transient Iirc this comes from the following: I ran into this case where I had a running domain and undefined it. The domain keeps running of course, the XML definition is gone. In this case we cannot just delete the TPM emulator's storage while it is running but we have to do it later once that VM terminates.

I wonder if this is something where using the /var/run/libvirt (or whatever the temporary running domain path is - my brain is fried from so many reviews)... Using this would cause/allow cleanup when the process eventually dies

For this persistent state I am now using /var/lib/libvirt/swtpm; it seems better than /var/run/...

...

...

...

+ virDomainTPMDelete(dom->def); + [...]

...

...

...

virQEMUDriverConfigNew(bool privileged) &cfg->nfirmwares) < 0) goto error; + if (virGetUserID("tss", &cfg->swtpm_user) < 0) + cfg->swtpm_user = 0; /* root */ + Something doesn't feel right here especially for unprivileged mode. I haven't tried it in unprivileged mode. Not sure how to invoke it even...

-c qemu:///session

Ok, thanks. I tried that now and fixed a few things. swtpm_setup needs high privileges to run (or be run as 'tss' user), so it won't run when in session mode. What is also a bit of a concern is the SELinux svirt support. I have the following rules I need to apply with the 3rd one now to support session mode, the 4th one added for FC27 (previously only run on FC23): allow svirt_t virtd_t:fifo_file write; allow svirt_t virtd_t:process sigchld; allow svirt_t user_tmp_t:sock_file { create setattr }; allow svirt_t swtpm_exec_t:file entrypoint; https://github.com/stefanberger/swtpm/blob/tpm2-preview.v2/src/selinux/swtpm...

...

...

If there isn't a "tss" user, then are things configured correctly? There are only two choice, one is tss and the other one is root.

...

Should we follow the other callers to virGetUserID and set to -1 instead? Set swtpm_user = -1; ? What happens when we need to actually use this variable then ? Report an error ?

When using the virFile* API's if the incoming uid/gid is -1, then it's not specifically and it IIRC it takes whatever the current default running mode is. If it is set, then it's set specifically to the value. Been a while since I traversed through all that code.

-1 seems to mean to not change anything. I initialize swtpm_user and swtpm_group now to -1 when in session mode. It works.

...

...

Still 0 is the default for @cfg members anyway.

[...]

...

...

...

@@ -7510,6 +7514,9 @@ qemuDomainUndefineFlags(virDomainPtr dom, if (!(vm = qemuDomObjFromDomain(dom))) return -1; + if (qemuExtDevicesInitPaths(driver, vm->def) < 0) + return -1; Again, confused over failure in an undefine path? Or is this just git diff fooling me? This is for a domain that's being undefined but was never started (which would have caused the path initialization) since libvirtd was restarted. Again, we need to initialize the paths. If there was a better place to put these paths, I'd be curious where that is.

Perhaps "stateDir" - it just came to me - hopefully it's what I was thinking about ;-)

[...]

...

...

...

+{ + struct dirent *ent; + int ret; + DIR *dir; + char *path; + + if (virDirOpen(&dir, name) < 0) + return -1; Hope that @name isn't a file path on an NFS volume - there's some other That would depend on the configuration of the host

...

consumers of chown in this module that you may want to have a look at. The other consumers stat the file before and check whether chown() is necessary. Hm, is that the recommended way. I just try to change all of them without looking at the state.

The NFS thing is something that came to mind while thinking of uid/gid mgmt and rootsquash mode (or something like that) which absolutely causes nightmares w/r/t using root...

Hm, not sure what to do about this. Document it? Prevent root user from being used ?

...

...

...

+ + while ((ret = virDirRead(dir, &ent, name)) > 0) { + if (ent->d_type != DT_REG) + continue; + + if (virAsprintf(&path, "%s/%s", name, ent->d_name) < 0) { + ret = -1; + break; + } + if (chown(path, uid, gid) < 0) { If uid/gid change from non root user X to user Y, then wouldn't this just fail anyway? Why would it fail if root does that? libvirtd runs as root.

Guess I was thinking of session mode or unprivileged mode.

Right, hadn't used that before. Makes sense now.

...

...

Still using the -1, -1 logic like other chown/chmod consumers may actually be a benefit here at least w/r/t not failing. Not sure what you mean. uid = -1 passed to chown() means, don't change it. Same for gid = -1.

I think I redescribed that a bit above. It wasn't fresh in my mind, but essentially looking at the various virFile API's that would check uid/gid vs. -1 and thinking how qemu.conf and storage pool <permissions> used -1, -1... Again the details escape me at the moment.

[...]

...

...

...

+static int +virTPMCreateEmulatorStorage(const char *storagepath, + bool *created, + uid_t swtpm_user) +{ + int ret = -1; + char *swtpmStorageDir = virTPMGetTPMStorageDir(storagepath); + + if (!swtpmStorageDir) + return -1; + + if (virTPMEmulatorInitStorage(swtpmStorageDir) < 0) + return -1; + + *created = false; + + if (!virFileExists(storagepath)) + *created = true;> + + if (virDirCreate(storagepath, 0700, swtpm_user, swtpm_user, IIRC: We only checked if swtpm_user was a user - we did not check if it was also a gid... You cannot use the same value for both, can you? You are right. I need a swtpm_group, eh ? On my system tss happens to be mapped to 59 for uid and gid.

...

Still may want to consider using the -1, -1 because it is handled via the File API's. -1 uid and -1 gid passed to chown() would mean don't change it. One problem is that a user may edit /etc/libvirt/qemu.conf and change this line here from root to tss:

swtpm_user = "tss"

In that case all file ownerships have to be adjusted so that the swtpm can access the files. That's why I am always adjusting the ownerships before swtpm or the other tools are started.

I suspect by this time you've dug in a bit more and perhaps have more details than I can recall at the moment.

there's s swtpm_group = "tss" now as well.

[...]

...

I broke this patch up into several patches. Thanks for looking at it.

Stefan Thanks... I know it's painful from experience ;-)

Not so bad when one can cut and paste whole files form a patch into a new patch.

John

Stefan