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

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

[Jarkko Sakkinen]

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@huawei.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

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@huawei.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.

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

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

[Roberto Sassu]

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@huawei.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.
>
> 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

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

[Jarkko Sakkinen]

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@huawei.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.
> > 
> > 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.

/Jarkko

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

[Roberto Sassu]

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@huawei.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.
>>>
>>> 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).

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

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

[Jarkko Sakkinen]

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@huawei.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.
> > > > 
> > > > 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?

You can always allocate it from heap.

> 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.


/Jarkko

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

[Roberto Sassu]

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@huawei.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.
>>>>>
>>>>> 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

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

[Jarkko Sakkinen]

On Tue, May 30, 2017 at 10:44:50AM +0200, Roberto Sassu wrote:
> 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@huawei.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.
> > > > > > 
> > > > > > 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

OK, got it.

/Jarkko

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

[Jarkko Sakkinen]

On Mon, May 15, 2017 at 03:18:41PM +0200, 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@huawei.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.
> 
> I wanted to avoid that a negative value returned by tpm_pcr_algorithms()
> is passed to tpm_pcr_extend() as unsigned int.

Why would you pass a negative value? I'm sorry but I have to admit tht I'm
seriously not computing your argument.

> > 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

Something along those lines.

Doing precalcuation makes the code more stable as you can only fall to
an error conditio when the driver is initialized.

/Jarkko

Re: [PATCH v2 2/5] tpm: introduce tpm_pcr_algo_to_crypto() and tpm_pcr_algo_from_crypto()

[Jarkko Sakkinen]

On Fri, May 05, 2017 at 04:21:49PM +0200, Roberto Sassu wrote:
> tpm_pcr_algorithms() returns to its callers the IDs of the hash algorithms
> supported by the TPM. This patch introduces tpm_pcr_algo_to_crypto(),
> so that the callers can use the crypto subsystem to calculate the digest
> to be passed to tpm_pcr_extend().
> 
> tpm_pcr_algo_from_crypto(), implemented for completeness, is instead used

What do you mean by completeness? Please, never add unused stuff.

> by tpm2_seal_trusted() to perform the opposite conversion.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
> v2
> 
> - fixed return values of tpm2_pcr_algo_to_crypto() and
>   tpm2_pcr_algo_from_crypto() if TPM support is disabled in the kernel

Change Log only to the cover letter.

>  drivers/char/tpm/tpm-interface.c | 51 ++++++++++++++++++++++++++++++++++++++++
>  drivers/char/tpm/tpm2-cmd.c      | 42 +++++++++------------------------
>  include/linux/tpm.h              | 13 ++++++++++
>  3 files changed, 75 insertions(+), 31 deletions(-)

This commit is just deadly wrong in so many ways.

I would suggest to make extend always just take crypto ID in so you
don't have to add these bizarre conversion functions.

This probably covers all the patches but just "unsigned int" as type
for parameters and local variables.

I think I'll pass rest of the patches for now because just reviewing the
two first took too much time and wait for an updated version.

/Jarkko

Re: [PATCH v2 2/5] tpm: introduce tpm_pcr_algo_to_crypto() and tpm_pcr_algo_from_crypto()

[Roberto Sassu]

On 5/15/2017 1:16 PM, Jarkko Sakkinen wrote:
> On Fri, May 05, 2017 at 04:21:49PM +0200, Roberto Sassu wrote:
>> tpm_pcr_algorithms() returns to its callers the IDs of the hash algorithms
>> supported by the TPM. This patch introduces tpm_pcr_algo_to_crypto(),
>> so that the callers can use the crypto subsystem to calculate the digest
>> to be passed to tpm_pcr_extend().
>>
>> tpm_pcr_algo_from_crypto(), implemented for completeness, is instead used
>
> What do you mean by completeness? Please, never add unused stuff.
>
>> by tpm2_seal_trusted() to perform the opposite conversion.
>>
>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>> ---
>> v2
>>
>> - fixed return values of tpm2_pcr_algo_to_crypto() and
>>   tpm2_pcr_algo_from_crypto() if TPM support is disabled in the kernel
>
> Change Log only to the cover letter.
>
>>  drivers/char/tpm/tpm-interface.c | 51 ++++++++++++++++++++++++++++++++++++++++
>>  drivers/char/tpm/tpm2-cmd.c      | 42 +++++++++------------------------
>>  include/linux/tpm.h              | 13 ++++++++++
>>  3 files changed, 75 insertions(+), 31 deletions(-)
>
> This commit is just deadly wrong in so many ways.
>
> I would suggest to make extend always just take crypto ID in so you
> don't have to add these bizarre conversion functions.

The reason of this choice (as I explained in the cover letter)
is that TPM users might want to produce an event log with
the TCG format (which includes the TPM algorithm ID). Also,
TPM IDs should be preferred because, with them, TPM users
can calculate a digest directly with the TPM.

Taking crypto IDs means relying on the fact that there
is always a mapping between TPM IDs and crypto IDs.
Otherwise, tpm_pcr_algorithms() cannot return the algorithms
to its callers and PCRs cannot be extended. If TPM IDs are used,
TPM users have two alternatives: calculate the digest with
the TPM, or pass a SHA1 digest to tpm_pcr_extend(), so that
it can be padded to extend remanining PCR banks.

However, this second option will work only when the TPM driver
determines the size of an algorithm without relying on the crypto
subsystem. At the moment, if a TPM ID is not mapped to a crypto ID,
tpm2_pcr_extend() ignores the digest.

Roberto

Re: [PATCH v2 2/5] tpm: introduce tpm_pcr_algo_to_crypto() and tpm_pcr_algo_from_crypto()

[Jarkko Sakkinen]

On Mon, May 15, 2017 at 04:22:22PM +0200, Roberto Sassu wrote:
> On 5/15/2017 1:16 PM, Jarkko Sakkinen wrote:
> > On Fri, May 05, 2017 at 04:21:49PM +0200, Roberto Sassu wrote:
> > > tpm_pcr_algorithms() returns to its callers the IDs of the hash algorithms
> > > supported by the TPM. This patch introduces tpm_pcr_algo_to_crypto(),
> > > so that the callers can use the crypto subsystem to calculate the digest
> > > to be passed to tpm_pcr_extend().
> > > 
> > > tpm_pcr_algo_from_crypto(), implemented for completeness, is instead used
> > 
> > What do you mean by completeness? Please, never add unused stuff.
> > 
> > > by tpm2_seal_trusted() to perform the opposite conversion.
> > > 
> > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > ---
> > > v2
> > > 
> > > - fixed return values of tpm2_pcr_algo_to_crypto() and
> > >   tpm2_pcr_algo_from_crypto() if TPM support is disabled in the kernel
> > 
> > Change Log only to the cover letter.
> > 
> > >  drivers/char/tpm/tpm-interface.c | 51 ++++++++++++++++++++++++++++++++++++++++
> > >  drivers/char/tpm/tpm2-cmd.c      | 42 +++++++++------------------------
> > >  include/linux/tpm.h              | 13 ++++++++++
> > >  3 files changed, 75 insertions(+), 31 deletions(-)
> > 
> > This commit is just deadly wrong in so many ways.
> > 
> > I would suggest to make extend always just take crypto ID in so you
> > don't have to add these bizarre conversion functions.
> 
> The reason of this choice (as I explained in the cover letter)
> is that TPM users might want to produce an event log with
> the TCG format (which includes the TPM algorithm ID). Also,
> TPM IDs should be preferred because, with them, TPM users
> can calculate a digest directly with the TPM.
> 
> Taking crypto IDs means relying on the fact that there
> is always a mapping between TPM IDs and crypto IDs.
> Otherwise, tpm_pcr_algorithms() cannot return the algorithms
> to its callers and PCRs cannot be extended. If TPM IDs are used,
> TPM users have two alternatives: calculate the digest with
> the TPM, or pass a SHA1 digest to tpm_pcr_extend(), so that
> it can be padded to extend remanining PCR banks.
> 
> However, this second option will work only when the TPM driver
> determines the size of an algorithm without relying on the crypto
> subsystem. At the moment, if a TPM ID is not mapped to a crypto ID,
> tpm2_pcr_extend() ignores the digest.
> 
> Roberto

