diff mbox series

[v5,12/21] tpm: move pcr extend code to tpm2-cmd.c

Message ID 20180928223035.14471-13-tomas.winkler@intel.com (mailing list archive)
State New, archived
Headers show
Series tpm: separate tpm 1.x and tpm 2.x commands | expand

Commit Message

Winkler, Tomas Sept. 28, 2018, 10:30 p.m. UTC
Add tpm2_pcr_extend() function to tpm2-cmd.c with signature required
by tpm-interface.c. It wraps the original open code
implementation. The original original tpm2_pcr_extend() function
is renamed to __tpm2_pcr_extend() and made static, it is called
only from new tpm2_pcr_extend().

Fix warnings in __tpm2_pcr_extend()
tpm2-cmd.c:251:16: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
tpm2-cmd.c:252:17: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]

Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
V3: Fix the commit message.
V4: 1. Fix the kdoc.
    2. Fix the commit message.
    3. Fix compilation warnings.
V5: A small fix in the kdoc.

 drivers/char/tpm/tpm-interface.c | 24 +++++----------------
 drivers/char/tpm/tpm.h           |  3 +--
 drivers/char/tpm/tpm1-cmd.c      |  2 +-
 drivers/char/tpm/tpm2-cmd.c      | 46 +++++++++++++++++++++++++++++++---------
 4 files changed, 43 insertions(+), 32 deletions(-)

Comments

Jarkko Sakkinen Oct. 2, 2018, 12:52 a.m. UTC | #1
On Sat, Sep 29, 2018 at 01:30:26AM +0300, Tomas Winkler wrote:
> Add tpm2_pcr_extend() function to tpm2-cmd.c with signature required
> by tpm-interface.c. It wraps the original open code
> implementation. The original original tpm2_pcr_extend() function
> is renamed to __tpm2_pcr_extend() and made static, it is called
> only from new tpm2_pcr_extend().
> 
> Fix warnings in __tpm2_pcr_extend()
> tpm2-cmd.c:251:16: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
> tpm2-cmd.c:252:17: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
> 
> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>

We do not want the signature change, especially because as we are
working on getting Roberto's changes in and also because it has
absolutely a zero gain. Who cares if those functions take different
parameters? I don't.

/Jarkko
Winkler, Tomas Oct. 2, 2018, 4:58 a.m. UTC | #2
> 
> On Sat, Sep 29, 2018 at 01:30:26AM +0300, Tomas Winkler wrote:
> > Add tpm2_pcr_extend() function to tpm2-cmd.c with signature required
> > by tpm-interface.c. It wraps the original open code implementation.
> > The original original tpm2_pcr_extend() function is renamed to
> > __tpm2_pcr_extend() and made static, it is called only from new
> > tpm2_pcr_extend().
> >
> > Fix warnings in __tpm2_pcr_extend()
> > tpm2-cmd.c:251:16: warning: comparison between signed and unsigned
> > integer expressions [-Wsign-compare]
> > tpm2-cmd.c:252:17: warning: comparison between signed and unsigned
> > integer expressions [-Wsign-compare]
> >
> > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> 
> We do not want the signature change, especially because as we are working
> on getting Roberto's changes in and also because it has absolutely a zero
> gain. Who cares if those functions take different parameters? I don't.

Yes, we do care this series tries to have a clean cut between 1.x  and 2.x specs. Please, let's finish one transformation and then move to another.
I understand that Roberto will have to rebase anyhow, if this series goes in first, if this is hard I can do it myself, it's trivial.

Tomas
Jarkko Sakkinen Oct. 3, 2018, 12:01 p.m. UTC | #3
On Tue, Oct 02, 2018 at 04:58:25AM +0000, Winkler, Tomas wrote:
> 
> 
> > 
> > On Sat, Sep 29, 2018 at 01:30:26AM +0300, Tomas Winkler wrote:
> > > Add tpm2_pcr_extend() function to tpm2-cmd.c with signature required
> > > by tpm-interface.c. It wraps the original open code implementation.
> > > The original original tpm2_pcr_extend() function is renamed to
> > > __tpm2_pcr_extend() and made static, it is called only from new
> > > tpm2_pcr_extend().
> > >
> > > Fix warnings in __tpm2_pcr_extend()
> > > tpm2-cmd.c:251:16: warning: comparison between signed and unsigned
> > > integer expressions [-Wsign-compare]
> > > tpm2-cmd.c:252:17: warning: comparison between signed and unsigned
> > > integer expressions [-Wsign-compare]
> > >
> > > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > 
> > We do not want the signature change, especially because as we are working
> > on getting Roberto's changes in and also because it has absolutely a zero
> > gain. Who cares if those functions take different parameters? I don't.
> 
> Yes, we do care this series tries to have a clean cut between 1.x  and 2.x specs. Please, let's finish one transformation and then move to another.
> I understand that Roberto will have to rebase anyhow, if this series goes in first, if this is hard I can do it myself, it's trivial.
> 
> Tomas

