tpmdd-devel.lists.sourceforge.net archive mirror
 help / color / mirror / Atom feed
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: Wed, 17 May 2017 10:42:35 +0200	[thread overview]
Message-ID: <8aacf9c7-ac99-578e-ca60-6bb45bf3ab2b@huawei.com> (raw)
In-Reply-To: <d2974dd2-e30d-e8f0-a60a-2147f3290670-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>

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

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

  parent reply	other threads:[~2017-05-17  8:42 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 [this message]
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
     [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=8aacf9c7-ac99-578e-ca60-6bb45bf3ab2b@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).