search

microsoft / ms-tpm-20-ref

Reference implementation of the TCG Trusted Platform Module 2.0 specification.
Other
345 stars 134 forks source link

Startup() fails on TPM Simulator when built with wolfSSL #26

Closed gpdloader closed 5 years ago

gpdloader commented 5 years ago

In Implementation.h

line : #define AES_MAX_KEY_SIZE ((AES_MAX_KEY_SIZE_BITS + 7) / 8)

should be: #define AES_MAX_KEY_SIZE AES_MAX_KEY_SIZE_BITS

Otherwise it doesn't work with wolfcrypt. I tried to fix locally. and seems to work.

dvescovi1 commented 5 years ago

having similar build issues. Seems all to be related to defines in this file.

DavidWooten commented 5 years ago

Convention is that alg_MAX_KEY_SIZE is in bytes. Besides, there is no reason to define AES_MAX_KEY_SIZE to be AES_MAX_KEY_SIZE_BITS when there already is a definition for the maximum number of bits in the key.

From: gpdloader [mailto:notifications@github.com] Sent: Monday, March 4, 2019 8:14 AM To: Microsoft/ms-tpm-20-ref Cc: Subscribed Subject: [Microsoft/ms-tpm-20-ref] Implementation.h bug? (#26)

In Implementation.h

line :

define AES_MAX_KEY_SIZE ((AES_MAX_KEY_SIZE_BITS + 7) / 8)

should be:

define AES_MAX_KEY_SIZE AES_MAX_KEY_SIZE_BITS

Otherwise It doesn't work with wolfcrypt

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/Microsoft/ms-tpm-20-ref/issues/26 , or mute the thread https://github.com/notifications/unsubscribe-auth/AKE3mOh05-LhiyPHjJjkB4I9QcBJNVGjks5vTRw1gaJpZM4bcUMI . https://github.com/notifications/beacon/AKE3mCkoziAXs3kFEaGgYNIywc2GBU8Wks5vTRw1gaJpZM4bcUMI.gif

amarochk commented 5 years ago

This definition cannot be the reason of the failure you see because it is absolutely correct syntactically, and the suggested replacement is logically wrong, as it assigns the number of bits to the number of bytes without proper conversion.

gpdloader commented 5 years ago

Thanks for reply, Then it is wrong usage by wolfcrypt aes.c wc_AesSetKeyLocal(), this constant is only used in this file. (and settings.h from wolfcrypt) TPM was always in failure mode because of this. BTW. i compiled solution as described in README.md file.

dvescovi1 commented 5 years ago

In my case the error was introduced with the recent commits. It seems to work for the core code but it broke the Samples Firmware TPM code. I am building for ARM64. The sample folder should also be renamed as it should support both ARM32 and ARM64 builds now.

dmcilvaney commented 5 years ago

I'll take some time today or tomorrow and update our Implemenatation.h in the ARM32 sample. I'll leave the name for now since we are considering consolidating the fTPM TA with some other TAs in a central location. I would rather not break the build infrastructure twice in quick succession for that.

If you want to get back to building quickly 65b65354c6cce3212d9c512ec3ae2e23fe37c94d is a solid version.

dvescovi1 commented 5 years ago

That would be great.

I am using the previous commit for my build now.

Thanks,

David Vescovi, MVP

From: Daniel McIlvaney notifications@github.com Sent: Tuesday, March 5, 2019 1:55 PM To: Microsoft/ms-tpm-20-ref ms-tpm-20-ref@noreply.github.com Cc: David Vescovi dvescovi@tampabay.rr.com; Comment comment@noreply.github.com Subject: Re: [Microsoft/ms-tpm-20-ref] Implementation.h bug? (#26)

I'll take some time today or tomorrow and update our Implemenatation.h in the ARM32 sample. I'll leave the name for now since we are considering consolidating the fTPM TA with some other TAs in a central location. I would rather not break the build infrastructure twice in quick succession for that.

If you want to get back to building quickly https://github.com/Microsoft/ms-tpm-20-ref/commit/65b65354c6cce3212d9c512ec3ae2e23fe37c94d 65b6535 is a solid version.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/Microsoft/ms-tpm-20-ref/issues/26#issuecomment-469813317 , or mute the thread https://github.com/notifications/unsubscribe-auth/ABQFNHk5qGxUHTuVu-BvL7r_ugPH3_Ybks5vTr2YgaJpZM4bcUMI . https://github.com/notifications/beacon/ABQFNHdQynyEo7rL2ZHt9lgCOzuZFN5Mks5vTr2YgaJpZM4bcUMI.gif

dmcilvaney commented 5 years ago

Having looked into it a bit more, I think its probably better to just leave the current version alone for a while.

I have a bunch of cleanup work comming for the TPM, and a new UEFI variable store TA. The UEFI repos include a pre-compiled version of the TPM which I think should be ok for now. The build instructions will be updated to reflect this for now (See https://github.com/ms-iot/imx-iotcore/pull/41).

If there is a need for an update in the short term let me know, I can spend the time bringing this version forward.

dmcilvaney commented 5 years ago

As a follow up, the new version is available at www.github.com/Microsoft/MSRSec. That repo includes ms-tpm-20-ref as a submodule. When I have some time I'll see about updating the sample here.

DavidWooten commented 5 years ago

A problem with a specific crypto library should not be fixed in Implementation.h (now TpmAlgorithmDefines.h and TpmProfile.h). Those files are 'auto-generated' by a script that is run every time there is a new version of the spec. If you manually change an 'auto-generated' file in order to handle a specific crypto library then the script will change it back and it won't get caught unless it is tested with your favorite library. Things that are specific to a crypto library should go in a library-specific header, not in an auto-generated file.