I'm happy to tune this minor stuff. I'll wait for Nayna to test
the one patch and make those adjustments :-) Would not make sense
to roll another series for these changes.

/Jarkko
Winkler, Tomas Oct. 3, 2018, 10:24 p.m. UTC | #4
> -----Original Message-----
> From: Jarkko Sakkinen [mailto:jarkko.sakkinen@linux.intel.com]
> Sent: Wednesday, October 03, 2018 15:02
> To: Winkler, Tomas <tomas.winkler@intel.com>
> Cc: Jason Gunthorpe <jgg@ziepe.ca>; Nayna Jain
> <nayna@linux.vnet.ibm.com>; Usyskin, Alexander
> <alexander.usyskin@intel.com>; Struk, Tadeusz <tadeusz.struk@intel.com>;
> linux-integrity@vger.kernel.org; linux-security-module@vger.kernel.org;
> linux-kernel@vger.kernel.org; roberto.sassu@huawei.com
> Subject: Re: [PATCH v5 12/21] tpm: move pcr extend code to tpm2-cmd.c
> 
> On Tue, Oct 02, 2018 at 04:58:25AM +0000, Winkler, Tomas wrote:
> >
> >
> > >
> > > On Sat, Sep 29, 2018 at 01:30:26AM +0300, Tomas Winkler wrote:
> > > > Add tpm2_pcr_extend() function to tpm2-cmd.c with signature
> > > > required by tpm-interface.c. It wraps the original open code
> implementation.
> > > > The original original tpm2_pcr_extend() function is renamed to
> > > > __tpm2_pcr_extend() and made static, it is called only from new
> > > > tpm2_pcr_extend().
> > > >
> > > > Fix warnings in __tpm2_pcr_extend()
> > > > tpm2-cmd.c:251:16: warning: comparison between signed and unsigned
> > > > integer expressions [-Wsign-compare]
> > > > tpm2-cmd.c:252:17: warning: comparison between signed and unsigned
> > > > integer expressions [-Wsign-compare]
> > > >
> > > > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > >
> > > We do not want the signature change, especially because as we are
> > > working on getting Roberto's changes in and also because it has
> > > absolutely a zero gain. Who cares if those functions take different
> parameters? I don't.
> >
> > Yes, we do care this series tries to have a clean cut between 1.x  and 2.x
> specs. Please, let's finish one transformation and then move to another.
> > I understand that Roberto will have to rebase anyhow, if this series goes in
> first, if this is hard I can do it myself, it's trivial.
> >
> > Tomas
> 
> I'm happy to tune this minor stuff.
What minor stuff?  This patch is just okay, let's change the API in next round.

 I'll wait for Nayna to test the one patch
> and make those adjustments :-) Would not make sense to roll another series
> for these changes.
I agree 
Tomas
Jarkko Sakkinen Oct. 4, 2018, 11:35 a.m. UTC | #5
On Wed, Oct 03, 2018 at 10:24:09PM +0000, Winkler, Tomas wrote:
> 
> 
> > -----Original Message-----
> > From: Jarkko Sakkinen [mailto:jarkko.sakkinen@linux.intel.com]
> > Sent: Wednesday, October 03, 2018 15:02
> > To: Winkler, Tomas <tomas.winkler@intel.com>
> > Cc: Jason Gunthorpe <jgg@ziepe.ca>; Nayna Jain
> > <nayna@linux.vnet.ibm.com>; Usyskin, Alexander
> > <alexander.usyskin@intel.com>; Struk, Tadeusz <tadeusz.struk@intel.com>;
> > linux-integrity@vger.kernel.org; linux-security-module@vger.kernel.org;
> > linux-kernel@vger.kernel.org; roberto.sassu@huawei.com
> > Subject: Re: [PATCH v5 12/21] tpm: move pcr extend code to tpm2-cmd.c
> > 
> > On Tue, Oct 02, 2018 at 04:58:25AM +0000, Winkler, Tomas wrote:
> > >
> > >
> > > >
> > > > On Sat, Sep 29, 2018 at 01:30:26AM +0300, Tomas Winkler wrote:
> > > > > Add tpm2_pcr_extend() function to tpm2-cmd.c with signature
> > > > > required by tpm-interface.c. It wraps the original open code
> > implementation.
> > > > > The original original tpm2_pcr_extend() function is renamed to
> > > > > __tpm2_pcr_extend() and made static, it is called only from new
> > > > > tpm2_pcr_extend().
> > > > >
> > > > > Fix warnings in __tpm2_pcr_extend()
> > > > > tpm2-cmd.c:251:16: warning: comparison between signed and unsigned
> > > > > integer expressions [-Wsign-compare]
> > > > > tpm2-cmd.c:252:17: warning: comparison between signed and unsigned
> > > > > integer expressions [-Wsign-compare]
> > > > >
> > > > > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > > >
> > > > We do not want the signature change, especially because as we are
> > > > working on getting Roberto's changes in and also because it has
> > > > absolutely a zero gain. Who cares if those functions take different
> > parameters? I don't.
> > >
> > > Yes, we do care this series tries to have a clean cut between 1.x  and 2.x
> > specs. Please, let's finish one transformation and then move to another.
> > > I understand that Roberto will have to rebase anyhow, if this series goes in
> > first, if this is hard I can do it myself, it's trivial.
> > >
> > > Tomas
> > 
> > I'm happy to tune this minor stuff.
> What minor stuff?  This patch is just okay, let's change the API in next round.

