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

TCTI: Add USB TPM (LetsTrust-TPM2Go) TCTI module #2479

Merged

Conversation

wxleong
Copy link
Member

@wxleong wxleong commented Nov 12, 2022

Bringing up a TCTI module to support the LetsTrust-TPM2Go, a USB TPM from @PaulKissinger. It's a combined effort and resources are taken from:

The CI is expected to fail at this stage since the library libusb-1.0-0-dev is missing from the CI container image.

@AndreasFuchsTPM
Copy link
Member

Apparently, the build is still failing on Fedora-32 and Alpine-3.15.

This is weird, because of https://github.com/tpm2-software/tpm2-software-container/blob/f15f2ee3e4d69ac332c8acfaa8867d0546090cda/fedora-32.docker.m4#L62 and https://github.com/tpm2-software/tpm2-software-container/blob/f15f2ee3e4d69ac332c8acfaa8867d0546090cda/alpine-3.15.docker.m4#L55

I thought, the dockers were updated automatically?

@williamcroberts Do you have any idea ?

@williamcroberts
Copy link
Member

Apparently, the build is still failing on Fedora-32 and Alpine-3.15.

This is weird, because of https://github.com/tpm2-software/tpm2-software-container/blob/f15f2ee3e4d69ac332c8acfaa8867d0546090cda/fedora-32.docker.m4#L62 and https://github.com/tpm2-software/tpm2-software-container/blob/f15f2ee3e4d69ac332c8acfaa8867d0546090cda/alpine-3.15.docker.m4#L55

I thought, the dockers were updated automatically?

@williamcroberts Do you have any idea ?

Their's a rule in the build do only build/and publish for containers detected in the diff, but the diff action seems to have a bug and only compares the top level commit (the build output said this is the valid compare url):

@williamcroberts
Copy link
Member

@wxleong and @AndreasFuchsTPM I have updated all the containers, what a nightmare. I have no idea why their is a curl error popping up in the build now, here is the PR that fixes that:

So @wxleong you can cherry-pick that change into your branch or just rebase once it's merged.

@wxleong wxleong force-pushed the develop-tcti-spi-lt2go3 branch 6 times, most recently from 5ea4383 to 8e0ebd8 Compare December 21, 2022 07:00
@wxleong
Copy link
Member Author

wxleong commented Dec 21, 2022

@williamcroberts CI workflow has passed, but CIFuzz and CirrusCI workflows still encountered libusb-1.0 library missing error.

@williamcroberts
Copy link
Member

@williamcroberts CI workflow has passed, but CIFuzz and CirrusCI workflows still encountered libusb-1.0 library missing error.

@wxleong for
CI Fuzz, add it to the dockerfile here:

