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

Supports automatic startup of tpm2-abrmd.service when TCM devices exist #848

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

chench246
Copy link

TCM(Trusted Cryptography Module) is a Chinese standard and is compatible with TPM.

TCM(Trusted Cryptography Module) is a Chinese standard and is compatible with TPM

Signed-off-by: chench246 <chench246@hotmail.com>
@chench246
Copy link
Author

@williamcroberts

Requires=dev-tpm0.device
After=dev-tpm0.device dev-tcm0.device
ConditionPathExists=|/dev/tpm0
ConditionPathExists=|/dev/tcm0
Copy link
Member

Choose a reason for hiding this comment

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

Let me get an adult to look at this, I just pinged Lennart.

Choose a reason for hiding this comment

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

Uh, no. This is wrong. After= and Requires= are orthogonal on systemd. That means that one will not imply the other, one is purely about ordering, and the other about pulling it into the transaction. But this means that dev-tpm0.device is never pulled into the initial transaction anymore after this PR (unless soemthing else happens to do that, but you cannot rely on that).

Hence this is not right. You drop the synchronization entirely here in the worst case, which doesn#t seem to be wat you want.

Choose a reason for hiding this comment

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

So, in systemd v256 we introduced a generic target "tpm2.target" that is supposed to be a generic milestone that indicates when TPM connectivity is available. Stuff that provides TPM functionality (such as those TPM supplicants on various arm hw) should be ordered before, as should /dev/tpm0 when it exists. Users of the /dev/tpm0 device shuld be ordered after it.

This new target is entirely generic, i.e. people can plug arbitrary stuff they want before it, and arbitrary stuff after it. Hence it appears to me that abrmd should order after that, and no longer directly after /dev/tpm0.

In systemd upstream there's systemd-tpm2-generator which does some checks against firmware to see if there potentially will be a tpm device showing up, and in that case it will pull in dev-tpm0.device from tpm2.target, so that the chain on regular tpm devices is complete again: dev-tpm2.device → tpm2.target → (any consumers of /dev/tpm0).

Hence I think the TCM stuff should integrate with that: i.e. provide your own generator that checks if /dev/tcm0 is likely to show up. If so, synthesize a dep from tpm2.target on dev-tcm0.device, and things should work correctly: people can build a single image that will do the right thing on tcm equipped and tpm equipped systems.

That said, if tcm and tpm are compatible, why even expose this under a different name? Is the tcm stuff merged in the upstream kernel?

Choose a reason for hiding this comment

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

or in other words, just replace

After=dev-tpm0.device
Requires=dev-tpm0.device

by a simple:

After=tpm2.target

(Requires= is not needed there as the aforementioned systemd-tpm2-generator already pulls tpm2.target into the boot wherever necessary)

That said, this is v256 stuff. I have no good answer how you could cover this nicely on older systemd... v256 is still pretty new I guess.

src/tabrmd-defaults.h Show resolved Hide resolved
@@ -16,7 +16,7 @@
#define TABRMD_ENTROPY_SRC_DEFAULT "/dev/urandom"
#define TABRMD_SESSIONS_MAX_DEFAULT 4
#define TABRMD_SESSIONS_MAX 64
#define TABRMD_TCTI_CONF_DEFAULT "device:/dev/tpm0"
#define TABRMD_TCTI_CONF_DEFAULT ((!access("/dev/tcm0", F_OK)) ? ("device:/dev/tcm0") : ("device:/dev/tpm0"))
Copy link
Member

Choose a reason for hiding this comment

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

I don't like this, while it does work, its prone for a race condition bug.

Is their ever a case where /dev/tpm0 and /dev/tcm0 even exist, couldn't this be handled with a ueventd link by the tcm service? Or perhaps could we pass an argument from TCM service to this service (env var), where it expands to the --tcti=device:/dev/tcm0 in the service file?

Copy link
Member

Choose a reason for hiding this comment

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

or perhaps make this an array of things and the service tries them both via TCTILdr interface?

Let's see if Lennart has any good ideas.

Copy link

@poettering poettering Nov 21, 2024

Choose a reason for hiding this comment

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

uh, hidden code in a macro like this sounds icky. and racy...

maybe extend the "device" backend driver take multiple arguments and then open the first which works? so that you could specify device:/dev/tpm0,/dev/tcm0 or so as default?

Copy link
Member

Choose a reason for hiding this comment

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

uh, hidden code in a macro like this sounds icky. and racy...

maybe extend the "device" backend driver take multiple arguments and then open the first which works? so that you could specify device:/dev/tpm0,/dev/tcm0 or so as default?

I like that idea, good suggestion. The only downside is paths with a , are not supported, but who cares :-p

Copy link
Author

Choose a reason for hiding this comment

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

#841
This PR seems to be implemented in this way. What is the current progress of this PR?

@williamcroberts williamcroberts added this to the next (3.0.1) milestone Nov 20, 2024
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.

3 participants