What if tpm_pcr_algorithms would convert TPM IDs to crypto IDs?

Externally other subsystems would have to then deal with only crypto
IDs.

That would favor "memcpy" (or it would become a for loop) for 1/5.

/Jarkko

Re: [PATCH v2 2/5] tpm: introduce tpm_pcr_algo_to_crypto() and tpm_pcr_algo_from_crypto()

[Roberto Sassu]

On 5/20/2017 3:22 PM, Jarkko Sakkinen wrote:
> On Mon, May 15, 2017 at 04:22:22PM +0200, Roberto Sassu wrote:
>> On 5/15/2017 1:16 PM, Jarkko Sakkinen wrote:
>>> On Fri, May 05, 2017 at 04:21:49PM +0200, Roberto Sassu wrote:
>>>> tpm_pcr_algorithms() returns to its callers the IDs of the hash algorithms
>>>> supported by the TPM. This patch introduces tpm_pcr_algo_to_crypto(),
>>>> so that the callers can use the crypto subsystem to calculate the digest
>>>> to be passed to tpm_pcr_extend().
>>>>
>>>> tpm_pcr_algo_from_crypto(), implemented for completeness, is instead used
>>>
>>> What do you mean by completeness? Please, never add unused stuff.
>>>
>>>> by tpm2_seal_trusted() to perform the opposite conversion.
>>>>
>>>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>>>> ---
>>>> v2
>>>>
>>>> - fixed return values of tpm2_pcr_algo_to_crypto() and
>>>>   tpm2_pcr_algo_from_crypto() if TPM support is disabled in the kernel
>>>
>>> Change Log only to the cover letter.
>>>
>>>>  drivers/char/tpm/tpm-interface.c | 51 ++++++++++++++++++++++++++++++++++++++++
>>>>  drivers/char/tpm/tpm2-cmd.c      | 42 +++++++++------------------------
>>>>  include/linux/tpm.h              | 13 ++++++++++
>>>>  3 files changed, 75 insertions(+), 31 deletions(-)
>>>
>>> This commit is just deadly wrong in so many ways.
>>>
>>> I would suggest to make extend always just take crypto ID in so you
>>> don't have to add these bizarre conversion functions.
>>
>> The reason of this choice (as I explained in the cover letter)
>> is that TPM users might want to produce an event log with
>> the TCG format (which includes the TPM algorithm ID). Also,
>> TPM IDs should be preferred because, with them, TPM users
>> can calculate a digest directly with the TPM.
>>
>> Taking crypto IDs means relying on the fact that there
>> is always a mapping between TPM IDs and crypto IDs.
>> Otherwise, tpm_pcr_algorithms() cannot return the algorithms
>> to its callers and PCRs cannot be extended. If TPM IDs are used,
>> TPM users have two alternatives: calculate the digest with
>> the TPM, or pass a SHA1 digest to tpm_pcr_extend(), so that
>> it can be padded to extend remanining PCR banks.
>>
>> However, this second option will work only when the TPM driver
>> determines the size of an algorithm without relying on the crypto
>> subsystem. At the moment, if a TPM ID is not mapped to a crypto ID,
>> tpm2_pcr_extend() ignores the digest.
>>
>> Roberto
>
> What if tpm_pcr_algorithms would convert TPM IDs to crypto IDs?
>
> Externally other subsystems would have to then deal with only crypto
> IDs.

Then, other subsystems cannot:

- use the TPM to calculate a digest
- create an event log with the TCG format

Roberto

Re: [PATCH v2 2/5] tpm: introduce tpm_pcr_algo_to_crypto() and tpm_pcr_algo_from_crypto()

[Jarkko Sakkinen]

On Mon, May 22, 2017 at 09:21:28AM +0200, Roberto Sassu wrote:
> On 5/20/2017 3:22 PM, Jarkko Sakkinen wrote:
> > On Mon, May 15, 2017 at 04:22:22PM +0200, Roberto Sassu wrote:
> > > On 5/15/2017 1:16 PM, Jarkko Sakkinen wrote:
> > > > On Fri, May 05, 2017 at 04:21:49PM +0200, Roberto Sassu wrote:
> > > > > tpm_pcr_algorithms() returns to its callers the IDs of the hash algorithms
> > > > > supported by the TPM. This patch introduces tpm_pcr_algo_to_crypto(),
> > > > > so that the callers can use the crypto subsystem to calculate the digest
> > > > > to be passed to tpm_pcr_extend().
> > > > > 
> > > > > tpm_pcr_algo_from_crypto(), implemented for completeness, is instead used
> > > > 
> > > > What do you mean by completeness? Please, never add unused stuff.
> > > > 
> > > > > by tpm2_seal_trusted() to perform the opposite conversion.
> > > > > 
> > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > ---
> > > > > v2
> > > > > 
> > > > > - fixed return values of tpm2_pcr_algo_to_crypto() and
> > > > >   tpm2_pcr_algo_from_crypto() if TPM support is disabled in the kernel
> > > > 
> > > > Change Log only to the cover letter.
> > > > 
> > > > >  drivers/char/tpm/tpm-interface.c | 51 ++++++++++++++++++++++++++++++++++++++++
> > > > >  drivers/char/tpm/tpm2-cmd.c      | 42 +++++++++------------------------
> > > > >  include/linux/tpm.h              | 13 ++++++++++
> > > > >  3 files changed, 75 insertions(+), 31 deletions(-)
> > > > 
> > > > This commit is just deadly wrong in so many ways.
> > > > 
> > > > I would suggest to make extend always just take crypto ID in so you
> > > > don't have to add these bizarre conversion functions.
> > > 
> > > The reason of this choice (as I explained in the cover letter)
> > > is that TPM users might want to produce an event log with
> > > the TCG format (which includes the TPM algorithm ID). Also,
> > > TPM IDs should be preferred because, with them, TPM users
> > > can calculate a digest directly with the TPM.
> > > 
> > > Taking crypto IDs means relying on the fact that there
> > > is always a mapping between TPM IDs and crypto IDs.
> > > Otherwise, tpm_pcr_algorithms() cannot return the algorithms
> > > to its callers and PCRs cannot be extended. If TPM IDs are used,
> > > TPM users have two alternatives: calculate the digest with
> > > the TPM, or pass a SHA1 digest to tpm_pcr_extend(), so that
> > > it can be padded to extend remanining PCR banks.
> > > 
> > > However, this second option will work only when the TPM driver
> > > determines the size of an algorithm without relying on the crypto
> > > subsystem. At the moment, if a TPM ID is not mapped to a crypto ID,
> > > tpm2_pcr_extend() ignores the digest.
> > > 
> > > Roberto
> > 
> > What if tpm_pcr_algorithms would convert TPM IDs to crypto IDs?
> > 
> > Externally other subsystems would have to then deal with only crypto
> > IDs.
> 
> Then, other subsystems cannot:
> 
> - use the TPM to calculate a digest
> - create an event log with the TCG format
> 
> Roberto

