Re: [PATCH v2 1/5] tpm: introduce tpm_pcr_algorithms()
From: Roberto Sassu
Date: Tue May 30 2017 - 04:45:25 EST
On 5/24/2017 7:35 PM, Jarkko Sakkinen wrote:
On Mon, May 22, 2017 at 11:07:54AM +0200, Roberto Sassu wrote:
On 5/20/2017 3:18 PM, Jarkko Sakkinen wrote:
On Wed, May 17, 2017 at 10:42:35AM +0200, Roberto Sassu wrote:
On 5/15/2017 3:18 PM, Roberto Sassu wrote:
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:
This is not a viable option. chip could be freed and the reference
becomes invalid, without increasing the reference count.
Did you think about something different?
Thanks
Roberto
Two alternatives come in mind.
1. add tpm_put to linclude/linux/tpm.h
2. take put_ops away from the implementation
3. add documentation that tpm_put needs to be called
or acceptable alternative would memcpy in the implementation
In any case, you should probably drop completely 'count' and have
function for getting the number of active banks.
Having a variable number, makes things more complicated.
In the IMA patch set for multiple template digests,
the array of algorithms is stored inside a structure
called digest descriptor. The array size must be fixed,
to avoid VLAIS (which is not supported by clang).
What is VLAI?
Variable-length arrays in structures.
You can always allocate it from heap.
The algorithms to be used by IMA are set in a __setup function.
Since the size of tpm_chip->active_banks is a small number,
we can drop count, and assume that the size of the array
passed to tpm_pcr_algorithms() always matches the size
of the active_banks array.
Roberto
I don't understand what you mean.
I meant that callers of tpm_pcr_algorithms() can always pass
an array whose size is the same of that of tpm_chip->active_banks.
Since the array size is known, passing count is not necessary.
Roberto