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 |
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
> > 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
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
> -----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
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
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
> -----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
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
> > 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
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
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 --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;
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(-)