The patch is not okay because it does a completely unnecessary API
change.

/Jarkko
Jarkko Sakkinen Oct. 4, 2018, 11:36 a.m. UTC | #6
On Thu, Oct 04, 2018 at 02:35:02PM +0300, Jarkko Sakkinen wrote:
> On Wed, Oct 03, 2018 at 10:24:09PM +0000, Winkler, Tomas wrote:
> > 
> > 
> > > -----Original Message-----
> > > From: Jarkko Sakkinen [mailto:jarkko.sakkinen@linux.intel.com]
> > > Sent: Wednesday, October 03, 2018 15:02
> > > To: Winkler, Tomas <tomas.winkler@intel.com>
> > > Cc: Jason Gunthorpe <jgg@ziepe.ca>; Nayna Jain
> > > <nayna@linux.vnet.ibm.com>; Usyskin, Alexander
> > > <alexander.usyskin@intel.com>; Struk, Tadeusz <tadeusz.struk@intel.com>;
> > > linux-integrity@vger.kernel.org; linux-security-module@vger.kernel.org;
> > > linux-kernel@vger.kernel.org; roberto.sassu@huawei.com
> > > Subject: Re: [PATCH v5 12/21] tpm: move pcr extend code to tpm2-cmd.c
> > > 
> > > On Tue, Oct 02, 2018 at 04:58:25AM +0000, Winkler, Tomas wrote:
> > > >
> > > >
> > > > >
> > > > > On Sat, Sep 29, 2018 at 01:30:26AM +0300, Tomas Winkler wrote:
> > > > > > Add tpm2_pcr_extend() function to tpm2-cmd.c with signature
> > > > > > required by tpm-interface.c. It wraps the original open code
> > > implementation.
> > > > > > The original original tpm2_pcr_extend() function is renamed to
> > > > > > __tpm2_pcr_extend() and made static, it is called only from new
> > > > > > tpm2_pcr_extend().
> > > > > >
> > > > > > Fix warnings in __tpm2_pcr_extend()
> > > > > > tpm2-cmd.c:251:16: warning: comparison between signed and unsigned
> > > > > > integer expressions [-Wsign-compare]
> > > > > > tpm2-cmd.c:252:17: warning: comparison between signed and unsigned
> > > > > > integer expressions [-Wsign-compare]
> > > > > >
> > > > > > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > > > >
> > > > > We do not want the signature change, especially because as we are
> > > > > working on getting Roberto's changes in and also because it has
> > > > > absolutely a zero gain. Who cares if those functions take different
> > > parameters? I don't.
> > > >
> > > > Yes, we do care this series tries to have a clean cut between 1.x  and 2.x
> > > specs. Please, let's finish one transformation and then move to another.
> > > > I understand that Roberto will have to rebase anyhow, if this series goes in
> > > first, if this is hard I can do it myself, it's trivial.
> > > >
> > > > Tomas
> > > 
> > > I'm happy to tune this minor stuff.
> > What minor stuff?  This patch is just okay, let's change the API in next round.
> 
> The patch is not okay because it does a completely unnecessary API
> change.

Other minor stuff was missing commas in the list of return values if I
recall...

