Hi Roberto, On Thu, 2026-06-25 at 15:10 +0200, Roberto Sassu wrote: > On Fri, 2026-06-19 at 21:14 +0200, Enrico Bravi wrote: > > When recovering the measurement list after kexec(), the number of > > violations is not recovered as well, causing a mismatch between the > > number reported by the <securityfs>/ima/violations user interface and > > the actual value. In addition, currently it is assumed that when > > recovering an entry, this is a violation if the template data hash > > read from the kexec buffer is an all-zero hash, which can actually be a > > valid hash. > > This sentence is a bit convoluted. Please rework it. > > > Verify that an all-zero hash corresponds to a violation and consequently > > correctly recover the number of violations. > > We need to clarify that this patch only fixes false positives (a record > is declared as a violation even if it isn't, and we fix it by cross- > checking the SHA1 of the template data). > > However, we are not fixing the false negatives (if the SHA1 of the > template data is zeros, we don't declare it as a violation, but it can > potentially be).
thank you very much for your feedback. I'll rework the patch description to make it more clear and describe what you highlighted. > > Reported-by: Roberto Sassu <[email protected]> > > Closes: https://github.com/linux-integrity/linux/issues/13 > > Signed-off-by: Enrico Bravi <[email protected]> > > > > --- > > security/integrity/ima/ima_template.c | 28 ++++++++++++++++++++------- > > 1 file changed, 21 insertions(+), 7 deletions(-) > > > > diff --git a/security/integrity/ima/ima_template.c > > b/security/integrity/ima/ima_template.c > > index 7034573fb41e..147f228ed246 100644 > > --- a/security/integrity/ima/ima_template.c > > +++ b/security/integrity/ima/ima_template.c > > @@ -430,6 +430,7 @@ int ima_restore_measurement_list(loff_t size, void *buf) > > DECLARE_BITMAP(hdr_mask, HDR__LAST); > > unsigned long count = 0; > > int ret = 0; > > + int i; > > > > if (!buf || size < sizeof(*khdr)) > > return 0; > > @@ -515,15 +516,28 @@ int ima_restore_measurement_list(loff_t size, void > > *buf) > > if (ret < 0) > > break; > > > > - if (memcmp(hdr[HDR_DIGEST].data, zero, sizeof(zero))) { > > - ret = ima_calc_field_array_hash( > > - &entry->template_data[0], > > + ret = ima_calc_field_array_hash(&entry->template_data[0], > > entry); > > - if (ret < 0) { > > - pr_err("cannot calculate template > > digest\n"); > > - ret = -EINVAL; > > - break; > > + if (ret < 0) { > > + pr_err("cannot calculate template digest\n"); > > + ret = -EINVAL; > > + break; > > + } > > + > > + if (!memcmp(hdr[HDR_DIGEST].data, zero, sizeof(zero)) && > > + memcmp(entry->digests[ima_sha1_idx].digest, zero, > > sizeof(zero))) { > > Here it would be helpful to have a comment saying that we are reverting > the effect of ima_calc_field_array_hash(): we tried to verify if the > entry is a violation, it is, but then we have to set the template > digest back to zero after the calculation. Sure, will add it in the next version. > > + for (i = 0; i < NR_BANKS(ima_tpm_chip) + > > ima_extra_slots; i++) { > > + /* Unmapped TPM algorithms */ > > + if (!ima_algo_array[i].tfm) { > > + memset(entry->digests[i].digest, 0, > > + TPM_DIGEST_SIZE); > > + continue; > > + } > > + > > + memset(entry->digests[i].digest, 0, > > + ima_algo_array[i].digest_size); > > Maybe we can just use the instruction above instead of differentiating > for unmapped algorithms? digest_size is taken from the TPM in that > case. Yes you're right. It should not be necessary to treat unmapped algorithms differently. I was thinking that it could be directly used TPM2_MAX_DIGEST_SIZE instead. What do you think? Enrico > Thanks > > Roberto > > > } > > + atomic_long_inc(&ima_htable.violations); > > } > > > > entry->pcr = !ima_canonical_fmt ? *(u32 > > *)(hdr[HDR_PCR].data) : > > > > base-commit: 8cd9520d35a6c38db6567e97dd93b1f11f185dc6
