Re: [PATCH v2 1/5] tpm: introduce tpm_pcr_algorithms()

[Roberto Sassu]

On 5/15/2017 12:36 PM, Jarkko Sakkinen wrote:
> On Fri, May 05, 2017 at 04:21:48PM +0200, Roberto Sassu wrote:
> > This function allows TPM users to know which algorithms the TPM supports.
> > It stores the algorithms in a static array of 'enum tpm2_algorithms',
> > allocated by the caller. If the array is not large enough, the function
> > returns an error. Otherwise, it returns the number of algorithms written
> > to the array. If the TPM version is 1.2, the function writes TPM2_ALG_SHA1
> > to first element of the array.
> >
> > Writing the algorithm also for TPM 1.2 has the advantage that callers
> > can use the API, tpm_pcr_algorithms() and tpm_pcr_extend(), regardless
> > of the TPM version.
> >
> > A minor change added to this patch was to make available the size of
> > the active_banks array, member of the tpm_chip structure, outside
> > the TPM driver. The array size (TPM_ACTIVE_BANKS_MAX) has been exported
> > to include/linux/tpm.h.
> >
> > With this information, callers of tpm_pcr_algorithms() can provide
> > a static array with enough room for all the algorithms, instead
> > of receiving the pointer of a dynamic array that they have to free later.
> >
> > Signed-off-by: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
> > ---
> > v2
> >
> > - tpm_pcr_algorithms() returns supported algorithms also for TPM 1.2
> >
> >  drivers/char/tpm/tpm-interface.c | 46 ++++++++++++++++++++++++++++++++++++++++
> >  drivers/char/tpm/tpm.h           | 13 +-----------
> >  include/linux/tpm.h              | 19 +++++++++++++++++
> >  3 files changed, 66 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> > index 4ed08ab..b90de3d 100644
> > --- a/drivers/char/tpm/tpm-interface.c
> > +++ b/drivers/char/tpm/tpm-interface.c
> > @@ -911,6 +911,52 @@ int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash)
> >  EXPORT_SYMBOL_GPL(tpm_pcr_extend);
> >
> >  /**
> > + * tpm_pcr_algorithms - get TPM IDs of active PCR banks algorithms
>
> The grammar is incorrect here I believe. Should rather be
>
>   "algorithms of the active PCR banks"
>
> And there is no such thing as "TPM ID".
>
> > + * @chip_num:	tpm idx # or ANY
> > + * @count:	# of items in algorithms
> > + * @algorithms:	array of TPM IDs
> > + *
> > + * Returns < 0 on error, and the number of active PCR banks on success.
> > + *
> > + * TPM 1.2 has always one SHA1 bank.
> > + */
> > +int tpm_pcr_algorithms(u32 chip_num, int count,
> > +		       enum tpm2_algorithms *algorithms)
>                        unsigned int
>
> In addition the function name is not too greatg,
>
> Your syntax for return value is not correct. In addition after
> describing the return value there should not be anything. You should
> study
>
> https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt
>
> Better name for the function would be tpm_get_pcr_algorithms().
>
> > +{
> > +	struct tpm_chip *chip;
> > +	int i;
> > +
> > +	if (count <= 0 || algorithms == NULL)
> > +		return -EINVAL;
>
> Is there a legal case where caller would pass these values? Now it
> looks like that there is.
>
> 'count' should unsigned int and zero should be a legal value for
> count.

I wanted to avoid that a negative value returned by tpm_pcr_algorithms()
is passed to tpm_pcr_extend() as unsigned int.


> That said I think the whole design is wrong as you could calculate
> array for algs only one time and pass a const reference to it on
> request.

Ok. If I understood it correctly, you are saying to pass a const
reference of chip->active_banks. Then, I should also:

- add a new field (e.g. active_banks_num) in the tpm_chip structure
  to store the number of algorithms supported by the TPM (unless,
  I should determine it every time tpm_pcr_algorithms() is called)
- initialize chip->active_banks and chip->active_banks_num
  respectively to TPM2_ALG_SHA1 and 1 in tpm1_auto_startup()

Roberto