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.

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

swtpm_setup.c: add "socket" argument to custom swtpm call #957

Conversation

mikkorapeli-linaro
Copy link

When a custom swtpm binary is provided with --tpm path/to/swtpm, then swtpm_setup is currently not adding "swtpm socket" argument when calling swtpm --print-capabilities which results in swtpm_setup always failing. --tpm documentation does not mention that "socket" argument should be provided so add it automatically. As workaround, --tpm "path/to/swtpm socket" works but it should be the default just like when swtpm is found from PATH.

Fixes use of swtpm in yocto cross compile environment where paths to swtpm and swtpm_setup binaries provided by swtpm-native recipe change frequently due to
recipe specific sysroot environment.

When a custom swtpm binary is provided with --tpm path/to/swtpm,
then swtpm_setup is currently not adding "swtpm socket" argument
when calling swtpm --print-capabilities which results in
swtpm_setup always failing. --tpm documentation does not mention
that "socket" argument should be provided so add it automatically.
As workaround, --tpm "path/to/swtpm socket" works but it should
be the default just like when swtpm is found from PATH.

Fixes use of swtpm in yocto cross compile environment
where paths to swtpm and swtpm_setup binaries provided
by swtpm-native recipe change frequently due to
recipe specific sysroot environment.

Signed-off-by: Mikko Rapeli <mikko.rapeli@linaro.org>
@@ -1472,6 +1472,11 @@ int main(int argc, char *argv[])
case 'T': /* --tpm */
g_free(swtpm_prg);
swtpm_prg = g_strdup(optarg);
if (swtpm_prg) {
tmp = g_strconcat(swtpm_prg, " socket", NULL);
g_free(swtpm_prg);
Copy link
Owner

Choose a reason for hiding this comment

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

This should not be necessary. Test cases already pass it like this using quotation marks around the parameters "swtpm socket" :

                             "./${libtpmsdir}/swtpm_setup" \
                                --tpm2 \
                                --tpmstate "${statedir}" \
                                --config "${workdir}/swtpm_setup.conf" \
                                --log "${statedir}/swtpm_setup.log" \
                                --tpm "${workdir}/${libtpmsdir}/swtpm socket" \
                                ${profileopt:+${profileopt}}

Copy link
Author

Choose a reason for hiding this comment

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

IMO this is wrong. And caused me to spend a day figuring out that "path/to/swtpm socket" was the correct way to define custom swtpm path.

With swtpm is found from PATH, then " socket" is added. IMO it should be added with custom swtpm path too.

Copy link
Owner

Choose a reason for hiding this comment

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

With swtpm is found from PATH, then " socket" is added. IMO it should be added with custom swtpm path too.

I think running swtpm_setup with the --tpm parameter is a special case and in this case you should just provide the full path to the executable.

Copy link
Owner

Choose a reason for hiding this comment

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

... just run $(type -P myswtpm) on command line and it's done.

Copy link
Author

Choose a reason for hiding this comment

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

If the test is failing with this patch, then I can provide fxi for it. I presume something like:

--- a/tests/test_tpm2_libtpms_versions_profiles
+++ b/tests/test_tpm2_libtpms_versions_profiles
@@ -686,7 +686,7 @@ function swtpm_setup_create_profile_state()
                                --tpmstate "${statedir}" \
                                --config "${workdir}/swtpm_setup.conf" \
                                --log "${statedir}/swtpm_setup.log" \
-                               --tpm "${workdir}/${libtpmsdir}/swtpm socket" \
+                               --tpm "${workdir}/${libtpmsdir}/swtpm" \
                                ${profileopt:+${profileopt}}; then
                                echo "Error: Could not run swtpm_setup with libtpms from branch ${branch} with profile ${profile}"
                                err=1

Or is the "--tpm" argument used to provide other, non-swtpm TPM device paths too? I did not think this was possible.

Copy link
Author

Choose a reason for hiding this comment

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

Full path to "swtpm" with swtpm_setup --tpm argument was failing. The command line help for swtpm_setup only mentioned --tpm as path to "swtpm" binary, not "path to 'swtpm' binary and argument 'socket'".

Copy link
Author

Choose a reason for hiding this comment

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

... just run $(type -P myswtpm) on command line and it's done.

This does not work. There needs to be the "socket" argument too. Without it older versions of swtpm fail with "support for TPM2 not in swtpm binary".

Copy link
Owner

Choose a reason for hiding this comment

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

This works:

swtpm_setup \
  --tpm "$(type -P swtpm) socket" \
  --tpm2 \
  --overwrite \
  --tpm-state .

To maintain backwards compatibility we cannot make that change anyway.

Copy link
Author

Choose a reason for hiding this comment

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

This was not at all obvious. I can add this to the command line help but I would really prefer to add the " socket" argument automatically.

Backwards compatibility is important but IMO it should not cover design bugs.

So really just fix the documentation then?

Copy link
Owner

Choose a reason for hiding this comment

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

So really just fix the documentation then?

Yes: #958

@mikkorapeli-linaro
Copy link
Author

Fixed documentation instead in #958

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants