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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87tt7gp1sg.fsf@>
Date: Wed, 26 Mar 2025 10:41:03 +0100
From: Nicolai Stange <nstange@...e.de>
To: Mimi Zohar <zohar@...ux.ibm.com>
Cc: Nicolai Stange <nstange@...e.de>,  Roberto Sassu
 <roberto.sassu@...wei.com>,  Dmitry Kasatkin <dmitry.kasatkin@...il.com>,
  Eric Snowberg <eric.snowberg@...cle.com>,  Jarkko Sakkinen
 <jarkko@...nel.org>,  James Bottomley
 <James.Bottomley@...senPartnership.com>,  linux-integrity@...r.kernel.org,
  linux-security-module@...r.kernel.org,  linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH v2 07/13] tpm: enable bank selection for PCR extend

Mimi Zohar <zohar@...ux.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)