Re: [RFC PATCH v2 07/13] tpm: enable bank selection for PCR extend

[Nicolai Stange]

Mimi Zohar <zohar@xxxxxxxxxxxxx> writes:

> On Sun, 2025-03-23 at 15:09 +0100, Nicolai Stange wrote:
>
>> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
>> index dfdcbd009720..23ded8ea47dc 100644
>> --- a/drivers/char/tpm/tpm2-cmd.c
>> +++ b/drivers/char/tpm/tpm2-cmd.c
>> @@ -226,16 +226,34 @@ int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
>>   * @chip:	TPM chip to use.
>>   * @pcr_idx:	index of the PCR.
>>   * @digests:	list of pcr banks and corresponding digest values to extend.
>> + * @banks_skip_mask:	pcr banks to skip
>>   *
>>   * Return: Same as with tpm_transmit_cmd.
>>   */
>>  int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
>> -		    struct tpm_digest *digests)
>> +		    struct tpm_digest *digests,
>> +		    unsigned long banks_skip_mask)
>>  {
>>  	struct tpm_buf buf;
>> +	unsigned long skip_mask;
>> +	u32 banks_count;
>>  	int rc;
>>  	int i;
>>  
>> +	banks_count = 0;
>> +	skip_mask = banks_skip_mask;
>> +	for (i = 0; i < chip->nr_allocated_banks; i++) {
>> +		const bool skip_bank = skip_mask & 1;
>> +
>> +		skip_mask >>= 1;
>> +		if (skip_bank)
>> +			continue;
>> +		banks_count++;
>> +	}
>
> Setting ima_unsupported_pcr_banks_mask used BIT(i).  Testing the bit should be
> as straight forward here and below.

I opted for not to using BIT(i) here because in theory
->nr_allocated_banks could be > BITS_PER_LONG. Not in practice though,
but I felt it would improve code readabily if there aren't any implict
assumptions. Also I'm not sure static checkers wouldn't complain about
for (i = 0; i < a; i++) {  1ul << i; }

Anyway, I'm realizing now that the code above is effectively just a
popcnt implementation on the lower bits of ~banks_skip_mask.

IMO it would be perhaps even better to do

	unsigned long skipped_banks_count, banks_count;

	skipped_banks_count = 0;
	skip_mask = banks_skip_mask;
	for (i = 0; skip_mask && i < chip->nr_allocated_banks; i++) {
        	skipped_banks_count += skip_mask & 1;
                skip_mask >>= 1;
	}
        banks_count = chip->nr_allocated_banks - skipped_banks_count;

instead. That way it's almost a nop in the common case of a clear
banks_skip_mask, plus there are no conditionals in the body.


> The first TPM extend after boot is the boot_aggregate.  Afterwards the number of
> banks being extended should always be the same.  Do we really need to re-
> calculate the number of banks needing to be extended each time?
>

Otherwise the number of banks to skip would have to get stored somewhere
and passed around, IIUC. I don't think that's worth it, the total number
of allocated banks is expected to be relatively small and
banks_skip_mask is zero in the common case anyway.

Thanks!

Nicolai

-- 
SUSE Software Solutions Germany GmbH, Frankenstraße 146, 90461 Nürnberg, Germany
GF: Ivo Totev, Andrew McDonald, Werner Knoblich
(HRB 36809, AG Nürnberg)