/Jarkko
Winkler, Tomas Oct. 4, 2018, 11:45 a.m. UTC | #7
> -----Original Message-----
> From: Jarkko Sakkinen [mailto:jarkko.sakkinen@linux.intel.com]
> Sent: Thursday, October 04, 2018 14:35
> To: Winkler, Tomas <tomas.winkler@intel.com>
> Cc: Jason Gunthorpe <jgg@ziepe.ca>; Nayna Jain
> <nayna@linux.vnet.ibm.com>; Usyskin, Alexander
> <alexander.usyskin@intel.com>; Struk, Tadeusz <tadeusz.struk@intel.com>;
> linux-integrity@vger.kernel.org; linux-security-module@vger.kernel.org;
> linux-kernel@vger.kernel.org; roberto.sassu@huawei.com
> Subject: Re: [PATCH v5 12/21] tpm: move pcr extend code to tpm2-cmd.c
> 
> On Wed, Oct 03, 2018 at 10:24:09PM +0000, Winkler, Tomas wrote:
> >
> >
> > > -----Original Message-----
> > > From: Jarkko Sakkinen [mailto:jarkko.sakkinen@linux.intel.com]
> > > Sent: Wednesday, October 03, 2018 15:02
> > > To: Winkler, Tomas <tomas.winkler@intel.com>
> > > Cc: Jason Gunthorpe <jgg@ziepe.ca>; Nayna Jain
> > > <nayna@linux.vnet.ibm.com>; Usyskin, Alexander
> > > <alexander.usyskin@intel.com>; Struk, Tadeusz
> > > <tadeusz.struk@intel.com>; linux-integrity@vger.kernel.org;
> > > linux-security-module@vger.kernel.org;
> > > linux-kernel@vger.kernel.org; roberto.sassu@huawei.com
> > > Subject: Re: [PATCH v5 12/21] tpm: move pcr extend code to
> > > tpm2-cmd.c
> > >
> > > On Tue, Oct 02, 2018 at 04:58:25AM +0000, Winkler, Tomas wrote:
> > > >
> > > >
> > > > >
> > > > > On Sat, Sep 29, 2018 at 01:30:26AM +0300, Tomas Winkler wrote:
> > > > > > Add tpm2_pcr_extend() function to tpm2-cmd.c with signature
> > > > > > required by tpm-interface.c. It wraps the original open code
> > > implementation.
> > > > > > The original original tpm2_pcr_extend() function is renamed to
> > > > > > __tpm2_pcr_extend() and made static, it is called only from
> > > > > > new tpm2_pcr_extend().
> > > > > >
> > > > > > Fix warnings in __tpm2_pcr_extend()
> > > > > > tpm2-cmd.c:251:16: warning: comparison between signed and
> > > > > > unsigned integer expressions [-Wsign-compare]
> > > > > > tpm2-cmd.c:252:17: warning: comparison between signed and
> > > > > > unsigned integer expressions [-Wsign-compare]
> > > > > >
> > > > > > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > > > >
> > > > > We do not want the signature change, especially because as we
> > > > > are working on getting Roberto's changes in and also because it
> > > > > has absolutely a zero gain. Who cares if those functions take
> > > > > different
> > > parameters? I don't.
> > > >
> > > > Yes, we do care this series tries to have a clean cut between 1.x
> > > > and 2.x
> > > specs. Please, let's finish one transformation and then move to another.
> > > > I understand that Roberto will have to rebase anyhow, if this
> > > > series goes in
> > > first, if this is hard I can do it myself, it's trivial.
> > > >
> > > > Tomas
> > >
> > > I'm happy to tune this minor stuff.
> > What minor stuff?  This patch is just okay, let's change the API in next
> round.
> 
> The patch is not okay because it does a completely unnecessary API change.
 
There is no API change, in that sense.
The exported API is in tpm-interface.c int tpm_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash)  
that is used is outside of the tpm reminds the same, only the open coded implementation of tpm2_pcr_extned has moved to
tpm2-cmd.c, This code is not called out of tpm module.
Please review the code again.

