Mimi Zohar <zo...@linux.ibm.com> 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)
