Tcgtpm v184 cmake#20
Conversation
Reviewer's GuideThis PR overhauls the libtcgtpm build by migrating the TPM 2.0 reference implementation from autotools to CMake, defines new cryptolib build targets, refactors TpmConfiguration headers and CMake modules, introduces a Vendor_TCG_Test command module, and updates documentation and CI workflows to reflect the new CMake-based workflow. Class diagram for Vendor_TCG_Test command integrationclassDiagram
class Vendor_TCG_Test_In {
+TPM2B_DATA inputData
}
class Vendor_TCG_Test_Out {
+TPM2B_DATA outputData
}
class Vendor_TCG_Test_Entry {
+operator()(Vendor_TCG_Test_In* in, Vendor_TCG_Test_Out* out): TPM_RC
}
class Vendor_TCG_Test_COMMAND_DESCRIPTOR_t {
+Vendor_TCG_Test_Entry* entry
+UINT16 inSize
+UINT16 outSize
+UINT16 offsetOfTypes
+BYTE types[4]
}
Vendor_TCG_Test_Entry <|-- Vendor_TCG_Test_COMMAND_DESCRIPTOR_t
Vendor_TCG_Test_In <|-- Vendor_TCG_Test_COMMAND_DESCRIPTOR_t
Vendor_TCG_Test_Out <|-- Vendor_TCG_Test_COMMAND_DESCRIPTOR_t
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `libtcgtpm/deps/TpmConfiguration/TpmVendorCommandHandlers/CMakeLists.txt:4-7` </location>
<code_context>
+##########################################################
+# DO NOT MODIFY - TPM Configuration Library
+##########################################################
+# The contents of the source files may be modified to specify any valid
+# desired configuraton. However, the CMake library name "TpmVendorCommands"
+# is required by the CoreLib to reference this project. Recommend making
+# no changes in this file.
</code_context>
<issue_to_address>
**issue (typo):** Typo in comment: 'configuraton' should be 'configuration'.
Please update the comment to fix the spelling error.
```suggestion
# The contents of the source files may be modified to specify any valid
# desired configuration. However, the CMake library name "TpmVendorCommands"
# is required by the CoreLib to reference this project. Recommend making
# no changes in this file.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| # The contents of the source files may be modified to specify any valid | ||
| # desired configuraton. However, the CMake library name "TpmVendorCommands" | ||
| # is required by the CoreLib to reference this project. Recommend making | ||
| # no changes in this file. |
There was a problem hiding this comment.
issue (typo): Typo in comment: 'configuraton' should be 'configuration'.
Please update the comment to fix the spelling error.
| # The contents of the source files may be modified to specify any valid | |
| # desired configuraton. However, the CMake library name "TpmVendorCommands" | |
| # is required by the CoreLib to reference this project. Recommend making | |
| # no changes in this file. | |
| # The contents of the source files may be modified to specify any valid | |
| # desired configuration. However, the CMake library name "TpmVendorCommands" | |
| # is required by the CoreLib to reference this project. Recommend making | |
| # no changes in this file. |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughMigrates the TPM 2.0 build from autotools to CMake: CI workflows and docs replace autotools packages with cmake; libtcgtpm Makefile is restructured to use a centralized TCGTPM_BUILD_DIR, new library target variables, and CMake-based configuration and build rules. Changes
Sequence Diagram(s)sequenceDiagram
participant CI as CI Workflows
participant Make as Top-level Make
participant CMake as CMake (TCGTPM_BUILD_DIR)
participant Lib as Libraries (TPM, Platform, Crypto)
participant Art as Build Artifacts
rect rgb(241,250,255)
Note over CI,Make: New CI installs cmake & pkg-config
CI->>Make: invoke build
end
rect rgb(255,249,240)
Note over Make,CMake: New Flow (CMake-based)
Make->>CMake: run cmake in TCGTPM_BUILD_DIR (pass CFLAGS, crypto paths, config dir)
CMake->>Lib: configure and build LIBBNMATH, LIBTPMBIGNUM, LIBTPM, LIBPLATFORM
Lib->>Art: produce centralized artifacts under TCGTPM_BUILD_DIR
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
libtcgtpm/deps/TpmConfiguration/TpmVendorCommandHandlers/CMakeLists.txt (1)
4-7: Correct the typo in the guidance comment.Line 5 spells “configuration” incorrectly; please fix the comment copy.
Apply this diff:
-# desired configuraton. However, the CMake library name "TpmVendorCommands" +# desired configuration. However, the CMake library name "TpmVendorCommands"
🧹 Nitpick comments (2)
libtcgtpm/deps/TpmConfiguration/TpmConfiguration/VendorCommands/CommandAttributeData_s_ccAttr.inl (1)
13-16: Consider adding inline parameter documentation.The TPMA_CC_INITIALIZER call and conditional logic are correct. For improved maintainability, consider adding a brief inline comment documenting what each of the nine parameters represents, especially since this is vendor-specific configuration.
Example:
#if (PAD_LIST || CC_Vendor_TCG_Test) // TPM_CC_Vendor_TCG_Test +// TPMA_CC_INITIALIZER(commandCode, commandIndex, nv, extensive, flushed, cHandles, rHandle, V, Res) TPMA_CC_INITIALIZER(0x0000, 0, 0, 0, 0, 0, 0, 1, 0), #endiflibtcgtpm/deps/TpmConfiguration/TpmConfiguration/TpmProfile_CommandList.h (1)
166-167: Consider adding documentation for new command macros.The new
CC_ReadOnlyControlandCC_PolicyTransportSPDMcommand macros lack explanatory comments. For consistency with the file's existing style (see lines 28-42), consider adding brief comments describing the purpose of these commands or why they are enabled.Example:
+// ReadOnlyControl command allows managing read-only control state #define CC_ReadOnlyControl CC_YES +// PolicyTransportSPDM enables policy-based SPDM transport #define CC_PolicyTransportSPDM CC_YES
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
.github/workflows/publish-docs.yml(1 hunks).github/workflows/qemu.yml(1 hunks).github/workflows/rust.yml(1 hunks)Documentation/docs/installation/INSTALL.md(1 hunks)libtcgtpm/Makefile(2 hunks)libtcgtpm/deps/TpmConfiguration/CMakeLists.txt(1 hunks)libtcgtpm/deps/TpmConfiguration/TpmConfiguration/TpmBuildSwitches.h(4 hunks)libtcgtpm/deps/TpmConfiguration/TpmConfiguration/TpmProfile.h(0 hunks)libtcgtpm/deps/TpmConfiguration/TpmConfiguration/TpmProfile_CommandList.h(2 hunks)libtcgtpm/deps/TpmConfiguration/TpmConfiguration/TpmProfile_Common.h(2 hunks)libtcgtpm/deps/TpmConfiguration/TpmConfiguration/TpmProfile_ErrorCodes.h(1 hunks)libtcgtpm/deps/TpmConfiguration/TpmConfiguration/VendorCommands/CommandAttributeData_s_ccAttr.inl(1 hunks)libtcgtpm/deps/TpmConfiguration/TpmConfiguration/VendorCommands/CommandAttributeData_s_commandAttributes.inl(1 hunks)libtcgtpm/deps/TpmConfiguration/TpmConfiguration/VendorCommands/CommandDispatchData_CommandStructures.inl(1 hunks)libtcgtpm/deps/TpmConfiguration/TpmConfiguration/VendorCommands/CommandDispatchData_s_CommandDataArray.inl(1 hunks)libtcgtpm/deps/TpmConfiguration/TpmConfiguration/VendorCommands/CommandDispatcher.inl(1 hunks)libtcgtpm/deps/TpmConfiguration/TpmConfiguration/VendorCommands/HandleProcess.inl(1 hunks)libtcgtpm/deps/TpmConfiguration/TpmConfiguration/VendorCommands/VendorCommandList.h(1 hunks)libtcgtpm/deps/TpmConfiguration/TpmConfiguration/VendorCommands/prototypes/Vendor_TCG_Test_fp.h(1 hunks)libtcgtpm/deps/TpmConfiguration/TpmConfiguration/VendorInfo.h(0 hunks)libtcgtpm/deps/TpmConfiguration/TpmVendorCommandHandlers/CMakeLists.txt(1 hunks)libtcgtpm/deps/TpmConfiguration/TpmVendorCommandHandlers/Vendor_TCG_Test.c(1 hunks)libtcgtpm/deps/tpm-20-ref(1 hunks)scripts/container/opensuse-rust.docker(1 hunks)
💤 Files with no reviewable changes (2)
- libtcgtpm/deps/TpmConfiguration/TpmConfiguration/TpmProfile.h
- libtcgtpm/deps/TpmConfiguration/TpmConfiguration/VendorInfo.h
🔇 Additional comments (29)
libtcgtpm/deps/TpmConfiguration/TpmConfiguration/TpmProfile_ErrorCodes.h (1)
45-48: Verify usage of new error codes.The new error code definitions are well-structured with sequential numbering and consistent naming. Since this appears to be a submodule update (per the PR summary), ensure these error codes are properly handled in the consuming code where they're raised.
Run the following script to verify usage:
libtcgtpm/deps/TpmConfiguration/TpmConfiguration/TpmProfile_Common.h (2)
175-176: Verify the vendor property constraint is properly enforced.The comment states that
MAX_VENDOR_PROPERTY"must currently be 1," but it's unclear whether this constraint is validated elsewhere in the codebase. Given that vendor command scaffolding is being introduced in this PR, ensure that:
- This value aligns with the actual number of vendor properties being supported
- The constraint is enforced at compile-time or runtime to prevent misconfigurations
Run the following script to check for related vendor property definitions and usages:
225-230: Verify PolicyTransportSPDM implementation is complete.Enabling
SEC_CHANNEL_SUPPORTintroduces secure channel functionality with PolicyTransportSPDM. Please confirm that:
- The referenced
CC_PolicyTransportSPDMcommand is fully implemented- The PolicyTransportSPDM feature is production-ready and tested
- This enablement is intentional for the current build configuration
Run the following script to verify the PolicyTransportSPDM implementation:
libtcgtpm/deps/TpmConfiguration/TpmConfiguration/TpmBuildSwitches.h (5)
75-75: Verify that disabling CERTIFYX509_DEBUG is intentional.
CERTIFYX509_DEBUGwas changed from(YES * DEBUG)to(NO * DEBUG), which effectively disables it even whenDEBUGis enabled. Please confirm this is intentional and not a mistake that should be(YES * DEBUG).
157-158: LGTM!The
LONGJMP_SUPPORTEDmacro is a clear feature flag with a reasonable default value.
54-63: Verify that SIMULATION cannot be enabled in production builds.
ENABLE_CRYPTO_DEBUGis controlled byENABLE_TPM_DEBUG_PRINT, which is enabled whenSIMULATIONisYES. The comment on line 62 explicitly states this is "entirely insecure" because it prints actual crypto values. Ensure thatSIMULATIONis alwaysNOin production builds to prevent this security risk.Run the following script to verify build configurations and usage of SIMULATION:
195-195: Verify the new include path exists and is correct.The include path was updated from
<public/CompilerDependencies.h>to<tpm_public/CompilerDependencies.h>as part of the CMake migration. Ensure this path exists and the header file is accessible at the new location.Run the following script to verify the new include path:
51-197: Verify that removed macros are not referenced elsewhere.According to the AI summary, several macros were removed from this file:
TABLE_DRIVEN_DISPATCH,USE_MARSHALING_DEFINES,COMPRESSED_LISTS, andUSE_BIT_FIELD_STRUCTURES. Ensure these removals don't break any dependent code.Run the following script to check for references to the removed macros:
libtcgtpm/deps/TpmConfiguration/TpmConfiguration/VendorCommands/CommandAttributeData_s_ccAttr.inl (2)
1-9: LGTM!The header comments clearly explain the file's purpose and usage constraints, including the important warning about array initializer context and the rationale for the .INL extension.
10-12: Effective safety guard.The preprocessor check ensures this file is only included in the correct context, preventing misuse and providing a clear error message if included elsewhere.
libtcgtpm/deps/TpmConfiguration/TpmConfiguration/VendorCommands/HandleProcess.inl (1)
8-9: Verify that the empty case statement is intentional.The case for
TPM_CC_Vendor_TCG_Testonly contains abreakwithout calling any handler function. According to the PR summary, the corresponding vendor command handler logic is implemented inVendor_TCG_Test.c. Confirm whether:
- This is intentional scaffolding for future implementation
- The handler call is missing and should be added
- The command is processed elsewhere in the flow
Run the following script to check for handler implementations:
libtcgtpm/deps/TpmConfiguration/TpmConfiguration/VendorCommands/CommandDispatcher.inl (1)
11-14: Verify buffer allocation success.While
MemoryGetInBufferandMemoryGetOutBufferare likely guaranteed to succeed or handle errors internally in TPM reference code, consider verifying that these functions cannot return NULL or document this assumption.libtcgtpm/deps/TpmConfiguration/TpmVendorCommandHandlers/Vendor_TCG_Test.c (2)
1-7: LGTM! Conditional compilation structure is appropriate.The include directives and conditional compilation guard follow standard patterns for vendor-specific TPM command handlers.
13-13: Verify data structure assignment safety.The assignment
out->outputData = in->inputDataperforms a direct copy. Depending on the types ofinputDataandoutputData, verify that:
- If they are TPM2B buffer structures, size constraints are enforced to prevent buffer overruns
- If they are pointers, the lifetime and ownership semantics are correct
- If they contain nested structures or buffers, deep copying is handled appropriately
Run the following script to examine the struct definitions and identify potential safety concerns:
libtcgtpm/deps/TpmConfiguration/TpmConfiguration/VendorCommands/CommandAttributeData_s_commandAttributes.inl (2)
1-12: LGTM!The file structure, comments, and include guard are well-implemented. The include guard properly ensures the file is only included in the correct context within CommandAttributeData.h.
13-16: Verify the command code comment and attribute expression.The comment on line 14 states
0x0000for CC_Vendor_TCG_Test, which seems atypical for vendor TPM commands that usually occupy higher command code ranges. Additionally, confirm that the arithmetic expressionCC_Vendor_TCG_Test * (DECRYPT_2 + ENCRYPT_2)correctly encodes the intended command attributes for this vendor command.Run the following script to examine the command code definition and attribute macros:
libtcgtpm/deps/TpmConfiguration/TpmConfiguration/VendorCommands/CommandDispatchData_s_CommandDataArray.inl (1)
1-15: LGTM! Include file follows the expected pattern.The structure is correct for an .INL file designed to be included in an array initializer. The preprocessor guard ensures proper usage context, the conditional compilation is clear, and the trailing comma on line 14 is appropriate for an array element.
Verify that
_Vendor_TCG_TestDataAddressandCOMMAND_DESCRIPTOR_tare properly defined:libtcgtpm/deps/TpmConfiguration/TpmConfiguration/VendorCommands/prototypes/Vendor_TCG_Test_fp.h (4)
2-5: LGTM!The conditional compilation guard and include guard pattern is correct and follows standard conventions for TPM private prototypes.
19-20: LGTM!The response code modifier follows the standard TPM pattern for parameter-specific error codes.
23-24: LGTM!The function prototype follows the standard TPM command handler signature with appropriate return type and parameter pointers.
7-17: Verify that TPM types are defined before this header is included.The structures use
TPM2B_DATAwithout including the header that defines it. Ensure this header is always included after the base TPM type definitions (e.g.,Tpm.horTpmTypes.h).Run the following script to check the include order and verify that TPM2B_DATA is defined in the expected headers:
libtcgtpm/deps/TpmConfiguration/TpmConfiguration/TpmProfile_CommandList.h (2)
15-21: LGTM! Documentation and formatting improvements.The clarified comment about the CC_YES/CC_NO debug feature and the clang-format directives properly document the formatting boundaries for this section.
169-171: LGTM! Formatting directive and vendor command include.The clang-format on directive properly closes the formatting boundary, and the include of
VendorCommandList.haligns with the PR's vendor command infrastructure changes.libtcgtpm/deps/TpmConfiguration/TpmConfiguration/VendorCommands/CommandDispatchData_CommandStructures.inl (2)
14-41: LGTM with minor notes above.The command descriptor structure and conditional compilation logic are well-designed:
- Proper use of designated initializers for clarity
- Correct sizeof() and offsetof() usage for portability
- Clean conditional macro pattern enabling unconditional use
- Types array marshaling descriptor follows expected pattern
The implementation correctly sets up vendor command dispatch metadata for table-driven command processing.
29-36: Verify identifier naming compliance with C/C++ standards.The variable
_Vendor_TCG_TestDatabegins with an underscore followed by an uppercase letter. Per C/C++ standards, such identifiers are reserved for the implementation in the global namespace. While this may be intentional for internal symbols, please verify that this naming convention aligns with your project's guidelines and doesn't conflict with implementation-reserved identifiers.Run the following script to check for similar naming patterns in the codebase:
.github/workflows/publish-docs.yml (1)
30-31: CMake dependency switch looks good.Dropping the autotools stack in favor of
build-essentialandcmakealigns with the new build flow. No issues noticed..github/workflows/qemu.yml (1)
115-115: CMake dependency update makes sense.Switching the TPMLib build to CMake makes
build-essential+cmakesufficient here, so the trimmed package list looks good..github/workflows/rust.yml (1)
31-32: CMake dependency update looks goodSwapping in
cmakealongsidebuild-essentialaligns with the new CMake-based TPM build flow; no additional packages appear necessary.scripts/container/opensuse-rust.docker (1)
26-27: LGTM! Migration to CMake looks correct.The replacement of autotools packages (autoconf, autoconf-archive, pkg-config, automake, perl) with cmake aligns with the PR's objective to migrate the TPM library build system to CMake.
Please verify that the cmake package alone provides all necessary build capabilities for the TPM library without the removed autotools dependencies. You may also want to confirm whether the CMakeLists.txt files require a minimum cmake version and whether the openSUSE cmake package meets that requirement.
| export_targets_cmake_file(${PROJECT_NAME}) | ||
|
|
||
| add_subdirectory(TpmVendorCommandHandlers) |
There was a problem hiding this comment.
Keep export_targets_cmake_file() as the final step.
export_targets_cmake_file(${PROJECT_NAME}) configures the package export and the file itself is annotated as “LAST”. By invoking add_subdirectory(TpmVendorCommandHandlers) afterwards, the handler targets are defined only after the export has already run, so they are not captured in the generated targets file/install set. Please move the add_subdirectory call above the export (or otherwise ensure the export runs last).
🤖 Prompt for AI Agents
In libtcgtpm/deps/TpmConfiguration/CMakeLists.txt around lines 59-61, the call
to export_targets_cmake_file(${PROJECT_NAME}) currently runs before
add_subdirectory(TpmVendorCommandHandlers), so the handler targets defined in
that subdirectory are not included in the generated export/install targets; move
the add_subdirectory(TpmVendorCommandHandlers) line so it appears before the
export_targets_cmake_file(${PROJECT_NAME}) call (or otherwise ensure the export
invocation happens last) so that all targets from the subdirectory are defined
and captured in the exported targets file.
| // entry per vendor command. See the private Tpm header CommandAttributeData.h for | ||
| // more info. | ||
| // (This is why the file has the .INL extension, it's not a normal header. | ||
| // |
There was a problem hiding this comment.
Fix the incomplete sentence.
The opening parenthesis on Line 9 is never closed, making the comment grammatically incomplete.
Apply this diff to fix the comment:
-// (This is why the file has the .INL extension, it's not a normal header.
+// (This is why the file has the .INL extension, it's not a normal header.)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // | |
| // TCG Vendor Command Structures - used in command dispatch table | |
| #ifndef _COMMAND_TABLE_DISPATCH_ | |
| #error "Do not include this file without defining _COMMAND_TABLE_DISPATCH_" | |
| #endif | |
| // (This is why the file has the .INL extension, it's not a normal header.) |
🤖 Prompt for AI Agents
libtcgtpm/deps/TpmConfiguration/TpmConfiguration/VendorCommands/CommandDispatchData_CommandStructures.inl
around line 9: the comment contains an unclosed opening parenthesis leaving the
sentence incomplete; edit the comment on line 9 to close the parenthesis and
complete the sentence (or remove the stray parenthesis), ensuring the comment
reads as a grammatically complete sentence with matched parentheses.
| if(result != TPM_RC_SUCCESS) | ||
| return result; |
There was a problem hiding this comment.
Inconsistent error handling may skip cleanup code.
Line 24 returns directly on handler error, while lines 16 and 20 use the goto Exit pattern. If the Exit label (defined outside this inline file) performs cleanup operations such as releasing buffers or resetting state, returning early here bypasses that cleanup, potentially causing resource leaks or corrupted state.
Apply this diff to maintain consistent error handling:
- result = TPM2_Vendor_TCG_Test(in, out);
- if(result != TPM_RC_SUCCESS)
- return result;
+ result = TPM2_Vendor_TCG_Test(in, out);
+ EXIT_IF_ERROR(result);Alternatively, if the EXIT_IF_ERROR macro is not available without a parameter, use:
- if(result != TPM_RC_SUCCESS)
- return result;
+ if(result != TPM_RC_SUCCESS)
+ goto Exit;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
libtcgtpm/deps/TpmConfiguration/TpmConfiguration/VendorCommands/CommandDispatcher.inl
around lines 23-24, the handler error is returned directly which bypasses the
external Exit label cleanup; change the direct return to follow the same pattern
as lines 16 and 20 by setting the error return value and branching to the Exit
label (or replace the return with the appropriate EXIT_IF_ERROR /
EXIT_OK_IF_ERROR macro invocation) so that the function always jumps to Exit for
cleanup before returning.
| @@ -0,0 +1,10 @@ | |||
| // This file contains an inlined portion of the HandleProcssor switch statement. | |||
There was a problem hiding this comment.
Fix typo in comment.
"HandleProcssor" should be "HandleProcessor".
Apply this diff:
-// This file contains an inlined portion of the HandleProcssor switch statement.
+// This file contains an inlined portion of the HandleProcessor switch statement.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // This file contains an inlined portion of the HandleProcssor switch statement. | |
| // This file contains an inlined portion of the HandleProcessor switch statement. |
🤖 Prompt for AI Agents
In
libtcgtpm/deps/TpmConfiguration/TpmConfiguration/VendorCommands/HandleProcess.inl
around lines 1 to 1, there is a typo in the comment "HandleProcssor" which
should read "HandleProcessor"; update the comment text to correct the spelling
to "HandleProcessor" and ensure any related occurrences in this file are
similarly corrected for consistency.
| target_include_directories(TpmVendorCommands_Handlers PRIVATE | ||
| "$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/../../tpm/include>" | ||
| ) |
There was a problem hiding this comment.
Fix the private include path.
Line 32 resolves ${CMAKE_CURRENT_SOURCE_DIR}/../../tpm/include to .../deps/tpm/include, which does not exist; the headers live under TpmConfiguration/tpm/include. This breaks compilation as soon as the handler sources include those headers.
Apply this diff:
target_include_directories(TpmVendorCommands_Handlers PRIVATE
- "$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/../../tpm/include>"
+ "$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/../tpm/include>"
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| target_include_directories(TpmVendorCommands_Handlers PRIVATE | |
| "$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/../../tpm/include>" | |
| ) | |
| target_include_directories(TpmVendorCommands_Handlers PRIVATE | |
| "$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/../tpm/include>" | |
| ) |
🤖 Prompt for AI Agents
In libtcgtpm/deps/TpmConfiguration/TpmVendorCommandHandlers/CMakeLists.txt
around lines 31-33, the PRIVATE include path currently points to
../../tpm/include which resolves to deps/tpm/include (nonexistent); change the
include path to the correct relative location
${CMAKE_CURRENT_SOURCE_DIR}/../tpm/include (or adjust the
target_include_directories/INCLUDE directive to reference
TpmConfiguration/tpm/include) so the handler sources can find the headers.
| TPM_RC | ||
| TPM2_Vendor_TCG_Test(Vendor_TCG_Test_In* in, // IN: input parameter list | ||
| Vendor_TCG_Test_Out* out // OUT: output parameter list | ||
| ) | ||
| { | ||
| out->outputData = in->inputData; | ||
| return TPM_RC_SUCCESS; | ||
| } |
There was a problem hiding this comment.
Add NULL pointer validation for input/output parameters.
The function dereferences in and out without validating that they are non-NULL. In TPM command handlers, input validation is critical to prevent crashes or undefined behavior when the dispatcher invokes the handler with invalid parameters.
Apply this diff to add validation:
TPM_RC
TPM2_Vendor_TCG_Test(Vendor_TCG_Test_In* in, // IN: input parameter list
Vendor_TCG_Test_Out* out // OUT: output parameter list
)
{
+ if (in == NULL || out == NULL)
+ return TPM_RC_FAILURE;
+
out->outputData = in->inputData;
return TPM_RC_SUCCESS;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| TPM_RC | |
| TPM2_Vendor_TCG_Test(Vendor_TCG_Test_In* in, // IN: input parameter list | |
| Vendor_TCG_Test_Out* out // OUT: output parameter list | |
| ) | |
| { | |
| out->outputData = in->inputData; | |
| return TPM_RC_SUCCESS; | |
| } | |
| TPM_RC | |
| TPM2_Vendor_TCG_Test(Vendor_TCG_Test_In* in, // IN: input parameter list | |
| Vendor_TCG_Test_Out* out // OUT: output parameter list | |
| ) | |
| { | |
| if (in == NULL || out == NULL) | |
| return TPM_RC_FAILURE; | |
| out->outputData = in->inputData; | |
| return TPM_RC_SUCCESS; | |
| } |
🤖 Prompt for AI Agents
In libtcgtpm/deps/TpmConfiguration/TpmVendorCommandHandlers/Vendor_TCG_Test.c
around lines 8-15, the function dereferences the pointers in and out without
validation; add NULL checks at the top of the function to verify both input
pointers are non-NULL and if either is NULL, return an appropriate TPM error
(e.g., TPM_RC_FAILURE or TPM_RC_BAD_PARAMETER) before any dereference, ensuring
any required cleanup or logging is done prior to returning.
stefano-garzarella
force-pushed
the
tcgtpm-v184-cmake
branch
from
308b27a to
d186704
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libtcgtpm/Makefile (1)
31-180: Release build inherits debug CFLAGS
$(TCGTPM_MAKEFILE)is generated only once per build directory. The first configure (typically the debug build) recordsCMAKE_C_FLAGSin the cache; when we later invoke the Makefile withRELEASE=1, CMake is not re-run, so the release artifacts continue to use the debug flags (-g -O0 -DDEBUG=YES). As a result the release images are built unoptimized.Please keep separate build directories (or another stamp) per variant so that a release configure actually applies the release flags. One option:
-TCGTPM_BUILD_DIR = $(TCGTPM_DIR)/build +TCGTPM_BUILD_VARIANT = $(if $(RELEASE),release,debug) +TCGTPM_BUILD_DIR = $(TCGTPM_DIR)/build/$(TCGTPM_BUILD_VARIANT) @@ -distclean: clean - rm -f $(OPENSSL_MAKEFILE) - rm -rf $(TCGTPM_BUILD_DIR) +distclean: clean + rm -f $(OPENSSL_MAKEFILE) + rm -rf $(TCGTPM_DIR)/build
♻️ Duplicate comments (3)
libtcgtpm/deps/TpmConfiguration/TpmVendorCommandHandlers/Vendor_TCG_Test.c (1)
8-15: Validate pointers before dereference
TPM2_Vendor_TCG_Testdereferencesinandoutwithout verifying they are non-NULL. A NULL pointer from the dispatcher will crash the handler. Please guard both pointers and return an appropriate TPM error before using them.TPM_RC TPM2_Vendor_TCG_Test(Vendor_TCG_Test_In* in, // IN: input parameter list Vendor_TCG_Test_Out* out // OUT: output parameter list ) { + if (in == NULL || out == NULL) + return TPM_RC_FAILURE; + out->outputData = in->inputData; return TPM_RC_SUCCESS; }libtcgtpm/deps/TpmConfiguration/CMakeLists.txt (1)
59-61: Keepexport_targets_cmake_file()as the final step.
export_targets_cmake_file(${PROJECT_NAME})should run after all targets are defined. Sinceadd_subdirectory(TpmVendorCommandHandlers)is called after the export, the handler targets defined in that subdirectory are not included in the generated export/install targets. Move theadd_subdirectorycall to appear before theexport_targets_cmake_filecall.libtcgtpm/deps/TpmConfiguration/TpmConfiguration/VendorCommands/CommandDispatchData_CommandStructures.inl (1)
9-9: Fix the incomplete sentence.The opening parenthesis is never closed, making the comment grammatically incomplete.
Apply this diff to fix the comment:
-// (This is why the file has the .INL extension, it's not a normal header. +// (This is why the file has the .INL extension, it's not a normal header.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
.github/workflows/publish-docs.yml(1 hunks).github/workflows/qemu.yml(1 hunks).github/workflows/rust.yml(1 hunks)Documentation/docs/installation/INSTALL.md(1 hunks)libtcgtpm/Makefile(2 hunks)libtcgtpm/deps/TpmConfiguration/CMakeLists.txt(1 hunks)libtcgtpm/deps/TpmConfiguration/TpmConfiguration/TpmBuildSwitches.h(4 hunks)libtcgtpm/deps/TpmConfiguration/TpmConfiguration/TpmProfile.h(0 hunks)libtcgtpm/deps/TpmConfiguration/TpmConfiguration/TpmProfile_CommandList.h(2 hunks)libtcgtpm/deps/TpmConfiguration/TpmConfiguration/TpmProfile_Common.h(2 hunks)libtcgtpm/deps/TpmConfiguration/TpmConfiguration/TpmProfile_ErrorCodes.h(1 hunks)libtcgtpm/deps/TpmConfiguration/TpmConfiguration/VendorCommands/CommandAttributeData_s_ccAttr.inl(1 hunks)libtcgtpm/deps/TpmConfiguration/TpmConfiguration/VendorCommands/CommandAttributeData_s_commandAttributes.inl(1 hunks)libtcgtpm/deps/TpmConfiguration/TpmConfiguration/VendorCommands/CommandDispatchData_CommandStructures.inl(1 hunks)libtcgtpm/deps/TpmConfiguration/TpmConfiguration/VendorCommands/CommandDispatchData_s_CommandDataArray.inl(1 hunks)libtcgtpm/deps/TpmConfiguration/TpmConfiguration/VendorCommands/CommandDispatcher.inl(1 hunks)libtcgtpm/deps/TpmConfiguration/TpmConfiguration/VendorCommands/HandleProcess.inl(1 hunks)libtcgtpm/deps/TpmConfiguration/TpmConfiguration/VendorCommands/VendorCommandList.h(1 hunks)libtcgtpm/deps/TpmConfiguration/TpmConfiguration/VendorCommands/prototypes/Vendor_TCG_Test_fp.h(1 hunks)libtcgtpm/deps/TpmConfiguration/TpmConfiguration/VendorInfo.h(0 hunks)libtcgtpm/deps/TpmConfiguration/TpmVendorCommandHandlers/CMakeLists.txt(1 hunks)libtcgtpm/deps/TpmConfiguration/TpmVendorCommandHandlers/Vendor_TCG_Test.c(1 hunks)libtcgtpm/deps/tpm-20-ref(1 hunks)scripts/container/opensuse-rust.docker(1 hunks)
💤 Files with no reviewable changes (2)
- libtcgtpm/deps/TpmConfiguration/TpmConfiguration/VendorInfo.h
- libtcgtpm/deps/TpmConfiguration/TpmConfiguration/TpmProfile.h
🚧 Files skipped from review as they are similar to previous changes (13)
- libtcgtpm/deps/TpmConfiguration/TpmConfiguration/VendorCommands/CommandAttributeData_s_ccAttr.inl
- libtcgtpm/deps/TpmConfiguration/TpmConfiguration/TpmProfile_ErrorCodes.h
- libtcgtpm/deps/TpmConfiguration/TpmConfiguration/VendorCommands/HandleProcess.inl
- libtcgtpm/deps/TpmConfiguration/TpmConfiguration/VendorCommands/VendorCommandList.h
- libtcgtpm/deps/tpm-20-ref
- Documentation/docs/installation/INSTALL.md
- libtcgtpm/deps/TpmConfiguration/TpmVendorCommandHandlers/CMakeLists.txt
- .github/workflows/qemu.yml
- libtcgtpm/deps/TpmConfiguration/TpmConfiguration/TpmBuildSwitches.h
- .github/workflows/publish-docs.yml
- libtcgtpm/deps/TpmConfiguration/TpmConfiguration/VendorCommands/CommandAttributeData_s_commandAttributes.inl
- libtcgtpm/deps/TpmConfiguration/TpmConfiguration/VendorCommands/CommandDispatcher.inl
- libtcgtpm/deps/TpmConfiguration/TpmConfiguration/TpmProfile_CommandList.h
🧰 Additional context used
🧬 Code graph analysis (1)
libtcgtpm/deps/TpmConfiguration/TpmConfiguration/VendorCommands/prototypes/Vendor_TCG_Test_fp.h (1)
libtcgtpm/deps/TpmConfiguration/TpmVendorCommandHandlers/Vendor_TCG_Test.c (1)
TPM2_Vendor_TCG_Test(8-15)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: QEMU NOCC boot test
- GitHub Check: Check
- GitHub Check: Sourcery review
🔇 Additional comments (6)
libtcgtpm/deps/TpmConfiguration/TpmConfiguration/TpmProfile_Common.h (2)
1-232: No references to removed macros found
Verified thatMOST_SIGNIFICANT_BIT_0andLEAST_SIGNIFICANT_BIT_0are not used anywhere in the codebase.
225-230: Verify CC_PolicyTransportSPDM handler and SPDM secure channel implementation are present
Search returned no CC_PolicyTransportSPDM handler definitions or any SPDM secure channel code. Ensure the command handler and all required secure‐channel infrastructure are implemented before setting SEC_CHANNEL_SUPPORT to YES.libtcgtpm/deps/TpmConfiguration/TpmConfiguration/VendorCommands/prototypes/Vendor_TCG_Test_fp.h (1)
2-27: LGTM! Well-structured vendor command header.The vendor command header follows TPM conventions correctly with proper conditional compilation, input/output structures, error code macros, and function prototypes. The implementation matches the prototype as seen in the related source file.
libtcgtpm/deps/TpmConfiguration/CMakeLists.txt (1)
43-55: LGTM! Vendor command headers properly installed.The installation of VendorCommands headers and prototypes is correctly structured with appropriate destination paths for downstream consumers.
libtcgtpm/deps/TpmConfiguration/TpmConfiguration/VendorCommands/CommandDispatchData_CommandStructures.inl (2)
10-12: LGTM! Proper inclusion guard.The guard correctly ensures this file is only included in the appropriate context (inside CommandDispatchData.h with table dispatching enabled).
14-41: LGTM! Vendor command descriptor correctly structured.The descriptor implementation is correct:
- Function type and structure definition match the command prototypes
- Initialization properly sets up entry point, sizes, and marshaling types
- The types array
{TPM2B_DATA_P_UNMARSHAL, END_OF_LIST, TPM2B_DATA_P_MARSHAL, END_OF_LIST}correctly matches the input/output structures (one TPM2B_DATA parameter each)- Conditional macro definition provides the address when enabled or zero otherwise
| // number of vendor properties, must currently be 1. | ||
| #define MAX_VENDOR_PROPERTY (1) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify the "must currently be 1" constraint is enforced.
The comment states that MAX_VENDOR_PROPERTY must currently be 1, which suggests a hard constraint in the implementation. Ensure that this constraint is validated at compile-time or runtime to prevent configuration errors.
Run the following script to search for validation or usage of this macro:
🏁 Script executed:
#!/bin/bash
# Description: Search for validation and usage of MAX_VENDOR_PROPERTY.
# Search for static assertions or validation
rg -nP --type=c --type=cpp -C3 'MAX_VENDOR_PROPERTY'
# Search for vendor property array declarations that may depend on this size
ast-grep --pattern 'vendorProperty[$_]'Length of output: 771
Enforce MAX_VENDOR_PROPERTY == 1 via static_assert
Add a compile-time assertion in TpmProfile_Common.h, for example:
static_assert(MAX_VENDOR_PROPERTY == 1, "MAX_VENDOR_PROPERTY must be 1");to guarantee the hard constraint.
🤖 Prompt for AI Agents
In libtcgtpm/deps/TpmConfiguration/TpmConfiguration/TpmProfile_Common.h around
lines 175-176, add a compile-time assertion to enforce that MAX_VENDOR_PROPERTY
equals 1. Insert a static_assert right after the #define to validate the
constant at compile time and provide a clear error message if the value changes,
ensuring builds fail early when the hard constraint is violated.
| // for vendor commands. | ||
| // | ||
| // IMPORTANT: This file is included in the middle of an array initializer | ||
| // therefore it must not contain anything other than comments and exactly one TPMA_CC | ||
| // entry per vendor command. See the private Tpm header CommandAttributeData.h for | ||
| // more info. | ||
| // (This is why the file has the .INL extension, it's not a normal header. |
There was a problem hiding this comment.
Update misleading comment to match actual file content.
The comment describes this file as containing "an inlined portion of the s_ccAttr array definition" with "exactly one TPMA_CC entry per vendor command", but the actual content defines descriptor structures (Vendor_TCG_Test_COMMAND_DESCRIPTOR_t and _Vendor_TCG_TestData), not TPMA_CC entries. This discrepancy suggests the comment was copied from a different .inl file template.
Update the comment to accurately describe the file's purpose:
-// This file contains an inlined portion of the s_ccAttr array definition
-// for vendor commands.
+// This file contains command descriptor structures for vendor commands.
//
-// IMPORTANT: This file is included in the middle of an array initializer
-// therefore it must not contain anything other than comments and exactly one TPMA_CC
-// entry per vendor command. See the private Tpm header CommandAttributeData.h for
-// more info.
+// IMPORTANT: This file should only be included inside CommandDispatchData.h
+// when table dispatching is turned on. It defines descriptor structures that
+// are referenced by address in the command dispatch table.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // for vendor commands. | |
| // | |
| // IMPORTANT: This file is included in the middle of an array initializer | |
| // therefore it must not contain anything other than comments and exactly one TPMA_CC | |
| // entry per vendor command. See the private Tpm header CommandAttributeData.h for | |
| // more info. | |
| // (This is why the file has the .INL extension, it's not a normal header. | |
| // This file contains command descriptor structures for vendor commands. | |
| // | |
| // IMPORTANT: This file should only be included inside CommandDispatchData.h | |
| // when table dispatching is turned on. It defines descriptor structures that | |
| // are referenced by address in the command dispatch table. | |
| // (This is why the file has the .INL extension, it's not a normal header. |
🤖 Prompt for AI Agents
In
libtcgtpm/deps/TpmConfiguration/TpmConfiguration/VendorCommands/CommandDispatchData_CommandStructures.inl
around lines 2 to 8, the top comment incorrectly states the file is an inlined
portion of an s_ccAttr array with TPMA_CC entries; instead the file contains
vendor command descriptor structures (e.g., Vendor_TCG_Test_COMMAND_DESCRIPTOR_t
and Vendor_TCG_TestData). Replace the misleading comment with a concise
description stating that this .inl defines vendor command descriptor structures
and data for command dispatch (and must be included where those structures are
expected), removing any reference to TPMA_CC entries or array initializers.
b362225 to
80fb22a
Compare
stefano-garzarella
force-pushed
the
tcgtpm-v184-cmake
branch
from
80fb22a to
3e691bc
Compare
The next TCG TPM reference implementation release (v185) will remove the support for autotools [1]. Currently, v184 supports both, so let's switch to cmake to be prepared. The main difference is that dependency on TpmBigNum and Math_Ossl was previously specified in the autotools files when producing libtpm.a and libplatform.a, but now it is only specified for the final product (e.g. the simulator in their project). Therefore, we must explicitly build and link them when producing libtcgtpm.a. Update documentation and CI about deps to install to build TPM. `gcc-c++` is required by the cmake project, otherwise the "No CMAKE_CXX_COMPILER could be found" error is printed. [1] TrustedComputingGroup/TPM#6 (comment) Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
stefano-garzarella
force-pushed
the
tcgtpm-v184-cmake
branch
from
3e691bc to
f381899
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libtcgtpm/Makefile (1)
34-43: Do not nest static archives; flatten inputs when building libtcgtpm.a. Also fix static lib order.
ar rcsTPD $@ $^nests .a files inside libtcgtpm.a; linkers won’t resolve symbols in nested archives. Extract object files and re-archive. Also list the dependent BigNum before its provider Math_Ossl to satisfy static resolution (left-to-right).Apply:
-LIBS = $(LIBCRT) $(LIBCRYPTO) $(LIBBNMATH) $(LIBTPMBIGNUM) $(LIBTPM) $(LIBPLATFORM) +LIBS = $(LIBCRT) $(LIBCRYPTO) $(LIBTPMBIGNUM) $(LIBBNMATH) $(LIBTPM) $(LIBPLATFORM) $(OUT_DIR)/libtcgtpm.a: $(LIBS) - rm -f $@ - ar rcsTPD $@ $^ + rm -f $@ + tmpdir=$(@:.a=.objs) && rm -rf $$tmpdir && mkdir -p $$tmpdir && \ + for a in $(LIBS); do (cd $$tmpdir && ar x $$a); done && \ + ar rcsTPD $@ $$tmpdir/*.o && ranlib $@ && rm -rf $$tmpdirAlternatively, use an MRI script with
ar -Mandaddlibfor each archive.
🧹 Nitpick comments (4)
libtcgtpm/Makefile (3)
173-181: Force the Makefiles generator to match subsequentmake -Cusage.On systems defaulting to Ninja (via env or CMake prefs),
$(TCGTPM_BUILD_DIR)/Makefilewon’t exist andmake -Cwill fail. Pin the generator.- cmake \ + cmake -G "Unix Makefiles" \ -DCMAKE_C_FLAGS="${TCGTPM_CFLAGS}" \
156-167: Make parallelism probe more portable.
nprocmay be unavailable on some distros/containers. Use a fallback.- $(MAKE) -j$$(nproc) -C $(TCGTPM_BUILD_DIR) $(LIBBNMATH_TARGET) + $(MAKE) -j$$(getconf _NPROCESSORS_ONLN 2>/dev/null || nproc || echo 1) -C $(TCGTPM_BUILD_DIR) $(LIBBNMATH_TARGET)Apply similarly to other
-j$$(nproc)calls.
31-33: Consider out-of-tree build dir to avoid polluting submodule.Placing the CMake build under the TPM source tree can dirty the vendored code. Consider
TCGTPM_BUILD_DIR := $(DEPS_DIR)/.build/tpm-20-ref/TPMCmd(or similar).Documentation/docs/installation/INSTALL.md (1)
206-208: Add minimum CMake version (3.16.3) to INSTALL.md
Specify “CMake ≥ 3.16.3” in Documentation/docs/installation/INSTALL.md (lines 206–208) to match the top-level CMakeLists.txt.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/publish-docs.yml(1 hunks).github/workflows/qemu.yml(1 hunks).github/workflows/rust.yml(1 hunks)Documentation/docs/installation/INSTALL.md(1 hunks)libtcgtpm/Makefile(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/workflows/rust.yml
- .github/workflows/publish-docs.yml
- .github/workflows/qemu.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Check
- GitHub Check: Sourcery review
- GitHub Check: QEMU NOCC test-in-svsm
1 participant
Summary by Sourcery
Migrate libtcgtpm’s TPM reference implementation build to CMake with explicit library targets and integrate a new vendor command framework.
New Features:
Enhancements:
CI:
Documentation:
Summary by CodeRabbit
Chores
Documentation