Thanks
Tomas
Roberto Sassu Oct. 4, 2018, 12:20 p.m. UTC | #8
On 10/4/2018 1:45 PM, Winkler, Tomas wrote:
> 
> 
>> -----Original Message-----
>> From: Jarkko Sakkinen [mailto:jarkko.sakkinen@linux.intel.com]
>> Sent: Thursday, October 04, 2018 14:35
>> To: Winkler, Tomas <tomas.winkler@intel.com>
>> Cc: Jason Gunthorpe <jgg@ziepe.ca>; Nayna Jain
>> <nayna@linux.vnet.ibm.com>; Usyskin, Alexander
>> <alexander.usyskin@intel.com>; Struk, Tadeusz <tadeusz.struk@intel.com>;
>> linux-integrity@vger.kernel.org; linux-security-module@vger.kernel.org;
>> linux-kernel@vger.kernel.org; roberto.sassu@huawei.com
>> Subject: Re: [PATCH v5 12/21] tpm: move pcr extend code to tpm2-cmd.c
>>
>> On Wed, Oct 03, 2018 at 10:24:09PM +0000, Winkler, Tomas wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Jarkko Sakkinen [mailto:jarkko.sakkinen@linux.intel.com]
>>>> Sent: Wednesday, October 03, 2018 15:02
>>>> To: Winkler, Tomas <tomas.winkler@intel.com>
>>>> Cc: Jason Gunthorpe <jgg@ziepe.ca>; Nayna Jain
>>>> <nayna@linux.vnet.ibm.com>; Usyskin, Alexander
>>>> <alexander.usyskin@intel.com>; Struk, Tadeusz
>>>> <tadeusz.struk@intel.com>; linux-integrity@vger.kernel.org;
>>>> linux-security-module@vger.kernel.org;
>>>> linux-kernel@vger.kernel.org; roberto.sassu@huawei.com
>>>> Subject: Re: [PATCH v5 12/21] tpm: move pcr extend code to
>>>> tpm2-cmd.c
>>>>
>>>> On Tue, Oct 02, 2018 at 04:58:25AM +0000, Winkler, Tomas wrote:
>>>>>
>>>>>
>>>>>>
>>>>>> On Sat, Sep 29, 2018 at 01:30:26AM +0300, Tomas Winkler wrote:
>>>>>>> Add tpm2_pcr_extend() function to tpm2-cmd.c with signature
>>>>>>> required by tpm-interface.c. It wraps the original open code
>>>> implementation.
>>>>>>> The original original tpm2_pcr_extend() function is renamed to
>>>>>>> __tpm2_pcr_extend() and made static, it is called only from
>>>>>>> new tpm2_pcr_extend().
>>>>>>>
>>>>>>> Fix warnings in __tpm2_pcr_extend()
>>>>>>> tpm2-cmd.c:251:16: warning: comparison between signed and
>>>>>>> unsigned integer expressions [-Wsign-compare]
>>>>>>> tpm2-cmd.c:252:17: warning: comparison between signed and
>>>>>>> unsigned integer expressions [-Wsign-compare]
>>>>>>>
>>>>>>> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
>>>>>>
>>>>>> We do not want the signature change, especially because as we
>>>>>> are working on getting Roberto's changes in and also because it
>>>>>> has absolutely a zero gain. Who cares if those functions take
>>>>>> different
>>>> parameters? I don't.
>>>>>
>>>>> Yes, we do care this series tries to have a clean cut between 1.x
>>>>> and 2.x
>>>> specs. Please, let's finish one transformation and then move to another.
>>>>> I understand that Roberto will have to rebase anyhow, if this
>>>>> series goes in
>>>> first, if this is hard I can do it myself, it's trivial.
>>>>>
>>>>> Tomas
>>>>
>>>> I'm happy to tune this minor stuff.
>>> What minor stuff?  This patch is just okay, let's change the API in next
>> round.
>>
>> The patch is not okay because it does a completely unnecessary API change.
>   
> There is no API change, in that sense.
> The exported API is in tpm-interface.c int tpm_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash)
> that is used is outside of the tpm reminds the same, only the open coded implementation of tpm2_pcr_extned has moved to
> tpm2-cmd.c, This code is not called out of tpm module.
> Please review the code again.

Hi Tomas

I will update tpm_pcr_extend() by replacing the array of u8 with an
array of tpm2_digest structures, so that the caller can provide multiple
digests with one call. The array of tpm2_digest structures will be
passed to tpm2_pcr_extend(). Please, don't modify the parameters of
tpm2_pcr_extend().

Thanks