Can you open this up a bit? What is the use case that you canno
implement?

/Jarkko

Re: [PATCH v2 2/5] tpm: introduce tpm_pcr_algo_to_crypto() and tpm_pcr_algo_from_crypto()

[Jarkko Sakkinen]

On Wed, May 24, 2017 at 10:33:54AM -0700, Jarkko Sakkinen wrote:
> On Mon, May 22, 2017 at 09:21:28AM +0200, Roberto Sassu wrote:
> > On 5/20/2017 3:22 PM, Jarkko Sakkinen wrote:
> > > On Mon, May 15, 2017 at 04:22:22PM +0200, Roberto Sassu wrote:
> > > > On 5/15/2017 1:16 PM, Jarkko Sakkinen wrote:
> > > > > On Fri, May 05, 2017 at 04:21:49PM +0200, Roberto Sassu wrote:
> > > > > > tpm_pcr_algorithms() returns to its callers the IDs of the hash algorithms
> > > > > > supported by the TPM. This patch introduces tpm_pcr_algo_to_crypto(),
> > > > > > so that the callers can use the crypto subsystem to calculate the digest
> > > > > > to be passed to tpm_pcr_extend().
> > > > > > 
> > > > > > tpm_pcr_algo_from_crypto(), implemented for completeness, is instead used
> > > > > 
> > > > > What do you mean by completeness? Please, never add unused stuff.
> > > > > 
> > > > > > by tpm2_seal_trusted() to perform the opposite conversion.
> > > > > > 
> > > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > > ---
> > > > > > v2
> > > > > > 
> > > > > > - fixed return values of tpm2_pcr_algo_to_crypto() and
> > > > > >   tpm2_pcr_algo_from_crypto() if TPM support is disabled in the kernel
> > > > > 
> > > > > Change Log only to the cover letter.
> > > > > 
> > > > > >  drivers/char/tpm/tpm-interface.c | 51 ++++++++++++++++++++++++++++++++++++++++
> > > > > >  drivers/char/tpm/tpm2-cmd.c      | 42 +++++++++------------------------
> > > > > >  include/linux/tpm.h              | 13 ++++++++++
> > > > > >  3 files changed, 75 insertions(+), 31 deletions(-)
> > > > > 
> > > > > This commit is just deadly wrong in so many ways.
> > > > > 
> > > > > I would suggest to make extend always just take crypto ID in so you
> > > > > don't have to add these bizarre conversion functions.
> > > > 
> > > > The reason of this choice (as I explained in the cover letter)
> > > > is that TPM users might want to produce an event log with
> > > > the TCG format (which includes the TPM algorithm ID). Also,
> > > > TPM IDs should be preferred because, with them, TPM users
> > > > can calculate a digest directly with the TPM.
> > > > 
> > > > Taking crypto IDs means relying on the fact that there
> > > > is always a mapping between TPM IDs and crypto IDs.
> > > > Otherwise, tpm_pcr_algorithms() cannot return the algorithms
> > > > to its callers and PCRs cannot be extended. If TPM IDs are used,
> > > > TPM users have two alternatives: calculate the digest with
> > > > the TPM, or pass a SHA1 digest to tpm_pcr_extend(), so that
> > > > it can be padded to extend remanining PCR banks.
> > > > 
> > > > However, this second option will work only when the TPM driver
> > > > determines the size of an algorithm without relying on the crypto
> > > > subsystem. At the moment, if a TPM ID is not mapped to a crypto ID,
> > > > tpm2_pcr_extend() ignores the digest.
> > > > 
> > > > Roberto
> > > 
> > > What if tpm_pcr_algorithms would convert TPM IDs to crypto IDs?
> > > 
> > > Externally other subsystems would have to then deal with only crypto
> > > IDs.
> > 
> > Then, other subsystems cannot:
> > 
> > - use the TPM to calculate a digest
> > - create an event log with the TCG format
> > 
> > Roberto
> 
> Can you open this up a bit? What is the use case that you canno
> implement?
> 
> /Jarkko

The most important question is: are this used *right now*. If not, it is
a definitive NAK. We don't want any future-proof code to the kernel.
Always do the lowest common denominator in terms of generality and we
will refactor from there when need arises.

/Jarkko

Re: [PATCH v2 2/5] tpm: introduce tpm_pcr_algo_to_crypto() and tpm_pcr_algo_from_crypto()

[Roberto Sassu]

On 5/24/2017 10:25 PM, Jarkko Sakkinen wrote:
> On Wed, May 24, 2017 at 10:33:54AM -0700, Jarkko Sakkinen wrote:
>> On Mon, May 22, 2017 at 09:21:28AM +0200, Roberto Sassu wrote:
>>> On 5/20/2017 3:22 PM, Jarkko Sakkinen wrote:
>>>> On Mon, May 15, 2017 at 04:22:22PM +0200, Roberto Sassu wrote:
>>>>> On 5/15/2017 1:16 PM, Jarkko Sakkinen wrote:
>>>>>> On Fri, May 05, 2017 at 04:21:49PM +0200, Roberto Sassu wrote:
>>>>>>> tpm_pcr_algorithms() returns to its callers the IDs of the hash algorithms
>>>>>>> supported by the TPM. This patch introduces tpm_pcr_algo_to_crypto(),
>>>>>>> so that the callers can use the crypto subsystem to calculate the digest
>>>>>>> to be passed to tpm_pcr_extend().
>>>>>>>
>>>>>>> tpm_pcr_algo_from_crypto(), implemented for completeness, is instead used
>>>>>>
>>>>>> What do you mean by completeness? Please, never add unused stuff.
>>>>>>
>>>>>>> by tpm2_seal_trusted() to perform the opposite conversion.
>>>>>>>
>>>>>>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>>>>>>> ---
>>>>>>> v2
>>>>>>>
>>>>>>> - fixed return values of tpm2_pcr_algo_to_crypto() and
>>>>>>>   tpm2_pcr_algo_from_crypto() if TPM support is disabled in the kernel
>>>>>>
>>>>>> Change Log only to the cover letter.
>>>>>>
>>>>>>>  drivers/char/tpm/tpm-interface.c | 51 ++++++++++++++++++++++++++++++++++++++++
>>>>>>>  drivers/char/tpm/tpm2-cmd.c      | 42 +++++++++------------------------
>>>>>>>  include/linux/tpm.h              | 13 ++++++++++
>>>>>>>  3 files changed, 75 insertions(+), 31 deletions(-)
>>>>>>
>>>>>> This commit is just deadly wrong in so many ways.
>>>>>>
>>>>>> I would suggest to make extend always just take crypto ID in so you
>>>>>> don't have to add these bizarre conversion functions.
>>>>>
>>>>> The reason of this choice (as I explained in the cover letter)
>>>>> is that TPM users might want to produce an event log with
>>>>> the TCG format (which includes the TPM algorithm ID). Also,
>>>>> TPM IDs should be preferred because, with them, TPM users
>>>>> can calculate a digest directly with the TPM.
>>>>>
>>>>> Taking crypto IDs means relying on the fact that there
>>>>> is always a mapping between TPM IDs and crypto IDs.
>>>>> Otherwise, tpm_pcr_algorithms() cannot return the algorithms
>>>>> to its callers and PCRs cannot be extended. If TPM IDs are used,
>>>>> TPM users have two alternatives: calculate the digest with
>>>>> the TPM, or pass a SHA1 digest to tpm_pcr_extend(), so that
>>>>> it can be padded to extend remanining PCR banks.
>>>>>
>>>>> However, this second option will work only when the TPM driver
>>>>> determines the size of an algorithm without relying on the crypto
>>>>> subsystem. At the moment, if a TPM ID is not mapped to a crypto ID,
>>>>> tpm2_pcr_extend() ignores the digest.
>>>>>
>>>>> Roberto
>>>>
>>>> What if tpm_pcr_algorithms would convert TPM IDs to crypto IDs?
>>>>
>>>> Externally other subsystems would have to then deal with only crypto
>>>> IDs.
>>>
>>> Then, other subsystems cannot:
>>>
>>> - use the TPM to calculate a digest
>>> - create an event log with the TCG format
>>>
>>> Roberto
>>
>> Can you open this up a bit? What is the use case that you canno
>> implement?
>>
>> /Jarkko
>
> The most important question is: are this used *right now*. If not, it is
> a definitive NAK. We don't want any future-proof code to the kernel.
> Always do the lowest common denominator in terms of generality and we
> will refactor from there when need arises.

