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

[James Bottomley <James.Bottomley@HansenPartnership.com>]

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