Roberto
Winkler, Tomas Oct. 4, 2018, 1:46 p.m. UTC | #9
> 
> On 10/4/2018 1:45 PM, Winkler, Tomas wrote:
> >
> >
> >> -----Original Message-----
> >> From: Jarkko Sakkinen [mailto:jarkko.sakkinen@linux.intel.com]
> >> Sent: Thursday, October 04, 2018 14:35
> >> To: Winkler, Tomas <tomas.winkler@intel.com>
> >> Cc: Jason Gunthorpe <jgg@ziepe.ca>; Nayna Jain
> >> <nayna@linux.vnet.ibm.com>; Usyskin, Alexander
> >> <alexander.usyskin@intel.com>; Struk, Tadeusz
> >> <tadeusz.struk@intel.com>; linux-integrity@vger.kernel.org;
> >> linux-security-module@vger.kernel.org;
> >> linux-kernel@vger.kernel.org; roberto.sassu@huawei.com
> >> Subject: Re: [PATCH v5 12/21] tpm: move pcr extend code to tpm2-cmd.c
> >>
> >> On Wed, Oct 03, 2018 at 10:24:09PM +0000, Winkler, Tomas wrote:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Jarkko Sakkinen [mailto:jarkko.sakkinen@linux.intel.com]
> >>>> Sent: Wednesday, October 03, 2018 15:02
> >>>> To: Winkler, Tomas <tomas.winkler@intel.com>
> >>>> Cc: Jason Gunthorpe <jgg@ziepe.ca>; Nayna Jain
> >>>> <nayna@linux.vnet.ibm.com>; Usyskin, Alexander
> >>>> <alexander.usyskin@intel.com>; Struk, Tadeusz
> >>>> <tadeusz.struk@intel.com>; linux-integrity@vger.kernel.org;
> >>>> linux-security-module@vger.kernel.org;
> >>>> linux-kernel@vger.kernel.org; roberto.sassu@huawei.com
> >>>> Subject: Re: [PATCH v5 12/21] tpm: move pcr extend code to
> >>>> tpm2-cmd.c
> >>>>
> >>>> On Tue, Oct 02, 2018 at 04:58:25AM +0000, Winkler, Tomas wrote:
> >>>>>
> >>>>>
> >>>>>>
> >>>>>> On Sat, Sep 29, 2018 at 01:30:26AM +0300, Tomas Winkler wrote:
> >>>>>>> Add tpm2_pcr_extend() function to tpm2-cmd.c with signature
> >>>>>>> required by tpm-interface.c. It wraps the original open code
> >>>> implementation.
> >>>>>>> The original original tpm2_pcr_extend() function is renamed to
> >>>>>>> __tpm2_pcr_extend() and made static, it is called only from new
> >>>>>>> tpm2_pcr_extend().
> >>>>>>>
> >>>>>>> Fix warnings in __tpm2_pcr_extend()
> >>>>>>> tpm2-cmd.c:251:16: warning: comparison between signed and
> >>>>>>> unsigned integer expressions [-Wsign-compare]
> >>>>>>> tpm2-cmd.c:252:17: warning: comparison between signed and
> >>>>>>> unsigned integer expressions [-Wsign-compare]
> >>>>>>>
> >>>>>>> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> >>>>>>
> >>>>>> We do not want the signature change, especially because as we are
> >>>>>> working on getting Roberto's changes in and also because it has
> >>>>>> absolutely a zero gain. Who cares if those functions take
> >>>>>> different
> >>>> parameters? I don't.
> >>>>>
> >>>>> Yes, we do care this series tries to have a clean cut between 1.x
> >>>>> and 2.x
> >>>> specs. Please, let's finish one transformation and then move to
> another.
> >>>>> I understand that Roberto will have to rebase anyhow, if this
> >>>>> series goes in
> >>>> first, if this is hard I can do it myself, it's trivial.
> >>>>>
> >>>>> Tomas
> >>>>
> >>>> I'm happy to tune this minor stuff.
> >>> What minor stuff?  This patch is just okay, let's change the API in
> >>> next
> >> round.
> >>
> >> The patch is not okay because it does a completely unnecessary API
> change.
> >
> > There is no API change, in that sense.
> > The exported API is in tpm-interface.c int tpm_pcr_extend(struct
> > tpm_chip *chip, int pcr_idx, const u8 *hash) that is used is outside
> > of the tpm reminds the same, only the open coded implementation of
> tpm2_pcr_extned has moved to tpm2-cmd.c, This code is not called out of
> tpm module.
> > Please review the code again.
> 
> Hi Tomas
> 
> I will update tpm_pcr_extend() by replacing the array of u8 with an array of
> tpm2_digest structures, so that the caller can provide multiple digests with
> one call. The array of tpm2_digest structures will be passed to
> tpm2_pcr_extend(). Please, don't modify the parameters of
> tpm2_pcr_extend().

What about tpm1_pcr_extend/read()? 

