Re: [PATCH] ima: correctly recover number of violations after kexec

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