Re: [PATCH RFC 3/3] security: IMA: use TSM measurement registers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] ima: buffer capacity check bypass in TSM MR read/write operations
- [Medium] ima: implicit assumption of uniform MR sizes across a TSM bank
- [Low] ima: typographical error tgx instead of tdx in function name
--

commit 2a879775437b1a2234f6a7ecbc56b85ccdc11baf
Author: Yeoreum Yun <yeoreum.yun@arm.com>

security: IMA: use TSM measurement registers

This commit adds support for extending IMA measurement digests into Trusted
Security Module (TSM) measurement registers. This serves as an alternative
for guests when no TPM device is available. Mappings are provided for Intel
TDX and Arm CCA.

Link: [0] https://uefi.org/specs/UEFI/2.11/38_Confidential_Computing.html#intel-trust-domain-extension
Link: [1] https://github.com/tianocore/edk2/issues/11383

> diff --git a/security/integrity/ima/ima_mr_tsm.c b/security/integrity/ima/ima_mr_tsm.c
> new file mode 100644
> index 0000000000000..3f88edfb8511c
> --- /dev/null
> +++ b/security/integrity/ima/ima_mr_tsm.c
[ ... ]
> +static int create_tsm_tgx_pcr_map(struct tsm_context *ctx)

[Severity: Low]
This isn't a bug, but is tgx here a typographical error for tdx (Intel
Trust Domain Extensions)?

> +{
> +	int mrtd_idx, rtmr0_idx, rtmr1_idx, rtmr2_idx;
[ ... ]
> +static int tsm_mr_calc_boot_aggregate(struct ima_mr *mr, int bank,
> +				      char *digest, struct crypto_shash *tfm)
> +{
[ ... ]
> +	for (pcr_idx = TPM_PCR0; pcr_idx <= TPM_PCR2; pcr_idx++) {
> +		tsm_mr = tsm_mr_get(ctx, pcr_idx);
> +		if (!tsm_mr)
> +			return -ENODEV;
> +
> +		mr_idx = tsm_mr_idx(ctx->tm, tsm_mr);
> +		rc = tsm_mr_read(ctx->tm, mr_idx, d.digest, tsm_mr->mr_size);

[Severity: Medium]
Does passing tsm_mr->mr_size instead of the actual buffer capacity bypass
internal bounds checking?

If a future TSM architecture implements an mr_size larger than the
statically sized d.digest buffer (which is bounded to TPM2_MAX_DIGEST_SIZE),
passing the hardware-defined size rather than sizeof(d.digest) could
result in a stack buffer overflow.

> +		if (rc) {
> +			pr_err("Error Communicating to TSM(%d)\n", rc);
> +			return rc;
> +		}
> +
> +		/* now accumulate with current aggregate */
> +		rc = crypto_shash_update(shash, d.digest,
> +					 crypto_shash_digestsize(tfm));

[Severity: Medium]
Does this code assume that all TSM measurement registers mapped to this bank
share the exact same digest size as TPM_PCR0?

The bank information is initialized using PCR0's hash algorithm and size.
If a future TSM architecture were to use heterogeneous MR sizes, and a
subsequent register had a smaller size than PCR0, could tsm_mr_read() only
partially overwrite d.digest?

This would leave stale bytes from previous loop iterations in the buffer.
These stale bytes would then be hashed by crypto_shash_update(), since it
uses the larger size returned by crypto_shash_digestsize(tfm), which could
silently corrupt the boot aggregate.

> +		if (rc)
> +			return rc;
> +	}
[ ... ]
> +	if (d.alg_id != TPM_ALG_SHA1) {
> +		tsm_mr = tsm_mr_get(ctx, TPM_PCR8);
> +		if (!tsm_mr)
> +			return -ENODEV;
> +
> +		mr_idx = tsm_mr_idx(ctx->tm, tsm_mr);
> +		rc = tsm_mr_read(ctx->tm, mr_idx, d.digest, tsm_mr->mr_size);

[Severity: Medium]
Does this second instance also neutralize the capacity bounds check by
passing the hardware-defined mr_size instead of the destination buffer size?

> +		if (rc) {
> +			pr_err("Error Communicating to TSM(%d)\n", rc);
> +			return rc;
> +		}
> +
> +		rc = crypto_shash_update(shash, d.digest,
> +					crypto_shash_digestsize(tfm));
> +	}
[ ... ]
> +static int tsm_mr_extend(struct ima_mr *mr, u32 pcr_idx,
> +			 mr_digest_t *digests)
> +{
[ ... ]
> +	mr_idx = tsm_mr_idx(ctx->tm, tsm_mr);
> +
> +	/* TSM has only one bank. */
> +	rc = tsm_mr_write(ctx->tm, mr_idx, digests[0].digest, tsm_mr->mr_size);

[Severity: Medium]
Could this lead to an out-of-bounds read if tsm_mr->mr_size exceeds
TPM2_MAX_DIGEST_SIZE?

Similar to the read operations above, using the hardware-defined register
size instead of the array bounds of digests[0].digest might cause the code
to read past the end of the allocated structure if a larger MR size is
encountered.

> +	if (rc)
> +		pr_err("Error Communicating to TSM, result: %d\n", rc);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930-ima_tgx_integration_v2-v1-0-722c35370548@arm.com?part=3