This is still being discussed.

If we use crypto IDs, as you suggested, what it should be passed
to tpm_pcr_extend()? Or, probably to a new function (Mimi said that
changing the parameters of an existing function is not bisect-safe).

Roberto

-- 
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Qiuen PENG, Shengli WANG

Re: [PATCH v2 2/5] tpm: introduce tpm_pcr_algo_to_crypto() and tpm_pcr_algo_from_crypto()

[Jarkko Sakkinen]

On Tue, May 30, 2017 at 12:24:21PM +0200, Roberto Sassu wrote:
> On 5/24/2017 10:25 PM, Jarkko Sakkinen wrote:
> > On Wed, May 24, 2017 at 10:33:54AM -0700, Jarkko Sakkinen wrote:
> > > On Mon, May 22, 2017 at 09:21:28AM +0200, Roberto Sassu wrote:
> > > > On 5/20/2017 3:22 PM, Jarkko Sakkinen wrote:
> > > > > On Mon, May 15, 2017 at 04:22:22PM +0200, Roberto Sassu wrote:
> > > > > > On 5/15/2017 1:16 PM, Jarkko Sakkinen wrote:
> > > > > > > On Fri, May 05, 2017 at 04:21:49PM +0200, Roberto Sassu wrote:
> > > > > > > > tpm_pcr_algorithms() returns to its callers the IDs of the hash algorithms
> > > > > > > > supported by the TPM. This patch introduces tpm_pcr_algo_to_crypto(),
> > > > > > > > so that the callers can use the crypto subsystem to calculate the digest
> > > > > > > > to be passed to tpm_pcr_extend().
> > > > > > > > 
> > > > > > > > tpm_pcr_algo_from_crypto(), implemented for completeness, is instead used
> > > > > > > 
> > > > > > > What do you mean by completeness? Please, never add unused stuff.
> > > > > > > 
> > > > > > > > by tpm2_seal_trusted() to perform the opposite conversion.
> > > > > > > > 
> > > > > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > > > > ---
> > > > > > > > v2
> > > > > > > > 
> > > > > > > > - fixed return values of tpm2_pcr_algo_to_crypto() and
> > > > > > > >   tpm2_pcr_algo_from_crypto() if TPM support is disabled in the kernel
> > > > > > > 
> > > > > > > Change Log only to the cover letter.
> > > > > > > 
> > > > > > > >  drivers/char/tpm/tpm-interface.c | 51 ++++++++++++++++++++++++++++++++++++++++
> > > > > > > >  drivers/char/tpm/tpm2-cmd.c      | 42 +++++++++------------------------
> > > > > > > >  include/linux/tpm.h              | 13 ++++++++++
> > > > > > > >  3 files changed, 75 insertions(+), 31 deletions(-)
> > > > > > > 
> > > > > > > This commit is just deadly wrong in so many ways.
> > > > > > > 
> > > > > > > I would suggest to make extend always just take crypto ID in so you
> > > > > > > don't have to add these bizarre conversion functions.
> > > > > > 
> > > > > > The reason of this choice (as I explained in the cover letter)
> > > > > > is that TPM users might want to produce an event log with
> > > > > > the TCG format (which includes the TPM algorithm ID). Also,
> > > > > > TPM IDs should be preferred because, with them, TPM users
> > > > > > can calculate a digest directly with the TPM.
> > > > > > 
> > > > > > Taking crypto IDs means relying on the fact that there
> > > > > > is always a mapping between TPM IDs and crypto IDs.
> > > > > > Otherwise, tpm_pcr_algorithms() cannot return the algorithms
> > > > > > to its callers and PCRs cannot be extended. If TPM IDs are used,
> > > > > > TPM users have two alternatives: calculate the digest with
> > > > > > the TPM, or pass a SHA1 digest to tpm_pcr_extend(), so that
> > > > > > it can be padded to extend remanining PCR banks.
> > > > > > 
> > > > > > However, this second option will work only when the TPM driver
> > > > > > determines the size of an algorithm without relying on the crypto
> > > > > > subsystem. At the moment, if a TPM ID is not mapped to a crypto ID,
> > > > > > tpm2_pcr_extend() ignores the digest.
> > > > > > 
> > > > > > Roberto
> > > > > 
> > > > > What if tpm_pcr_algorithms would convert TPM IDs to crypto IDs?
> > > > > 
> > > > > Externally other subsystems would have to then deal with only crypto
> > > > > IDs.
> > > > 
> > > > Then, other subsystems cannot:
> > > > 
> > > > - use the TPM to calculate a digest
> > > > - create an event log with the TCG format
> > > > 
> > > > Roberto
> > > 
> > > Can you open this up a bit? What is the use case that you canno
> > > implement?
> > > 
> > > /Jarkko
> > 
> > The most important question is: are this used *right now*. If not, it is
> > a definitive NAK. We don't want any future-proof code to the kernel.
> > Always do the lowest common denominator in terms of generality and we
> > will refactor from there when need arises.
> 
> This is still being discussed.
> 
> If we use crypto IDs, as you suggested, what it should be passed
> to tpm_pcr_extend()? Or, probably to a new function (Mimi said that
> changing the parameters of an existing function is not bisect-safe).
> 
> Roberto
> 
> -- 
> HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
> Managing Director: Bo PENG, Qiuen PENG, Shengli WANG

Parameters should be changed when it makes sense to do so. It makes
sense in this case.

/Jarkko

Re: [Linux-ima-devel] [PATCH v2 3/5] tpm: pass multiple digests to tpm_pcr_extend()

[Mimi Zohar]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="maccentraleurope", Size: 7437 bytes --]