Thanks
Tomas
Roberto Sassu Oct. 4, 2018, 2:10 p.m. UTC | #10
On 10/4/2018 3:46 PM, Winkler, Tomas wrote:
>>
>> On 10/4/2018 1:45 PM, Winkler, Tomas wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Jarkko Sakkinen [mailto:jarkko.sakkinen@linux.intel.com]
>>>> Sent: Thursday, October 04, 2018 14:35
>>>> To: Winkler, Tomas <tomas.winkler@intel.com>
>>>> Cc: Jason Gunthorpe <jgg@ziepe.ca>; Nayna Jain
>>>> <nayna@linux.vnet.ibm.com>; Usyskin, Alexander
>>>> <alexander.usyskin@intel.com>; Struk, Tadeusz
>>>> <tadeusz.struk@intel.com>; linux-integrity@vger.kernel.org;
>>>> linux-security-module@vger.kernel.org;
>>>> linux-kernel@vger.kernel.org; roberto.sassu@huawei.com
>>>> Subject: Re: [PATCH v5 12/21] tpm: move pcr extend code to tpm2-cmd.c
>>>>
>>>> On Wed, Oct 03, 2018 at 10:24:09PM +0000, Winkler, Tomas wrote:
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Jarkko Sakkinen [mailto:jarkko.sakkinen@linux.intel.com]
>>>>>> Sent: Wednesday, October 03, 2018 15:02
>>>>>> To: Winkler, Tomas <tomas.winkler@intel.com>
>>>>>> Cc: Jason Gunthorpe <jgg@ziepe.ca>; Nayna Jain
>>>>>> <nayna@linux.vnet.ibm.com>; Usyskin, Alexander
>>>>>> <alexander.usyskin@intel.com>; Struk, Tadeusz
>>>>>> <tadeusz.struk@intel.com>; linux-integrity@vger.kernel.org;
>>>>>> linux-security-module@vger.kernel.org;
>>>>>> linux-kernel@vger.kernel.org; roberto.sassu@huawei.com
>>>>>> Subject: Re: [PATCH v5 12/21] tpm: move pcr extend code to
>>>>>> tpm2-cmd.c
>>>>>>
>>>>>> On Tue, Oct 02, 2018 at 04:58:25AM +0000, Winkler, Tomas wrote:
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> On Sat, Sep 29, 2018 at 01:30:26AM +0300, Tomas Winkler wrote:
>>>>>>>>> Add tpm2_pcr_extend() function to tpm2-cmd.c with signature
>>>>>>>>> required by tpm-interface.c. It wraps the original open code
>>>>>> implementation.
>>>>>>>>> The original original tpm2_pcr_extend() function is renamed to
>>>>>>>>> __tpm2_pcr_extend() and made static, it is called only from new
>>>>>>>>> tpm2_pcr_extend().
>>>>>>>>>
>>>>>>>>> Fix warnings in __tpm2_pcr_extend()
>>>>>>>>> tpm2-cmd.c:251:16: warning: comparison between signed and
>>>>>>>>> unsigned integer expressions [-Wsign-compare]
>>>>>>>>> tpm2-cmd.c:252:17: warning: comparison between signed and
>>>>>>>>> unsigned integer expressions [-Wsign-compare]
>>>>>>>>>
>>>>>>>>> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
>>>>>>>>
>>>>>>>> We do not want the signature change, especially because as we are
>>>>>>>> working on getting Roberto's changes in and also because it has
>>>>>>>> absolutely a zero gain. Who cares if those functions take
>>>>>>>> different
>>>>>> parameters? I don't.
>>>>>>>
>>>>>>> Yes, we do care this series tries to have a clean cut between 1.x
>>>>>>> and 2.x
>>>>>> specs. Please, let's finish one transformation and then move to
>> another.
>>>>>>> I understand that Roberto will have to rebase anyhow, if this
>>>>>>> series goes in
>>>>>> first, if this is hard I can do it myself, it's trivial.
>>>>>>>
>>>>>>> Tomas
>>>>>>
>>>>>> I'm happy to tune this minor stuff.
>>>>> What minor stuff?  This patch is just okay, let's change the API in
>>>>> next
>>>> round.
>>>>
>>>> The patch is not okay because it does a completely unnecessary API
>> change.
>>>
>>> There is no API change, in that sense.
>>> The exported API is in tpm-interface.c int tpm_pcr_extend(struct
>>> tpm_chip *chip, int pcr_idx, const u8 *hash) that is used is outside
>>> of the tpm reminds the same, only the open coded implementation of
>> tpm2_pcr_extned has moved to tpm2-cmd.c, This code is not called out of
>> tpm module.
>>> Please review the code again.
>>
>> Hi Tomas
>>
>> I will update tpm_pcr_extend() by replacing the array of u8 with an array of
>> tpm2_digest structures, so that the caller can provide multiple digests with
>> one call. The array of tpm2_digest structures will be passed to
>> tpm2_pcr_extend(). Please, don't modify the parameters of
>> tpm2_pcr_extend().
> 
> What about tpm1_pcr_extend/read()?

tpm_pcr_extend/read() would pass to them the array of u8 from the
tpm2_digest structure. Check this patch:

[PATCH v2 2/3] tpm: modify tpm_pcr_read() definition to pass TPM hash
algorithms

Roberto
Jarkko Sakkinen Oct. 5, 2018, 11:31 a.m. UTC | #11
On Thu, Oct 04, 2018 at 11:45:30AM +0000, Winkler, Tomas wrote:
> There is no API change, in that sense.
> The exported API is in tpm-interface.c int tpm_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash)  
> that is used is outside of the tpm reminds the same, only the open coded implementation of tpm2_pcr_extned has moved to
> tpm2-cmd.c, This code is not called out of tpm module.
> Please review the code again.

I did now revisit this and you are right that my choice of word was not
exactly correct. I apologize for that. The patch introduces API that we
would take away and that does make much sense.

