linux-kernel - Re: [PATCH v2 1/5] tpm: introduce tpm_pcr

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: <20170515103623.sumyo2vyldezual2@intel.com>
Date:   Mon, 15 May 2017 13:36:23 +0300
From:   Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>
To:     Roberto Sassu <roberto.sassu@...wei.com>
Cc:     tpmdd-devel@...ts.sourceforge.net,
        linux-ima-devel@...ts.sourceforge.net,
        linux-security-module@...r.kernel.org, keyrings@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/5] tpm: introduce tpm_pcr_algorithms()

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@...wei.com>
> ---
> 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.

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.

/Jarkko