On Fri, 2017-05-05 at 16:21 +0200, Roberto Sassu wrote:
> The tpm_pcr_extend() definition has been modified to take an array of
> tpm2_digest structures, and the size of the array as arguments.
> 
> The function now checks if callers provided a digests for each active
> PCR bank (or a SHA1 digest for TPM 1.2), to follow the recomendation from
> the TCG specifications. See commit c1f92b4b04ad ("tpm: enhance TPM 2.0
> PCR extend to support multiple banks"). All banks should be extended
> because unused banks could be used by an attacker to hide the true
> integrity status of the platform.
> 
> The only allowed exception to the rule above is to pass a SHA1 digest.
> It has been introduced to maintain compatibility with applications that
> expect to interact with a TPM 1.2, and provide only a SHA1 digest.
> In this case, the behavior of tpm_pcr_extend() is unchanged and
> remaining PCR banks are extended with that digest, padded with zeros.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
> v2
> 
> - tpm_pcr_extend() arguments checked by tpm_pcr_check_input()
> - modified parameters of tpm_pcr_extend()
> 
>  drivers/char/tpm/tpm-interface.c | 76 +++++++++++++++++++++++++++++++++++++---
>  drivers/char/tpm/tpm.h           |  6 ----
>  include/linux/tpm.h              | 11 ++++--
>  3 files changed, 80 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index aac703e..4b08b02 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -867,6 +867,55 @@ static int tpm1_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash,
>  }
> 
>  /**
> + * tpm_pcr_check_input - check digests argument
> + *
> + * Return values:
> + *       1: input correct
> + *       0: fill digests with SHA1 digest padded with zeros
> + * -EINVAL: input incorrect
> + */
> +static int tpm_pcr_check_input(struct tpm_chip *chip, int count,
> +			       struct tpm2_digest *digests)
> +{
> +	bool sha1_only;
> +	int found = 0, not_found = 0;
> +	int i, j;
> +
> +	if (count <= 0 || digests = NULL)
> +		return -EINVAL;
> +
> +	sha1_only = (count = 1 && digests[0].alg_id = TPM2_ALG_SHA1);
> +
> +	if (!(chip->flags & TPM_CHIP_FLAG_TPM2))
> +		return sha1_only ? 1 : -EINVAL;
> +
> +	if (sha1_only)
> +		return 0;
> +
> +	for (i = 0; i < ARRAY_SIZE(chip->active_banks) &&
> +	     chip->active_banks[i] != TPM2_ALG_ERROR; i++) {
> +		for (j = 0; j < count; j++) {
> +			if (digests[j].alg_id = chip->active_banks[i]) {
> +				found++;
> +				break;
> +			}
> +		}
> +
> +		if (j = count) {
> +			dev_dbg(&chip->dev, "%s: missing algorithm 0x%X\n",
> +				__func__, chip->active_banks[i]);
> +			not_found++;
> +		}
> +	}
> +
> +	if (not_found = 0 && found != count)
> +		dev_dbg(&chip->dev,
> +			"%s: duplicate or unsupported algorithm\n", __func__);
> +
> +	return (not_found = 0 && found = count) ? 1 : -EINVAL;
> +}
> +
> +/**
>   * tpm_pcr_extend - extend pcr value with hash
>   * @chip_num:	tpm idx # or AN&
>   * @pcr_idx:	pcr idx to extend
> @@ -876,29 +925,46 @@ static int tpm1_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash,
>   * isn't, protect against the chip disappearing, by incrementing
>   * the module usage count.
>   */
> -int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash)
> +int tpm_pcr_extend(u32 chip_num, int pcr_idx, int count,
> +		   struct tpm2_digest *digests)
>  {
>  	int rc;
>  	struct tpm_chip *chip;
>  	struct tpm2_digest digest_list[ARRAY_SIZE(chip->active_banks)];
> -	u32 count = 0;
> +	struct tpm2_digest *digests_ptr = digests;
> +	u32 filled_count = 0;
> +	u8 *hash;
>  	int i;
> 
>  	chip = tpm_chip_find_get(chip_num);
>  	if (chip = NULL)
>  		return -ENODEV;
> 
> -	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> +	rc = tpm_pcr_check_input(chip, count, digests);
> +	if (rc < 0) {
> +		dev_dbg(&chip->dev, "%s: invalid arguments\n", __func__);
> +		tpm_put_ops(chip);

This rejects the TPM extend, if ANY of the algorithms are unknown.
Suppose that the standards were updated, TPM vendors add support for
the new algorithm, but the kernel has not been updated to reflect the
new algorithms supported.  As the measurement hash already been added
to the IMA measurement list, verifying the measurement list against a
TPM quote will fail, not just for the unknown algorithm, but for all
algorithms.  Something is very broken with this approach.

Mimi

> +		return rc;
> +	}
> +
> +	hash = digests[0].digest;
> +
> +	if (!rc) {
>  		memset(digest_list, 0, sizeof(digest_list));
> 
>  		for (i = 0; i < ARRAY_SIZE(chip->active_banks) &&
>  			    chip->active_banks[i] != TPM2_ALG_ERROR; i++) {
>  			digest_list[i].alg_id = chip->active_banks[i];
>  			memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE);
> -			count++;
> +			filled_count++;
>  		}
> 
> -		rc = tpm2_pcr_extend(chip, pcr_idx, count, digest_list);
> +		digests_ptr = digest_list;
> +		count = filled_count;
> +	}
> +
> +	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> +		rc = tpm2_pcr_extend(chip, pcr_idx, count, digests_ptr);
>  		tpm_put_ops(chip);
>  		return rc;
>  	}
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index b22bc25..6d775c4 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -34,7 +34,6 @@
>  #include <linux/acpi.h>
>  #include <linux/cdev.h>
>  #include <linux/highmem.h>
> -#include <crypto/hash_info.h>
> 
>  enum tpm_const {
>  	TPM_MINOR = 224,	/* officially assigned */
> @@ -405,11 +404,6 @@ struct tpm_cmd_t {
>  	tpm_cmd_params	params;
>  } __packed;
> 
> -struct tpm2_digest {
> -	u16 alg_id;
> -	u8 digest[SHA512_DIGEST_SIZE];
> -} __packed;
> -
>  /* A string buffer type for constructing TPM commands. This is based on the
>   * ideas of string buffer code in security/keys/trusted.h but is heap based
>   * in order to keep the stack usage minimal.
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index 9ecd12c..5c5a600 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -36,6 +36,11 @@ struct tpm_chip;
>  struct trusted_key_payload;
>  struct trusted_key_options;
> 
> +struct tpm2_digest {
> +	u16 alg_id;
> +	u8 digest[SHA512_DIGEST_SIZE];
> +} __packed;
> +
>  enum TPM_OPS_FLAGS {
>  	TPM_OPS_AUTO_STARTUP = BIT(0),
>  };
> @@ -70,7 +75,8 @@ struct tpm_class_ops {
> 
>  extern int tpm_is_tpm2(u32 chip_num);
>  extern int tpm_pcr_read(u32 chip_num, int pcr_idx, u8 *res_buf);
> -extern int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash);
> +extern int tpm_pcr_extend(u32 chip_num, int pcr_idx, int count,
> +			  struct tpm2_digest *digests);
>  extern int tpm_pcr_algorithms(u32 chip_num, int count,
>  			      enum tpm2_algorithms *algorithms);
>  extern enum hash_algo tpm_pcr_algo_to_crypto(enum tpm2_algorithms tpm_id);
> @@ -91,7 +97,8 @@ static inline int tpm_is_tpm2(u32 chip_num)
>  static inline int tpm_pcr_read(u32 chip_num, int pcr_idx, u8 *res_buf) {
>  	return -ENODEV;
>  }
> -static inline int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash) {
> +static inline int tpm_pcr_extend(u32 chip_num, int pcr_idx, int count,
> +				 struct tpm2_digest *digests) {
>  	return -ENODEV;
>  }
>  static inline int tpm_pcr_algorithms(u32 chip_num, int count,

--
To unsubscribe from this list: send the line "unsubscribe keyrings" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Linux-ima-devel] [PATCH v2 3/5] tpm: pass multiple digests to tpm_pcr_extend()

[Roberto Sassu]

On 5/30/2017 5:29 AM, Mimi Zohar wrote:
> On Fri, 2017-05-05 at 16:21 +0200, Roberto Sassu wrote:
>> The tpm_pcr_extend() definition has been modified to take an array of
>> tpm2_digest structures, and the size of the array as arguments.
>>
>> The function now checks if callers provided a digests for each active
>> PCR bank (or a SHA1 digest for TPM 1.2), to follow the recomendation from
>> the TCG specifications. See commit c1f92b4b04ad ("tpm: enhance TPM 2.0
>> PCR extend to support multiple banks"). All banks should be extended
>> because unused banks could be used by an attacker to hide the true
>> integrity status of the platform.
>>
>> The only allowed exception to the rule above is to pass a SHA1 digest.
>> It has been introduced to maintain compatibility with applications that
>> expect to interact with a TPM 1.2, and provide only a SHA1 digest.
>> In this case, the behavior of tpm_pcr_extend() is unchanged and
>> remaining PCR banks are extended with that digest, padded with zeros.
>>
>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>> ---
>> v2
>>
>> - tpm_pcr_extend() arguments checked by tpm_pcr_check_input()
>> - modified parameters of tpm_pcr_extend()
>>
>>  drivers/char/tpm/tpm-interface.c | 76 +++++++++++++++++++++++++++++++++++++---
>>  drivers/char/tpm/tpm.h           |  6 ----
>>  include/linux/tpm.h              | 11 ++++--
>>  3 files changed, 80 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
>> index aac703e..4b08b02 100644
>> --- a/drivers/char/tpm/tpm-interface.c
>> +++ b/drivers/char/tpm/tpm-interface.c
>> @@ -867,6 +867,55 @@ static int tpm1_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash,
>>  }
>>
>>  /**
>> + * tpm_pcr_check_input - check digests argument
>> + *
>> + * Return values:
>> + *       1: input correct
>> + *       0: fill digests with SHA1 digest padded with zeros
>> + * -EINVAL: input incorrect
>> + */
>> +static int tpm_pcr_check_input(struct tpm_chip *chip, int count,
>> +			       struct tpm2_digest *digests)
>> +{
>> +	bool sha1_only;
>> +	int found = 0, not_found = 0;
>> +	int i, j;
>> +
>> +	if (count <= 0 || digests = NULL)
>> +		return -EINVAL;
>> +
>> +	sha1_only = (count = 1 && digests[0].alg_id = TPM2_ALG_SHA1);
>> +
>> +	if (!(chip->flags & TPM_CHIP_FLAG_TPM2))
>> +		return sha1_only ? 1 : -EINVAL;
>> +
>> +	if (sha1_only)
>> +		return 0;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(chip->active_banks) &&
>> +	     chip->active_banks[i] != TPM2_ALG_ERROR; i++) {
>> +		for (j = 0; j < count; j++) {
>> +			if (digests[j].alg_id = chip->active_banks[i]) {
>> +				found++;
>> +				break;
>> +			}
>> +		}
>> +
>> +		if (j = count) {
>> +			dev_dbg(&chip->dev, "%s: missing algorithm 0x%X\n",
>> +				__func__, chip->active_banks[i]);
>> +			not_found++;
>> +		}
>> +	}
>> +
>> +	if (not_found = 0 && found != count)
>> +		dev_dbg(&chip->dev,
>> +			"%s: duplicate or unsupported algorithm\n", __func__);
>> +
>> +	return (not_found = 0 && found = count) ? 1 : -EINVAL;
>> +}
>> +
>> +/**
>>   * tpm_pcr_extend - extend pcr value with hash
>>   * @chip_num:	tpm idx # or AN&
>>   * @pcr_idx:	pcr idx to extend
>> @@ -876,29 +925,46 @@ static int tpm1_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash,
>>   * isn't, protect against the chip disappearing, by incrementing
>>   * the module usage count.
>>   */
>> -int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash)
>> +int tpm_pcr_extend(u32 chip_num, int pcr_idx, int count,
>> +		   struct tpm2_digest *digests)
>>  {
>>  	int rc;
>>  	struct tpm_chip *chip;
>>  	struct tpm2_digest digest_list[ARRAY_SIZE(chip->active_banks)];
>> -	u32 count = 0;
>> +	struct tpm2_digest *digests_ptr = digests;
>> +	u32 filled_count = 0;
>> +	u8 *hash;
>>  	int i;
>>
>>  	chip = tpm_chip_find_get(chip_num);
>>  	if (chip = NULL)
>>  		return -ENODEV;
>>
>> -	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
>> +	rc = tpm_pcr_check_input(chip, count, digests);
>> +	if (rc < 0) {
>> +		dev_dbg(&chip->dev, "%s: invalid arguments\n", __func__);
>> +		tpm_put_ops(chip);
>
> This rejects the TPM extend, if ANY of the algorithms are unknown.
> Suppose that the standards were updated, TPM vendors add support for
> the new algorithm, but the kernel has not been updated to reflect the
> new algorithms supported.  As the measurement hash already been added
> to the IMA measurement list, verifying the measurement list against a
> TPM quote will fail, not just for the unknown algorithm, but for all
> algorithms.  Something is very broken with this approach.

The alternative is to extend remaining banks with a digest,
for example the first passed by the caller. I will modify
the patch, if everyone agrees on that.

Roberto


>
> Mimi
>
>> +		return rc;
>> +	}
>> +
>> +	hash = digests[0].digest;
>> +
>> +	if (!rc) {
>>  		memset(digest_list, 0, sizeof(digest_list));
>>
>>  		for (i = 0; i < ARRAY_SIZE(chip->active_banks) &&
>>  			    chip->active_banks[i] != TPM2_ALG_ERROR; i++) {
>>  			digest_list[i].alg_id = chip->active_banks[i];
>>  			memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE);
>> -			count++;
>> +			filled_count++;
>>  		}
>>
>> -		rc = tpm2_pcr_extend(chip, pcr_idx, count, digest_list);
>> +		digests_ptr = digest_list;
>> +		count = filled_count;
>> +	}
>> +
>> +	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
>> +		rc = tpm2_pcr_extend(chip, pcr_idx, count, digests_ptr);
>>  		tpm_put_ops(chip);
>>  		return rc;
>>  	}
>> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
>> index b22bc25..6d775c4 100644
>> --- a/drivers/char/tpm/tpm.h
>> +++ b/drivers/char/tpm/tpm.h
>> @@ -34,7 +34,6 @@
>>  #include <linux/acpi.h>
>>  #include <linux/cdev.h>
>>  #include <linux/highmem.h>
>> -#include <crypto/hash_info.h>
>>
>>  enum tpm_const {
>>  	TPM_MINOR = 224,	/* officially assigned */
>> @@ -405,11 +404,6 @@ struct tpm_cmd_t {
>>  	tpm_cmd_params	params;
>>  } __packed;
>>
>> -struct tpm2_digest {
>> -	u16 alg_id;
>> -	u8 digest[SHA512_DIGEST_SIZE];
>> -} __packed;
>> -
>>  /* A string buffer type for constructing TPM commands. This is based on the
>>   * ideas of string buffer code in security/keys/trusted.h but is heap based
>>   * in order to keep the stack usage minimal.
>> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
>> index 9ecd12c..5c5a600 100644
>> --- a/include/linux/tpm.h
>> +++ b/include/linux/tpm.h
>> @@ -36,6 +36,11 @@ struct tpm_chip;
>>  struct trusted_key_payload;
>>  struct trusted_key_options;
>>
>> +struct tpm2_digest {
>> +	u16 alg_id;
>> +	u8 digest[SHA512_DIGEST_SIZE];
>> +} __packed;
>> +
>>  enum TPM_OPS_FLAGS {
>>  	TPM_OPS_AUTO_STARTUP = BIT(0),
>>  };
>> @@ -70,7 +75,8 @@ struct tpm_class_ops {
>>
>>  extern int tpm_is_tpm2(u32 chip_num);
>>  extern int tpm_pcr_read(u32 chip_num, int pcr_idx, u8 *res_buf);
>> -extern int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash);
>> +extern int tpm_pcr_extend(u32 chip_num, int pcr_idx, int count,
>> +			  struct tpm2_digest *digests);
>>  extern int tpm_pcr_algorithms(u32 chip_num, int count,
>>  			      enum tpm2_algorithms *algorithms);
>>  extern enum hash_algo tpm_pcr_algo_to_crypto(enum tpm2_algorithms tpm_id);
>> @@ -91,7 +97,8 @@ static inline int tpm_is_tpm2(u32 chip_num)
>>  static inline int tpm_pcr_read(u32 chip_num, int pcr_idx, u8 *res_buf) {
>>  	return -ENODEV;
>>  }
>> -static inline int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash) {
>> +static inline int tpm_pcr_extend(u32 chip_num, int pcr_idx, int count,
>> +				 struct tpm2_digest *digests) {
>>  	return -ENODEV;
>>  }
>>  static inline int tpm_pcr_algorithms(u32 chip_num, int count,
>

Re: [Linux-ima-devel] [PATCH v2 3/5] tpm: pass multiple digests to tpm_pcr_extend()

[Mimi Zohar]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="maccentraleurope", Size: 2641 bytes --]

On Tue, 2017-05-30 at 09:28 +0200, Roberto Sassu wrote:
> On 5/30/2017 5:29 AM, Mimi Zohar wrote:
> > On Fri, 2017-05-05 at 16:21 +0200, Roberto Sassu wrote:


> >> @@ -876,29 +925,46 @@ static int tpm1_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash,
> >>   * isn't, protect against the chip disappearing, by incrementing
> >>   * the module usage count.
> >>   */
> >> -int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash)
> >> +int tpm_pcr_extend(u32 chip_num, int pcr_idx, int count,
> >> +		   struct tpm2_digest *digests)
> >>  {
> >>  	int rc;
> >>  	struct tpm_chip *chip;
> >>  	struct tpm2_digest digest_list[ARRAY_SIZE(chip->active_banks)];
> >> -	u32 count = 0;
> >> +	struct tpm2_digest *digests_ptr = digests;
> >> +	u32 filled_count = 0;
> >> +	u8 *hash;
> >>  	int i;
> >>
> >>  	chip = tpm_chip_find_get(chip_num);
> >>  	if (chip = NULL)
> >>  		return -ENODEV;
> >>
> >> -	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> >> +	rc = tpm_pcr_check_input(chip, count, digests);
> >> +	if (rc < 0) {
> >> +		dev_dbg(&chip->dev, "%s: invalid arguments\n", __func__);
> >> +		tpm_put_ops(chip);
> >
> > This rejects the TPM extend, if ANY of the algorithms are unknown.
> > Suppose that the standards were updated, TPM vendors add support for
> > the new algorithm, but the kernel has not been updated to reflect the
> > new algorithms supported.  As the measurement hash already been added
> > to the IMA measurement list, verifying the measurement list against a
> > TPM quote will fail, not just for the unknown algorithm, but for all
> > algorithms.  Something is very broken with this approach.
> 
> The alternative is to extend remaining banks with a digest,
> for example the first passed by the caller. I will modify
> the patch, if everyone agrees on that.

The solution you're proposing is similar to the original solution of
extending the TPM with a padded/truncated SHA1 hash, but this time it
might not be a padded/truncated SHA1 hash, but a different algorithm.
So the attestation server will then need to know which hash algorithm
was used to extend each of the TPM banks - a padded/truncated digest
value or the real digest value.

The only issue, from the kernel's perspective, will be determining the
algorithm's digest size as the kernel has no knowledge of it.  On TPM
registration/initialization, if you're not already querying the TPM
for the algorithm digest sizes, you will need to do so.

Mimi

--
To unsubscribe from this list: send the line "unsubscribe keyrings" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Linux-ima-devel] [PATCH v2 3/5] tpm: pass multiple digests to tpm_pcr_extend()

[Roberto Sassu]

On 5/30/2017 1:25 PM, Mimi Zohar wrote:
> On Tue, 2017-05-30 at 09:28 +0200, Roberto Sassu wrote:
>> On 5/30/2017 5:29 AM, Mimi Zohar wrote:
>>> On Fri, 2017-05-05 at 16:21 +0200, Roberto Sassu wrote:
>
>
>>>> @@ -876,29 +925,46 @@ static int tpm1_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash,
>>>>   * isn't, protect against the chip disappearing, by incrementing
>>>>   * the module usage count.
>>>>   */
>>>> -int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash)
>>>> +int tpm_pcr_extend(u32 chip_num, int pcr_idx, int count,
>>>> +		   struct tpm2_digest *digests)
>>>>  {
>>>>  	int rc;
>>>>  	struct tpm_chip *chip;
>>>>  	struct tpm2_digest digest_list[ARRAY_SIZE(chip->active_banks)];
>>>> -	u32 count = 0;
>>>> +	struct tpm2_digest *digests_ptr = digests;
>>>> +	u32 filled_count = 0;
>>>> +	u8 *hash;
>>>>  	int i;
>>>>
>>>>  	chip = tpm_chip_find_get(chip_num);
>>>>  	if (chip = NULL)
>>>>  		return -ENODEV;
>>>>
>>>> -	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
>>>> +	rc = tpm_pcr_check_input(chip, count, digests);
>>>> +	if (rc < 0) {
>>>> +		dev_dbg(&chip->dev, "%s: invalid arguments\n", __func__);
>>>> +		tpm_put_ops(chip);
>>>
>>> This rejects the TPM extend, if ANY of the algorithms are unknown.
>>> Suppose that the standards were updated, TPM vendors add support for
>>> the new algorithm, but the kernel has not been updated to reflect the
>>> new algorithms supported.  As the measurement hash already been added
>>> to the IMA measurement list, verifying the measurement list against a
>>> TPM quote will fail, not just for the unknown algorithm, but for all
>>> algorithms.  Something is very broken with this approach.
>>
>> The alternative is to extend remaining banks with a digest,
>> for example the first passed by the caller. I will modify
>> the patch, if everyone agrees on that.
>
> The solution you're proposing is similar to the original solution of
> extending the TPM with a padded/truncated SHA1 hash, but this time it
> might not be a padded/truncated SHA1 hash, but a different algorithm.
> So the attestation server will then need to know which hash algorithm
> was used to extend each of the TPM banks - a padded/truncated digest
> value or the real digest value.

The convention would be that the first digest in the event log
is truncated/padded, to extend banks for which no digest is
provided. This convention would apply to all callers of
tpm_pcr_extend().

Roberto


> The only issue, from the kernel's perspective, will be determining the
> algorithm's digest size as the kernel has no knowledge of it.  On TPM
> registration/initialization, if you're not already querying the TPM
> for the algorithm digest sizes, you will need to do so.
>
> Mimi
>

-- 
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Qiuen PENG, Shengli WANG

Re: [Linux-ima-devel] [PATCH v2 4/5] keys, trusted: modify arguments of tpm_pcr_extend()

[Mimi Zohar]

On Fri, 2017-05-05 at 16:21 +0200, Roberto Sassu wrote:
> pcrlock() has been modified to pass the correct arguments
> to tpm_pcr_extend(): the pointer of a tpm2_digest structure containing
> a random value generated by tpm_get_random() and the size of the array (1).

If the number of arguments is wrong, that means the patch that
introduced the change is not bi-sect safe.  (This comment is
applicable to patch 5/5 too.)

Mimi

> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>  security/keys/trusted.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/security/keys/trusted.c b/security/keys/trusted.c
> index 2ae31c5..3eb89e6 100644
> --- a/security/keys/trusted.c
> +++ b/security/keys/trusted.c
> @@ -377,15 +377,15 @@ static int trusted_tpm_send(const u32 chip_num, unsigned char *cmd,
>   */
>  static int pcrlock(const int pcrnum)
>  {
> -	unsigned char hash[SHA1_DIGEST_SIZE];
> +	struct tpm2_digest digestarg = {.alg_id = TPM2_ALG_SHA1};
>  	int ret;
> 
>  	if (!capable(CAP_SYS_ADMIN))
>  		return -EPERM;
> -	ret = tpm_get_random(TPM_ANY_NUM, hash, SHA1_DIGEST_SIZE);
> +	ret = tpm_get_random(TPM_ANY_NUM, digestarg.digest, SHA1_DIGEST_SIZE);
>  	if (ret != SHA1_DIGEST_SIZE)
>  		return ret;
> -	return tpm_pcr_extend(TPM_ANY_NUM, pcrnum, hash) ? -EINVAL : 0;
> +	return tpm_pcr_extend(TPM_ANY_NUM, pcrnum, 1, &digestarg) ? -EINVAL : 0;
>  }
> 
>  /*

Re: [Linux-ima-devel] [PATCH v2 4/5] keys, trusted: modify arguments of tpm_pcr_extend()

[Roberto Sassu]

On 5/30/2017 5:35 AM, Mimi Zohar wrote:
> On Fri, 2017-05-05 at 16:21 +0200, Roberto Sassu wrote:
>> pcrlock() has been modified to pass the correct arguments
>> to tpm_pcr_extend(): the pointer of a tpm2_digest structure containing
>> a random value generated by tpm_get_random() and the size of the array (1).
>
> If the number of arguments is wrong, that means the patch that
> introduced the change is not bi-sect safe.  (This comment is
> applicable to patch 5/5 too.)

Jarkko (the TPM driver maintainer) asked me to not introduce
a new function to pass multiple digests, but to modify
the parameters of tpm_pcr_extend().

Roberto


>
> Mimi
>
>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>> ---
>>  security/keys/trusted.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/security/keys/trusted.c b/security/keys/trusted.c
>> index 2ae31c5..3eb89e6 100644
>> --- a/security/keys/trusted.c
>> +++ b/security/keys/trusted.c
>> @@ -377,15 +377,15 @@ static int trusted_tpm_send(const u32 chip_num, unsigned char *cmd,
>>   */
>>  static int pcrlock(const int pcrnum)
>>  {
>> -	unsigned char hash[SHA1_DIGEST_SIZE];
>> +	struct tpm2_digest digestarg = {.alg_id = TPM2_ALG_SHA1};
>>  	int ret;
>>
>>  	if (!capable(CAP_SYS_ADMIN))
>>  		return -EPERM;
>> -	ret = tpm_get_random(TPM_ANY_NUM, hash, SHA1_DIGEST_SIZE);
>> +	ret = tpm_get_random(TPM_ANY_NUM, digestarg.digest, SHA1_DIGEST_SIZE);
>>  	if (ret != SHA1_DIGEST_SIZE)
>>  		return ret;
>> -	return tpm_pcr_extend(TPM_ANY_NUM, pcrnum, hash) ? -EINVAL : 0;
>> +	return tpm_pcr_extend(TPM_ANY_NUM, pcrnum, 1, &digestarg) ? -EINVAL : 0;
>>  }
>>
>>  /*
>

Re: [Linux-ima-devel] [PATCH v2 4/5] keys, trusted: modify arguments of tpm_pcr_extend()

[Mimi Zohar]

On Tue, 2017-05-30 at 09:36 +0200, Roberto Sassu wrote:
> On 5/30/2017 5:35 AM, Mimi Zohar wrote:
> > On Fri, 2017-05-05 at 16:21 +0200, Roberto Sassu wrote:
> >> pcrlock() has been modified to pass the correct arguments
> >> to tpm_pcr_extend(): the pointer of a tpm2_digest structure containing
> >> a random value generated by tpm_get_random() and the size of the array (1).
> >
> > If the number of arguments is wrong, that means the patch that
> > introduced the change is not bi-sect safe.  (This comment is
> > applicable to patch 5/5 too.)
> 
> Jarkko (the TPM driver maintainer) asked me to not introduce
> a new function to pass multiple digests, but to modify
> the parameters of tpm_pcr_extend().

Since struct tpm2_digest is a static size, shouldn't we be able to
compute the number of digests?

Mimi

Re: [Linux-ima-devel] [PATCH v2 4/5] keys, trusted: modify arguments of tpm_pcr_extend()

[Roberto Sassu]

On 5/30/2017 2:06 PM, Mimi Zohar wrote:
> On Tue, 2017-05-30 at 09:36 +0200, Roberto Sassu wrote:
>> On 5/30/2017 5:35 AM, Mimi Zohar wrote:
>>> On Fri, 2017-05-05 at 16:21 +0200, Roberto Sassu wrote:
>>>> pcrlock() has been modified to pass the correct arguments
>>>> to tpm_pcr_extend(): the pointer of a tpm2_digest structure containing
>>>> a random value generated by tpm_get_random() and the size of the array (1).
>>>
>>> If the number of arguments is wrong, that means the patch that
>>> introduced the change is not bi-sect safe.  (This comment is
>>> applicable to patch 5/5 too.)
>>
>> Jarkko (the TPM driver maintainer) asked me to not introduce
>> a new function to pass multiple digests, but to modify
>> the parameters of tpm_pcr_extend().
>
> Since struct tpm2_digest is a static size, shouldn't we be able to
> compute the number of digests?

The length of 'hash' is not passed to tpm_pcr_extend().

The only way to avoid changing the number of parameters would
be to pass a buffer whose format is the same of the event log
format defined by TCG:

<algo count> <algo1 ID> <digest1> ... <algoN ID> <digestN>

Roberto

-- 
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Qiuen PENG, Shengli WANG