For CirrusCI add it to this file (Note it's FreeBSD):

@wxleong wxleong force-pushed the develop-tcti-spi-lt2go3 branch 4 times, most recently from 12823a5 to 4337fe1 Compare December 22, 2022 08:46
@wxleong
Copy link
Member Author

wxleong commented Dec 22, 2022

Last item to fix: CIFuzz, simple fix but have to sort out the Google Contributor License Agreement before I can do anything.

@PaulKissinger
Copy link
Contributor

PaulKissinger commented Dec 22, 2022 via email

@williamcroberts
Copy link
Member

Last item to fix: CIFuzz, simple fix but have to sort out the Google Contributor License Agreement before I can do anything.

I can do CI Fuzz

@williamcroberts williamcroberts marked this pull request as ready for review December 22, 2022 14:13
@williamcroberts
Copy link
Member

@wxleong:

  1. I sent the PR to oss-fuzz here: tpm2-tss: disable spi lt2go TCTI google/oss-fuzz#9264
  2. Can you rebase this on-top of current master and fix the conflicts and re-push
    a. git pull --rebase origin master
    b. fix merge conflicts
    c. commit
    d. git push -f wxleong develop-tcti-spi-lt2go3; # You may need to change the names to match what you have

Thanks,
Bill

@wxleong
Copy link
Member Author

wxleong commented Dec 23, 2022

@wxleong:

  1. I sent the PR to oss-fuzz here: tpm2-tss: disable spi lt2go TCTI google/oss-fuzz#9264
  2. Can you rebase this on-top of current master and fix the conflicts and re-push
    a. git pull --rebase origin master
    b. fix merge conflicts
    c. commit
    d. git push -f wxleong develop-tcti-spi-lt2go3; # You may need to change the names to match what you have

Thanks, Bill

@williamcroberts thanks for the help with oss-fuzz. The conflicts was already resolved prior to this. I just did a dummy force push and all are passing now.

@tobyp
Copy link

tobyp commented Dec 23, 2022

Awesome! I look forward to giving this a spin after Christmas.

Copy link
Member

@williamcroberts williamcroberts left a comment

Choose a reason for hiding this comment

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

I think you also want to rebase ontop of current master

.cirrus.yml Show resolved Hide resolved
configure.ac Outdated Show resolved Hide resolved
src/tss2-tcti/tcti-spi-lt2go.c Outdated Show resolved Hide resolved
src/tss2-tcti/tcti-spi-lt2go.c Show resolved Hide resolved
src/tss2-tcti/tcti-spi-lt2go.c Outdated Show resolved Hide resolved
@wxleong
Copy link
Member Author

wxleong commented Dec 27, 2022

I think you also want to rebase ontop of current master

merged master, 2b7007c

@williamcroberts williamcroberts force-pushed the develop-tcti-spi-lt2go3 branch 2 times, most recently from de82b7d to 020c7c3 Compare December 30, 2022 18:03
Copy link
Member

@williamcroberts williamcroberts left a comment

Choose a reason for hiding this comment

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

Please force reset you branch and amend the history, as you make a change related to the PR git commit --amend. If you need to update the branch ontop of master, git pull --rebase origin master is your friend as well.

Makefile-test.am Outdated Show resolved Hide resolved
Makefile-test.am Show resolved Hide resolved
@tobyp
Copy link

tobyp commented Jan 3, 2023

I was able to build this on ArchLinux, but it fails for me with "Probing TPM failed" because spi_tpm_helper_read_reg(TCTI_SPI_HELPER_TPM_DID_VID_REG) keeps reading 0. Logs with tcti+TRACE are here if you'd like to have a look.

@PaulKissinger have there been any changes to the hardware? I have the relatively old stick with serial number 000008 here.

@PaulKissinger
Copy link
Contributor

I was able to build this on ArchLinux, but it fails for me with "Probing TPM failed" because spi_tpm_helper_read_reg(TCTI_SPI_HELPER_TPM_DID_VID_REG) keeps reading 0. Logs with tcti+TRACE are here if you'd like to have a look.

@PaulKissinger have there been any changes to the hardware? I have the relatively old stick with serial number 000008 here.

Hi @tobyp,
It is possible that you have the wrong hardware.
May you send me a detailed picture of the area between the chips with parts of both chips?

Thanks for testing!

Paul

@tobyp
Copy link

tobyp commented Jan 3, 2023

I also see an error message in the kernel log for every tss command, like this:

[ 9695.096293] usb 1-1.3: usbfs: process 108052 (tpm2_startup) did not claim interface 0 before use

I made a fix commit here, feel free to squash it into your branch if you agree this is a good solution (I'm not really fit with USB, I'm not even sure what "claiming" an interface means, just that the error bothers me :P).

@williamcroberts
Copy link
Member

I also see an error message in the kernel log for every tss command, like this:

[ 9695.096293] usb 1-1.3: usbfs: process 108052 (tpm2_startup) did not claim interface 0 before use

I made a fix commit here, feel free to squash it into your branch if you agree this is a good solution (I'm not really fit with USB, I'm not even sure what "claiming" an interface means, just that the error bothers me :P).

I'm not fit either, but I don't see anything glairingly wrong... I'll let @wxleong weigh in and we can pick it in.

@tobyp
Copy link

tobyp commented Jan 3, 2023

Hi @tobyp, It is possible that you have the wrong hardware. May you send me a detailed picture of the area between the chips with parts of both chips?

letstrust_tpm2go

@PaulKissinger I hope this is detailed enough, else I'll have to find a better camera than my phone 😅

@PaulKissinger
Copy link
Contributor

Hi @tobyp,

yes, wrong hardware revision. The chip select of the TPM is connected to the GPIO1 of the USB Bridge. I have a new revision on my desk so I'll replace your stick in the next weeks.

Context: The patchset from @wxleong uses the burst count register and so wait states are not needed. This behavior reduces the communication between the host and the USB bridge. In the early phases of the hardware, it was not clear if the Chipselect works as expected, or that these sticks need a user-controllable chip select via GPIO. The latest HW revision will have a USB Bridge-controlled chip select.

Thank you and best regards,

Paul

@wxleong wxleong force-pushed the develop-tcti-spi-lt2go3 branch 2 times, most recently from f8207a0 to 0aea261 Compare January 6, 2023 03:15
@wxleong
Copy link
Member Author

wxleong commented Jan 6, 2023

I also see an error message in the kernel log for every tss command, like this:

[ 9695.096293] usb 1-1.3: usbfs: process 108052 (tpm2_startup) did not claim interface 0 before use

I made a fix commit here, feel free to squash it into your branch if you agree this is a good solution (I'm not really fit with USB, I'm not even sure what "claiming" an interface means, just that the error bothers me :P).

I'll look into this

@wxleong
Copy link
Member Author

wxleong commented Jan 6, 2023

I think you also want to rebase ontop of current master

merged master, 2b7007c

Again, always rebase, if your using git directly its:

git pull --rebase origin master

Ill update this one as well, same instructions for updating as:

Also, ill squash the history down to one comimt, we don't want commits that are like:

  1. add feature X
  2. fix thing A
  3. fix thing B

It should all be atomic.

Thanks for the help on squashing the commits. Merge was a mistake, moving forward i'll stick to rebase.

@wxleong wxleong force-pushed the develop-tcti-spi-lt2go3 branch 2 times, most recently from 4dd5e4d to 9b97f20 Compare January 10, 2023 09:07
@wxleong
Copy link
Member Author

wxleong commented Jan 10, 2023

I also see an error message in the kernel log for every tss command, like this:

[ 9695.096293] usb 1-1.3: usbfs: process 108052 (tpm2_startup) did not claim interface 0 before use

I made a fix commit here, feel free to squash it into your branch if you agree this is a good solution (I'm not really fit with USB, I'm not even sure what "claiming" an interface means, just that the error bothers me :P).

I'll look into this

Fixed in 9b97f20 thanks @tobyp.

@wxleong wxleong force-pushed the develop-tcti-spi-lt2go3 branch 6 times, most recently from 8cbd219 to f5e27f9 Compare January 12, 2023 05:39
LetsTrust-TPM2Go is a USB 2.0 stick with built-in TPM.

The base is taken from https://gist.github.com/tobyp/aed5598188088f4abbeb737b408e5287.

Signed-off-by: Peter Huewe <Peter.Huewe@infineon.com>
Signed-off-by: wenxin.leong <wenxin.leong@infineon.com>
Signed-off-by: William Roberts <william.c.roberts@intel.com>
@codecov
Copy link

codecov bot commented Jan 12, 2023

Codecov Report

Merging #2479 (2b7007c) into master (6c2f14c) will decrease coverage by 0.00%.
The diff coverage is 57.25%.

❗ Current head 2b7007c differs from pull request most recent head 47b13bb. Consider uploading reports for the commit 47b13bb to get more accurate results

@@            Coverage Diff             @@
##           master    #2479      +/-   ##
==========================================
- Coverage   83.70%   83.70%   -0.01%     
==========================================
  Files         362      363       +1     
  Lines       41537    41564      +27     
==========================================
+ Hits        34769    34790      +21     
- Misses       6768     6774       +6     
Impacted Files Coverage Δ
src/tss2-tcti/tcti-spi-helper.c 41.23% <50.00%> (+0.05%) ⬆️
src/tss2-tcti/tcti-spi-lt2go.c 57.37% <57.37%> (ø)
src/tss2-esys/esys_crypto_ossl.c 75.25% <0.00%> (-0.76%) ⬇️
src/tss2-tcti/tcti-mssim.c 70.22% <0.00%> (ø)
src/tss2-esys/esys_crypto.c 87.18% <0.00%> (+0.15%) ⬆️
src/tss2-esys/esys_iutil.c 91.84% <0.00%> (+4.82%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@wxleong
Copy link
Member Author

wxleong commented Feb 7, 2023

@williamcroberts all requested changes are applied, please review

Copy link
Member

@williamcroberts williamcroberts left a comment

Choose a reason for hiding this comment

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

I am good with this, I'll let @JuergenReppSIT do a review and merge since I am stepping down as a full time maintainer.

@AndreasFuchsTPM AndreasFuchsTPM merged commit 24713d0 into tpm2-software:master Feb 17, 2023
@wxleong wxleong deleted the develop-tcti-spi-lt2go3 branch April 20, 2023 07:07
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.

None yet

6 participants