The best way to sort things out is to just fix the warnings and leave
the TPM 2.0 part open coded inside tpm_pcr_extend(). The rationale for
this is to avoid unnecessary mainline changes when ever possible (which
is bad for backporting for stable kernels).

> Thanks
> Tomas

/Jarkko
diff mbox series

Patch

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index d33060511a27..1ea83a3f1b02 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -491,31 +491,17 @@  EXPORT_SYMBOL_GPL(tpm_pcr_read);
 int tpm_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash)
 {
 	int rc;
-	struct tpm2_digest digest_list[ARRAY_SIZE(chip->active_banks)];
-	u32 count = 0;
-	int i;
 
 	chip = tpm_find_get_ops(chip);
 	if (!chip)
 		return -ENODEV;
 
-	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
-		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++;
-		}
-
-		rc = tpm2_pcr_extend(chip, pcr_idx, count, digest_list);
-		tpm_put_ops(chip);
-		return rc;
-	}
+	if (chip->flags & TPM_CHIP_FLAG_TPM2)
+		rc = tpm2_pcr_extend(chip, pcr_idx, hash);
+	else
+		rc = tpm1_pcr_extend(chip, pcr_idx, hash,
+				     "attempting extend a PCR value");
 
-	rc = tpm1_pcr_extend(chip, pcr_idx, hash,
-			     "attempting extend a PCR value");
 	tpm_put_ops(chip);
 	return rc;
 }
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index fa88102a0cab..f0963a0a8acd 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -592,8 +592,7 @@  static inline u32 tpm2_rc_value(u32 rc)
 
 int tpm2_get_timeouts(struct tpm_chip *chip);
 int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx, u8 *res_buf);
-int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, u32 count,
-		    struct tpm2_digest *digests);
+int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash);
 int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max);
 void tpm2_flush_context_cmd(struct tpm_chip *chip, u32 handle,
 			    unsigned int flags);
diff --git a/drivers/char/tpm/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c
index 5b2e743a3e51..8a84db315676 100644
--- a/drivers/char/tpm/tpm1-cmd.c
+++ b/drivers/char/tpm/tpm1-cmd.c
@@ -312,7 +312,7 @@  unsigned long tpm1_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal)
 #define TPM_ST_CLEAR 1
 
 /**
- * tpm_startup - turn on the TPM
+ * tpm_startup() - turn on the TPM
  * @chip: TPM chip to use
  *
  * Normally the firmware should start the TPM. This function is provided as a
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index a3d39360620f..2b705b00db78 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -216,23 +216,21 @@  struct tpm2_null_auth_area {
 } __packed;
 
 /**
- * tpm2_pcr_extend() - extend a PCR value
- *
- * @chip:	TPM chip to use.
- * @pcr_idx:	index of the PCR.
- * @count:	number of digests passed.
- * @digests:	list of pcr banks and corresponding digest values to extend.
+ * __tpm2_pcr_extend() - extend a PCR value
+ * @chip:       TPM chip to use.
+ * @pcr_idx:    index of the PCR.
+ * @count:      number of digests passed.
+ * @digests:    list of pcr banks and corresponding digest values to extend.
  *
  * Return: Same as with tpm_transmit_cmd.
  */
-int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, u32 count,
-		    struct tpm2_digest *digests)
+static int __tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, u32 count,
+			     struct tpm2_digest *digests)
 {
 	struct tpm_buf buf;
 	struct tpm2_null_auth_area auth_area;
+	u32 i, j;
 	int rc;
-	int i;
-	int j;
 
 	if (count > ARRAY_SIZE(chip->active_banks))
 		return -EINVAL;
@@ -272,6 +270,34 @@  int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, u32 count,
 	return rc;
 }
 
+/**
+ * tpm2_pcr_extend() - extend a PCR value
+ * @chip:       TPM chip to use.
+ * @pcr_idx:    index of the PCR to extend.
+ * @hash:       the hash of a measurement.
+ *
+ * Return: Same as with tpm_transmit_cmd.
+ */
+int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash)
+{
+	int rc;
+	struct tpm2_digest digest_list[ARRAY_SIZE(chip->active_banks)];
+	u32 count = 0;
+	unsigned int i;
+
+	memset(digest_list, 0, sizeof(digest_list));
+	for (i = 0; i < ARRAY_SIZE(chip->active_banks); i++) {
+		if (chip->active_banks[i] == TPM2_ALG_ERROR)
+			break;
+		digest_list[i].alg_id = chip->active_banks[i];
+		memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE);
+		count++;
+	}
+
+	rc = __tpm2_pcr_extend(chip, pcr_idx, count, digest_list);
+	return rc;
+}
+
 
 struct tpm2_get_random_out {
 	__be16 size;