Re: [PATCH v2 1/1] tpm: add sysfs exports for all banks of PCR registers

[Jerry Snitselaar <jsnitsel@redhat.com>]

James Bottomley @ 2020-07-21 17:39 MST:

> On Tue, 2020-07-21 at 17:02 -0700, Jerry Snitselaar wrote:
>> James Bottomley @ 2020-07-21 16:37 MST:
>> 
>> > On Tue, 2020-07-21 at 16:16 -0700, Jerry Snitselaar wrote:
>> > > James Bottomley @ 2020-07-21 08:56 MST:
>> > 
>> > [...]
>> > > > +	/*
>> > > > +	 * This will only trigger if someone has added an
>> > > > additional
>> > > > +	 * hash to the tpm_algorithms enum without
>> > > > incrementing
>> > > > +	 * TPM_MAX_HASHES.  This has to be a BUG_ON because
>> > > > under
>> > > > this
>> > > > +	 * condition, the chip->groups array will overflow
>> > > > corrupting
>> > > > +	 * the chips structure.
>> > > > +	 */
>> > > > +	BUG_ON(chip->groups_cnt > TPM_MAX_HASHES);
>> > > 
>> > > Should this check be 3 + TPM_MAX_HASHES like below?
>> > 
>> > No, because at this point only a single additional group has been
>> > addedin addition to the hashes groups.  The first line of
>> > tpm_sysfs_add_device is
>> > 
>> > 	WARN_ON(chip->groups_cnt != 0);
>> > 
>> > And then we add the unnamed group.  This loop over the banks
>> > follows it, so chip->groups_cnt should be nr_banks_allocated by the
>> > end (it's the index, which is one fewer than the number of entries
>> > in chip->groups[]).  We have a problem if nr_banks_allocated >
>> > TPM_MAX_HASHES
>> > 
>> > which is what the BUG_ON checks.
>> > 
>> > James
>> 
>> If the chip supported all 5 listed cases wouldn't groups_cnt be 6 at
>> this point?
>
> Actually, yes, I think it would be because it's pointing at the next
> free index not the current one.  So it should be BUG_ON (chip-
>>groups_cnt > TPM_MAX_HASHES + 1)
>
> James

One other thought, should a note be added above tpm_algorithms to note
that when that is changed TPM_MAX_HASHES should be changed as well?

With the above change to the BUG_ON you can add to v3:

Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>