From: Roberto Sassu <roberto.sassu-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
To: Jarkko Sakkinen
<jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Cc: linux-ima-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org,
linux-security-module-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org,
keyrings-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Subject: Re: [PATCH v2 1/5] tpm: introduce tpm_pcr_algorithms()
Date: Tue, 30 May 2017 10:44:50 +0200 [thread overview]
Message-ID: <1cf4fbb1-5683-ceb4-2d5b-8ade25a91e83@huawei.com> (raw)
In-Reply-To: <20170524173521.yiiohct7brkcozyk-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
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-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>>>>>>> ---
>>>>>>> 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
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
next prev parent reply other threads:[~2017-05-30 8:44 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-05-05 14:21 [PATCH v2 0/5] Updated API for TPM 2.0 PCR extend Roberto Sassu
2017-05-05 14:21 ` [PATCH v2 1/5] tpm: introduce tpm_pcr_algorithms() Roberto Sassu
2017-05-15 10:36 ` Jarkko Sakkinen
[not found] ` <20170515103623.sumyo2vyldezual2-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
2017-05-15 13:18 ` Roberto Sassu
[not found] ` <d2974dd2-e30d-e8f0-a60a-2147f3290670-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
2017-05-17 8:42 ` Roberto Sassu
2017-05-20 13:18 ` Jarkko Sakkinen
[not found] ` <20170520131846.e3niqiknlrttbdf4-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
2017-05-22 9:07 ` Roberto Sassu
2017-05-24 17:35 ` Jarkko Sakkinen
[not found] ` <20170524173521.yiiohct7brkcozyk-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
2017-05-30 8:44 ` Roberto Sassu [this message]
[not found] ` <1cf4fbb1-5683-ceb4-2d5b-8ade25a91e83-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
2017-05-31 11:39 ` Jarkko Sakkinen
2017-05-20 13:00 ` Jarkko Sakkinen
2017-05-05 14:21 ` [PATCH v2 2/5] tpm: introduce tpm_pcr_algo_to_crypto() and tpm_pcr_algo_from_crypto() Roberto Sassu
[not found] ` <20170505142152.29795-3-roberto.sassu-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
2017-05-15 11:16 ` Jarkko Sakkinen
[not found] ` <20170515111629.urjvbhqzohv4vakc-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
2017-05-15 14:22 ` Roberto Sassu
2017-05-20 13:22 ` Jarkko Sakkinen
[not found] ` <20170520132217.t7n7l2pjn7i63hbm-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
2017-05-22 7:21 ` Roberto Sassu
2017-05-24 17:33 ` Jarkko Sakkinen
2017-05-24 20:25 ` Jarkko Sakkinen
2017-05-30 10:24 ` Roberto Sassu
2017-05-31 11:40 ` Jarkko Sakkinen
2017-05-05 14:21 ` [PATCH v2 3/5] tpm: pass multiple digests to tpm_pcr_extend() Roberto Sassu
[not found] ` <20170505142152.29795-4-roberto.sassu-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
2017-05-30 3:29 ` [Linux-ima-devel] " Mimi Zohar
2017-05-30 7:28 ` Roberto Sassu
2017-05-30 11:25 ` Mimi Zohar
[not found] ` <1496143547.3841.517.camel-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
2017-05-30 13:25 ` Roberto Sassu
2017-05-05 14:21 ` [PATCH v2 4/5] keys, trusted: modify arguments of tpm_pcr_extend() Roberto Sassu
2017-05-30 3:35 ` [Linux-ima-devel] " Mimi Zohar
[not found] ` <1496115337.3841.485.camel-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
2017-05-30 7:36 ` Roberto Sassu
2017-05-30 12:06 ` Mimi Zohar
[not found] ` <1496145961.3841.537.camel-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
2017-05-30 13:41 ` Roberto Sassu
2017-05-05 14:21 ` [PATCH v2 5/5] ima: " Roberto Sassu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1cf4fbb1-5683-ceb4-2d5b-8ade25a91e83@huawei.com \
--to=roberto.sassu-hv44wf8li93qt0dzr+alfa@public.gmane.org \
--cc=jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org \
--cc=keyrings-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
--cc=linux-ima-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org \
--cc=linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
--cc=linux-security-module-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
--cc=tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).