[sam-kota]

Clean-up: USER_NVRAM_Unmarshal() updates the entrysize of a TPM_HT_NV_INDEX entry to sizeof(UINT32) + sizeof(nvi) + datasize only inside the datasize > 0 branch, so an index without data keeps the entrysize value read from the state. This moves the update out of that branch so it's done for every successfully unmarshalled NV index, the same way the TPM_HT_PERSISTENT branch already does it. In the failure case entrysize is left at its old value.

Summary by CodeRabbit

• Bug Fixes
  • Improved NVRAM data handling during unmarshalling.
  • NVRAM entry sizes are now updated only after successful processing, helping prevent inconsistent stored data when errors occur.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: 84b143e1-fdf5-4676-b6d9-bb8fc998eb56

📥 Commits

Reviewing files that changed from the base of the PR and between 1090aca and b04f394.

📒 Files selected for processing: 1

• src/tpm2/NVMarshal.c

Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review.

📝 Walkthrough

USER_NVRAM_Unmarshal now recalculates the NVRAM entry size only after successful unmarshalling. The nonzero-data write path is separate from the entry-size update.

Changes: NV marshal entry-size handling

Layer / File(s)	Summary
Guard entry-size recalculation src/tpm2/NVMarshal.c	The code closes the nonzero-data processing block before recalculating the NVRAM entry size. The recalculation runs only when unmarshalling succeeds.

Priority: ➖ Normal

Estimated code review effort: 1 (Trivial) | ~2 minutes

Change: Bug fix

Suggested reviewers: stefanberger

Merge Risk: ⚪ Minimal · up to b04f3

No actionable merge-blocking risk remains in the reviewed change.

Security architecture risk: ⚪ Minimal · up to b04f3

The change prevents successfully loaded zero-data NV entries from retaining an untrusted size. It preserves the existing storage layout and loading interfaces, and the review identified no security risk introduced or materially worsened by this change.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The relevant exposure requires control of state supplied through a library state-loading or callback-loading path. The directly affected asset is the loaded TPM instance's dynamic NVRAM image; this PR adds no caller, authority, or cross-instance access path.

Security Findings and Attack Paths

• inferred — At base, a forged nonzero size on a zero-data NV-index entry could survive successful unmarshalling and influence subsequent traversal or deletion. Head replaces that value with the parsed layout size before completing the entry, closing this path for successfully parsed entries.

Trust Boundaries and Controls

• observed — Existing maximum data-length and destination-capacity checks precede recomputation. Parsing or data-write failures prevent the final corrected-size write. Repeated successful loading derives the same canonical size from the same entry layout.

Resilience and Maintainability Implications

• observed — State loading remains non-transactional: entry fields are written before full validation, and failure cleanup clears cached blobs without restoring those NVRAM writes. This behavior exists at base and is not an introduced or worsened PR concern. External backend rollback and concurrent-access guarantees remain unverified.

Pre-merge checks: 5

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage		Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.
Title check		The title clearly and concisely describes the main code change: updating the NV index entry size only after successful unmarshalling in USER_NVRAM_Unmarshal.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[stefanberger]

Thanks for doing this. However, I suppose it's still true that if rc != TPM_RC_SUCCESS, we will leave the function without looking at entrysize again. So it doesn't matter whether it was changed in the failure case before your patch. However, it's probably better to leave it at is old value (in failure case).

[stefanberger]

-> I think you should describe this patch as a clean-up.

[sam-kota]

makes sense. reworded the commit and the PR description as a clean-up, and put the original comment back so the diff is just moving the update out of the datasize > 0 branch. the rc == TPM_RC_SUCCESS guard stays, so entrysize keeps its old value on failure.

[stefanberger]

I think the title of the patch is not correct. It should be more like 'Do not always update...' or 'Only update ... on success'

[sam-kota]

fair, retitled it to 'tpm2: Only update NV index entry size on success in USER_NVRAM_Unmarshal' and updated the PR title to match. the code is unchanged.

[stefanberger]

Thanks!