Re: [tpmdd-devel] [PATCH v9 2/2] tpm: add securityfs support,for TPM 2.0 firmware event log

Re: [tpmdd-devel] [PATCH v9 2/2] tpm: add securityfs support,for TPM 2.0 firmware event log

[Ken Goldman]

> You do not need to send a new patch set version as long as this
> one gets peer tested. And it needs to be tested without hacks
> like plumbing TCPA with TPM 2.0 in QEMU. OF code paths needs to
> be peer tested to be more specific.
>
> For me the code itself looks good but I simply cannot take it in
> in the current situation.
>
> /Jarkko

Tested-by: Kenneth Goldman <kgold@linux.vnet.ibm.com>

I validated a firmware event log taken from a Power 8 against PCR 0-7 
values for the SHA-1 and SHA-256 banks from a Nuvoton TPM 2.0 chip on 
that same platform.

Re: [tpmdd-devel] [PATCH v9 2/2] tpm: add securityfs support,for TPM 2.0 firmware event log

[Jarkko Sakkinen]

On Wed, Jan 25, 2017 at 04:22:45PM -0500, Ken Goldman wrote:
> > You do not need to send a new patch set version as long as this
> > one gets peer tested. And it needs to be tested without hacks
> > like plumbing TCPA with TPM 2.0 in QEMU. OF code paths needs to
> > be peer tested to be more specific.
> > 
> > For me the code itself looks good but I simply cannot take it in
> > in the current situation.
> > 
> > /Jarkko
> 
> Tested-by: Kenneth Goldman <kgold@linux.vnet.ibm.com>
> 
> I validated a firmware event log taken from a Power 8 against PCR 0-7 values
> for the SHA-1 and SHA-256 banks from a Nuvoton TPM 2.0 chip on that same
> platform.

Thank you! I'll add this to the commit.

/Jarkko

Re: Fwd: Re: [tpmdd-devel] [PATCH v9 2/2] tpm: add securityfs support,for TPM 2.0 firmware event log

[Nayna]

> From: "Ken Goldman" <kgold@linux.vnet.ibm.com
> <mailto:kgold@linux.vnet.ibm.com>>
> Date: 26-Jan-2017 2:53 AM
> Subject: Re: [tpmdd-devel] [PATCH v9 2/2] tpm: add securityfs
> support,for TPM 2.0 firmware event log
> To: <linux-kernel@vger.kernel.org
> <mailto:linux-kernel@vger.kernel.org>>,
> <linux-security-module@vger.kernel.org
> <mailto:linux-security-module@vger.kernel.org>>,
> <tpmdd-devel@lists.sourceforge.net
> <mailto:tpmdd-devel@lists.sourceforge.net>>
> Cc:
>
>     You do not need to send a new patch set version as long as this
>     one gets peer tested. And it needs to be tested without hacks
>     like plumbing TCPA with TPM 2.0 in QEMU. OF code paths needs to
>     be peer tested to be more specific.
>
>     For me the code itself looks good but I simply cannot take it in
>     in the current situation.
>
>     /Jarkko
>
>
> Tested-by: Kenneth Goldman <kgold@linux.vnet.ibm.com
> <mailto:kgold@linux.vnet.ibm.com>>
>
> I validated a firmware event log taken from a Power 8 against PCR 0-7
> values for the SHA-1 and SHA-256 banks from a Nuvoton TPM 2.0 chip on
> that same platform.
>

Thank You Ken.

Jarkko, I hope now these patches can be accepted for 4.11.

Thanks & Regards,
    - Nayna


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

Re: Fwd: Re: [tpmdd-devel] [PATCH v9 2/2] tpm: add securityfs support,for TPM 2.0 firmware event log

[Jarkko Sakkinen]

On Mon, Jan 30, 2017 at 03:08:42PM +0530, Nayna wrote:
> 
> > From: "Ken Goldman" <kgold@linux.vnet.ibm.com
> > <mailto:kgold@linux.vnet.ibm.com>>
> > Date: 26-Jan-2017 2:53 AM
> > Subject: Re: [tpmdd-devel] [PATCH v9 2/2] tpm: add securityfs
> > support,for TPM 2.0 firmware event log
> > To: <linux-kernel@vger.kernel.org
> > <mailto:linux-kernel@vger.kernel.org>>,
> > <linux-security-module@vger.kernel.org
> > <mailto:linux-security-module@vger.kernel.org>>,
> > <tpmdd-devel@lists.sourceforge.net
> > <mailto:tpmdd-devel@lists.sourceforge.net>>
> > Cc:
> > 
> >     You do not need to send a new patch set version as long as this
> >     one gets peer tested. And it needs to be tested without hacks
> >     like plumbing TCPA with TPM 2.0 in QEMU. OF code paths needs to
> >     be peer tested to be more specific.
> > 
> >     For me the code itself looks good but I simply cannot take it in
> >     in the current situation.
> > 
> >     /Jarkko
> > 
> > 
> > Tested-by: Kenneth Goldman <kgold@linux.vnet.ibm.com
> > <mailto:kgold@linux.vnet.ibm.com>>
> > 
> > I validated a firmware event log taken from a Power 8 against PCR 0-7
> > values for the SHA-1 and SHA-256 banks from a Nuvoton TPM 2.0 chip on
> > that same platform.
> > 
> 
> Thank You Ken.
> 
> Jarkko, I hope now these patches can be accepted for 4.11.
> 
> Thanks & Regards,
>    - Nayna

Thanks Ken. I somehow managed to miss that response.

/Jarkko

Re: [tpmdd-devel] [PATCH v9 2/2] tpm: add securityfs support, for TPM 2.0 firmware event log

[Jarkko Sakkinen]

On Wed, Jan 25, 2017 at 04:22:45PM -0500, Ken Goldman wrote:
> > You do not need to send a new patch set version as long as this
> > one gets peer tested. And it needs to be tested without hacks
> > like plumbing TCPA with TPM 2.0 in QEMU. OF code paths needs to
> > be peer tested to be more specific.
> >
> > For me the code itself looks good but I simply cannot take it in
> > in the current situation.
> >
> > /Jarkko
> 
> Tested-by: Kenneth Goldman <kgold@linux.vnet.ibm.com>

Thanks. This explains it. I got Naynas response before I got this one.
That's why it went unnoticed.

/Jarkko

Re: Fwd: Re: [tpmdd-devel] [PATCH v9 2/2] tpm: add securityfs support,for TPM 2.0 firmware event log

[Jarkko Sakkinen]

On Mon, Jan 30, 2017 at 03:08:42PM +0530, Nayna wrote:
> 
> > From: "Ken Goldman" <kgold@linux.vnet.ibm.com
> > <mailto:kgold@linux.vnet.ibm.com>>
> > Date: 26-Jan-2017 2:53 AM
> > Subject: Re: [tpmdd-devel] [PATCH v9 2/2] tpm: add securityfs
> > support,for TPM 2.0 firmware event log
> > To: <linux-kernel@vger.kernel.org
> > <mailto:linux-kernel@vger.kernel.org>>,
> > <linux-security-module@vger.kernel.org
> > <mailto:linux-security-module@vger.kernel.org>>,
> > <tpmdd-devel@lists.sourceforge.net
> > <mailto:tpmdd-devel@lists.sourceforge.net>>
> > Cc:
> > 
> >     You do not need to send a new patch set version as long as this
> >     one gets peer tested. And it needs to be tested without hacks
> >     like plumbing TCPA with TPM 2.0 in QEMU. OF code paths needs to
> >     be peer tested to be more specific.
> > 
> >     For me the code itself looks good but I simply cannot take it in
> >     in the current situation.
> > 
> >     /Jarkko
> > 
> > 
> > Tested-by: Kenneth Goldman <kgold@linux.vnet.ibm.com
> > <mailto:kgold@linux.vnet.ibm.com>>
> > 
> > I validated a firmware event log taken from a Power 8 against PCR 0-7
> > values for the SHA-1 and SHA-256 banks from a Nuvoton TPM 2.0 chip on
> > that same platform.
> > 
> 
> Thank You Ken.
> 
> Jarkko, I hope now these patches can be accepted for 4.11.
> 
> Thanks & Regards,
>    - Nayna

I already sent my pull request to 4.11 and even today I found something
fishy. You declared a function local array by using a variable in "tpm:
enhance TPM 2.0 PCR extend to support multiple banks" (max_active_banks
or something). And the event log patches have just passed the review. 

I've applied them to my tree but I'll only include bug fixes for 4.11
pull requests. You'll have to wait till' 4.12.

/Jarkko

Re: Fwd: Re: [tpmdd-devel] [PATCH v9 2/2] tpm: add securityfs support,for TPM 2.0 firmware event log

[Nayna]

On 01/31/2017 11:16 PM, Jarkko Sakkinen wrote:
> On Mon, Jan 30, 2017 at 03:08:42PM +0530, Nayna wrote:
>>
>>> From: "Ken Goldman" <kgold@linux.vnet.ibm.com
>>> <mailto:kgold@linux.vnet.ibm.com>>
>>> Date: 26-Jan-2017 2:53 AM
>>> Subject: Re: [tpmdd-devel] [PATCH v9 2/2] tpm: add securityfs
>>> support,for TPM 2.0 firmware event log
>>> To: <linux-kernel@vger.kernel.org
>>> <mailto:linux-kernel@vger.kernel.org>>,
>>> <linux-security-module@vger.kernel.org
>>> <mailto:linux-security-module@vger.kernel.org>>,
>>> <tpmdd-devel@lists.sourceforge.net
>>> <mailto:tpmdd-devel@lists.sourceforge.net>>
>>> Cc:
>>>
>>>      You do not need to send a new patch set version as long as this
>>>      one gets peer tested. And it needs to be tested without hacks
>>>      like plumbing TCPA with TPM 2.0 in QEMU. OF code paths needs to
>>>      be peer tested to be more specific.
>>>
>>>      For me the code itself looks good but I simply cannot take it in
>>>      in the current situation.
>>>
>>>      /Jarkko
>>>
>>>
>>> Tested-by: Kenneth Goldman <kgold@linux.vnet.ibm.com
>>> <mailto:kgold@linux.vnet.ibm.com>>
>>>
>>> I validated a firmware event log taken from a Power 8 against PCR 0-7
>>> values for the SHA-1 and SHA-256 banks from a Nuvoton TPM 2.0 chip on
>>> that same platform.
>>>
>>
>> Thank You Ken.
>>
>> Jarkko, I hope now these patches can be accepted for 4.11.
>>
>> Thanks & Regards,
>>     - Nayna
>
> I already sent my pull request to 4.11 and even today I found something
> fishy. You declared a function local array by using a variable in "tpm:
> enhance TPM 2.0 PCR extend to support multiple banks" (max_active_banks
> or something). And the event log patches have just passed the review.

Yes. I have checked using clang and it has passed the clang.. and I also 
verified there were no complains during build.

What type of problem do you see ?

Also, to understand, this is related to multi-bank patchset. I mean how 
does it affect for event log patchset ?

Thanks & Regards,
    - Nayna

>
> I've applied them to my tree but I'll only include bug fixes for 4.11
> pull requests. You'll have to wait till' 4.12.
>
> /Jarkko
>

Re: Fwd: Re: [tpmdd-devel] [PATCH v9 2/2] tpm: add securityfs support,for TPM 2.0 firmware event log

[Jarkko Sakkinen]

On Tue, Jan 31, 2017 at 07:46:59PM +0200, Jarkko Sakkinen wrote:
> On Mon, Jan 30, 2017 at 03:08:42PM +0530, Nayna wrote:
> > 
> > > From: "Ken Goldman" <kgold@linux.vnet.ibm.com
> > > <mailto:kgold@linux.vnet.ibm.com>>
> > > Date: 26-Jan-2017 2:53 AM
> > > Subject: Re: [tpmdd-devel] [PATCH v9 2/2] tpm: add securityfs
> > > support,for TPM 2.0 firmware event log
> > > To: <linux-kernel@vger.kernel.org
> > > <mailto:linux-kernel@vger.kernel.org>>,
> > > <linux-security-module@vger.kernel.org
> > > <mailto:linux-security-module@vger.kernel.org>>,
> > > <tpmdd-devel@lists.sourceforge.net
> > > <mailto:tpmdd-devel@lists.sourceforge.net>>
> > > Cc:
> > > 
> > >     You do not need to send a new patch set version as long as this
> > >     one gets peer tested. And it needs to be tested without hacks
> > >     like plumbing TCPA with TPM 2.0 in QEMU. OF code paths needs to
> > >     be peer tested to be more specific.
> > > 
> > >     For me the code itself looks good but I simply cannot take it in
> > >     in the current situation.
> > > 
> > >     /Jarkko
> > > 
> > > 
> > > Tested-by: Kenneth Goldman <kgold@linux.vnet.ibm.com
> > > <mailto:kgold@linux.vnet.ibm.com>>
> > > 
> > > I validated a firmware event log taken from a Power 8 against PCR 0-7
> > > values for the SHA-1 and SHA-256 banks from a Nuvoton TPM 2.0 chip on
> > > that same platform.
> > > 
> > 
> > Thank You Ken.
> > 
> > Jarkko, I hope now these patches can be accepted for 4.11.
> > 
> > Thanks & Regards,
> >    - Nayna
> 
> I already sent my pull request to 4.11 and even today I found something
> fishy. You declared a function local array by using a variable in "tpm:
> enhance TPM 2.0 PCR extend to support multiple banks" (max_active_banks
> or something). And the event log patches have just passed the review. 
> 
> I've applied them to my tree but I'll only include bug fixes for 4.11
> pull requests. You'll have to wait till' 4.12.

James,

The discussion is about two features:

1. Extension to tpm_pcr_extend() (used by IMA) to extend all PCR banks
   instead of just SHA-1 banks. It is recommended by TCG to do so in
   order to prevent malicious use of PCRs.
2. TPM 2.0 event log with backend support for OF device tree (for
   getting address where you can grab it).

These are required as baseline to implement full TPM 2.0 support for
IMA. The commits are fairly well baked and went through many iterations.
I've tested tpm_pcr_extend() patches. I haven't tested event log patches
but have extensively reviewed them and Ken Goldman has tested them with
POWER hardware.

I don't believe that there is major risk to put them already into 4.11
but it is fairly late so I just want a second opinion before putting
them into pull request.

I have some small scoped bug fixes that I will send to you in any case
(unrelated to this work).

/Jarkko

Re: Fwd: Re: [tpmdd-devel] [PATCH v9 2/2] tpm: add securityfs support,for TPM 2.0 firmware event log

[Jarkko Sakkinen]

On Wed, Feb 01, 2017 at 12:14:12AM +0530, Nayna wrote:
> > I already sent my pull request to 4.11 and even today I found something
> > fishy. You declared a function local array by using a variable in "tpm:
> > enhance TPM 2.0 PCR extend to support multiple banks" (max_active_banks
> > or something). And the event log patches have just passed the review.
> 
> Yes. I have checked using clang and it has passed the clang.. and I also
> verified there were no complains during build.

What we can deduce from that is that they didn't expose the issue in
question.

I found this by running sparse with make C=2 M=drives/char/tpm

> What type of problem do you see ?

It is disallowed to do stack allocation in the kernel code even if C
standard would allow it. Stack is scarce resource so you need to know
its usage at compile time.

In this case you actually know the allocation because the value is not
changed during the course of the function but it is still bad. Probably
compiler will optimize it out. Still it is not a good practice.

> Also, to understand, this is related to multi-bank patchset. I mean how does
> it affect for event log patchset ?

Well in both cases these have landed fairly late but I asked from James
whether I'll have to postpone these to 4.12.

Usually when I've sent my release pull request I do not want to make any
radical changes to the codebase because they always require extra QA and
thus take extra time.

/Jarkko

Re: Fwd: Re: [tpmdd-devel] [PATCH v9 2/2] tpm: add securityfs support,for TPM 2.0 firmware event log

[James Morris]

On Tue, 31 Jan 2017, Jarkko Sakkinen wrote:

> James,
> 
> The discussion is about two features:
> 
> 1. Extension to tpm_pcr_extend() (used by IMA) to extend all PCR banks
>    instead of just SHA-1 banks. It is recommended by TCG to do so in
>    order to prevent malicious use of PCRs.
> 2. TPM 2.0 event log with backend support for OF device tree (for
>    getting address where you can grab it).
> 
> These are required as baseline to implement full TPM 2.0 support for
> IMA. The commits are fairly well baked and went through many iterations.
> I've tested tpm_pcr_extend() patches. I haven't tested event log patches
> but have extensively reviewed them and Ken Goldman has tested them with
> POWER hardware.
> 
> I don't believe that there is major risk to put them already into 4.11
> but it is fairly late so I just want a second opinion before putting
> them into pull request.
> 

I'll take this for 4.11.  IMA + TPM 2.0 is still developmental and not in 
wide use, afaik.



-- 
James Morris
<jmorris@namei.org>

Re: Fwd: Re: [tpmdd-devel] [PATCH v9 2/2] tpm: add securityfs support,for TPM 2.0 firmware event log

[Jarkko Sakkinen]

On Wed, Feb 01, 2017 at 08:46:32AM +1100, James Morris wrote:
> On Tue, 31 Jan 2017, Jarkko Sakkinen wrote:
> 
> > James,
> > 
> > The discussion is about two features:
> > 
> > 1. Extension to tpm_pcr_extend() (used by IMA) to extend all PCR banks
> >    instead of just SHA-1 banks. It is recommended by TCG to do so in
> >    order to prevent malicious use of PCRs.
> > 2. TPM 2.0 event log with backend support for OF device tree (for
> >    getting address where you can grab it).
> > 
> > These are required as baseline to implement full TPM 2.0 support for
> > IMA. The commits are fairly well baked and went through many iterations.
> > I've tested tpm_pcr_extend() patches. I haven't tested event log patches
> > but have extensively reviewed them and Ken Goldman has tested them with
> > POWER hardware.
> > 
> > I don't believe that there is major risk to put them already into 4.11
> > but it is fairly late so I just want a second opinion before putting
> > them into pull request.
> > 
> 
> I'll take this for 4.11.  IMA + TPM 2.0 is still developmental and not in 
> wide use, afaik.
> 
> 
> 
> -- 
> James Morris
> <jmorris@namei.org>

Great! I'll prepare the pull request tomorrow (it's now 12:30AM in
Finland). Thank you.

/Jarkko

Re: Fwd: Re: [tpmdd-devel] [PATCH v9 2/2] tpm: add securityfs support,for TPM 2.0 firmware event log

[Mimi Zohar]

On Wed, 2017-02-01 at 08:46 +1100, James Morris wrote:
> On Tue, 31 Jan 2017, Jarkko Sakkinen wrote:
> 
> > James,
> > 
> > The discussion is about two features:
> > 
> > 1. Extension to tpm_pcr_extend() (used by IMA) to extend all PCR banks
> >    instead of just SHA-1 banks. It is recommended by TCG to do so in
> >    order to prevent malicious use of PCRs.
> > 2. TPM 2.0 event log with backend support for OF device tree (for
> >    getting address where you can grab it).
> > 
> > These are required as baseline to implement full TPM 2.0 support for
> > IMA. The commits are fairly well baked and went through many iterations.
> > I've tested tpm_pcr_extend() patches. I haven't tested event log patches
> > but have extensively reviewed them and Ken Goldman has tested them with
> > POWER hardware.
> > 
> > I don't believe that there is major risk to put them already into 4.11
> > but it is fairly late so I just want a second opinion before putting
> > them into pull request.
> > 
> 
> I'll take this for 4.11.  IMA + TPM 2.0 is still developmental and not in 
> wide use, afaik.

Thank you!   FYI, I've been running with them as well.

Mimi

Re: Fwd: Re: [tpmdd-devel] [PATCH v9 2/2] tpm: add securityfs support,for TPM 2.0 firmware event log

[Nayna]

On 02/01/2017 02:20 AM, Jarkko Sakkinen wrote:
> On Wed, Feb 01, 2017 at 12:14:12AM +0530, Nayna wrote:
>>> I already sent my pull request to 4.11 and even today I found something
>>> fishy. You declared a function local array by using a variable in "tpm:
>>> enhance TPM 2.0 PCR extend to support multiple banks" (max_active_banks
>>> or something). And the event log patches have just passed the review.
>>
>> Yes. I have checked using clang and it has passed the clang.. and I also
>> verified there were no complains during build.
>
> What we can deduce from that is that they didn't expose the issue in
> question.
>
> I found this by running sparse with make C=2 M=drives/char/tpm
>
>> What type of problem do you see ?
>
> It is disallowed to do stack allocation in the kernel code even if C
> standard would allow it. Stack is scarce resource so you need to know
> its usage at compile time.
>
> In this case you actually know the allocation because the value is not
> changed during the course of the function but it is still bad. Probably
> compiler will optimize it out. Still it is not a good practice.

Thanks Jarkko for explaining it.

Hmm, do you want me to send a patch for this ?
I think what we want is actually define it just array of size as 7.

>
>> Also, to understand, this is related to multi-bank patchset. I mean how does
>> it affect for event log patchset ?
>
> Well in both cases these have landed fairly late but I asked from James
> whether I'll have to postpone these to 4.12.
>
> Usually when I've sent my release pull request I do not want to make any
> radical changes to the codebase because they always require extra QA and
> thus take extra time.

Thanks again for explaining details.

Thanks & Regards,
    - Nayna

>
> /Jarkko
>

Re: Fwd: Re: [tpmdd-devel] [PATCH v9 2/2] tpm: add securityfs support,for TPM 2.0 firmware event log

[Jarkko Sakkinen]

On Tue, Jan 31, 2017 at 05:31:50PM -0500, Mimi Zohar wrote:
> On Wed, 2017-02-01 at 08:46 +1100, James Morris wrote:
> > On Tue, 31 Jan 2017, Jarkko Sakkinen wrote:
> > 
> > > James,
> > > 
> > > The discussion is about two features:
> > > 
> > > 1. Extension to tpm_pcr_extend() (used by IMA) to extend all PCR banks
> > >    instead of just SHA-1 banks. It is recommended by TCG to do so in
> > >    order to prevent malicious use of PCRs.
> > > 2. TPM 2.0 event log with backend support for OF device tree (for
> > >    getting address where you can grab it).
> > > 
> > > These are required as baseline to implement full TPM 2.0 support for
> > > IMA. The commits are fairly well baked and went through many iterations.
> > > I've tested tpm_pcr_extend() patches. I haven't tested event log patches
> > > but have extensively reviewed them and Ken Goldman has tested them with
> > > POWER hardware.
> > > 
> > > I don't believe that there is major risk to put them already into 4.11
> > > but it is fairly late so I just want a second opinion before putting
> > > them into pull request.
> > > 
> > 
> > I'll take this for 4.11.  IMA + TPM 2.0 is still developmental and not in 
> > wide use, afaik.
> 
> Thank you!   FYI, I've been running with them as well.
> 
> Mimi

OK, so can I also add your Tested-by to these four commits?

/Jarkko

Re: Fwd: Re: [tpmdd-devel] [PATCH v9 2/2] tpm: add securityfs support,for TPM 2.0 firmware event log

[Mimi Zohar]

On Wed, 2017-02-01 at 12:30 +0200, Jarkko Sakkinen wrote:
> On Tue, Jan 31, 2017 at 05:31:50PM -0500, Mimi Zohar wrote:
> > On Wed, 2017-02-01 at 08:46 +1100, James Morris wrote:
> > > On Tue, 31 Jan 2017, Jarkko Sakkinen wrote:
> > > 
> > > > James,
> > > > 
> > > > The discussion is about two features:
> > > > 
> > > > 1. Extension to tpm_pcr_extend() (used by IMA) to extend all PCR banks
> > > >    instead of just SHA-1 banks. It is recommended by TCG to do so in
> > > >    order to prevent malicious use of PCRs.
> > > > 2. TPM 2.0 event log with backend support for OF device tree (for
> > > >    getting address where you can grab it).
> > > > 
> > > > These are required as baseline to implement full TPM 2.0 support for
> > > > IMA. The commits are fairly well baked and went through many iterations.
> > > > I've tested tpm_pcr_extend() patches. I haven't tested event log patches
> > > > but have extensively reviewed them and Ken Goldman has tested them with
> > > > POWER hardware.
> > > > 
> > > > I don't believe that there is major risk to put them already into 4.11
> > > > but it is fairly late so I just want a second opinion before putting
> > > > them into pull request.
> > > > 
> > > 
> > > I'll take this for 4.11.  IMA + TPM 2.0 is still developmental and not in 
> > > wide use, afaik.
> > 
> > Thank you!   FYI, I've been running with them as well.
> > 
> > Mimi
> 
> OK, so can I also add your Tested-by to these four commits?

I provided Ken with the firmware event log, IMA measurement list, and
the PCRs for testing.   Ken's tested-by is fine.  Missing is Stefan's
tested-by - https://sourceforge.net/p/tpmdd/mailman/message/35602588/
I would really appreciate your adding his tested-by.

thanks,

Mimi

Re: Fwd: Re: [tpmdd-devel] [PATCH v9 2/2] tpm: add securityfs support,for TPM 2.0 firmware event log

[Jarkko Sakkinen]

On Tue, Jan 31, 2017 at 10:50:06PM +0200, Jarkko Sakkinen wrote:
> On Wed, Feb 01, 2017 at 12:14:12AM +0530, Nayna wrote:
> > > I already sent my pull request to 4.11 and even today I found something
> > > fishy. You declared a function local array by using a variable in "tpm:
> > > enhance TPM 2.0 PCR extend to support multiple banks" (max_active_banks
> > > or something). And the event log patches have just passed the review.
> > 
> > Yes. I have checked using clang and it has passed the clang.. and I also
> > verified there were no complains during build.
> 
> What we can deduce from that is that they didn't expose the issue in
> question.
> 
> I found this by running sparse with make C=2 M=drives/char/tpm
> 
> > What type of problem do you see ?
> 
> It is disallowed to do stack allocation in the kernel code even if C
> standard would allow it. Stack is scarce resource so you need to know
> its usage at compile time.
> 
> In this case you actually know the allocation because the value is not
> changed during the course of the function but it is still bad. Probably
> compiler will optimize it out. Still it is not a good practice.
> 
> > Also, to understand, this is related to multi-bank patchset. I mean how does
> > it affect for event log patchset ?
> 
> Well in both cases these have landed fairly late but I asked from James
> whether I'll have to postpone these to 4.12.
> 
> Usually when I've sent my release pull request I do not want to make any
> radical changes to the codebase because they always require extra QA and
> thus take extra time.

rc = tpm_transmit_cmd(chip, buf.data, tpm_buf_length(&buf), 0, 0,
		      "attempting extend a PCR value");

This should be

rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, 0, 0,
		      "attempting extend a PCR value");

The second parameter is the size of the buffer, not length of the input
data.

I'll update this...

/Jarkko

Re: Fwd: Re: [tpmdd-devel] [PATCH v9 2/2] tpm: add securityfs support,for TPM 2.0 firmware event log

[Jarkko Sakkinen]

On Wed, Feb 01, 2017 at 04:48:37PM +0200, Jarkko Sakkinen wrote:
> On Tue, Jan 31, 2017 at 10:50:06PM +0200, Jarkko Sakkinen wrote:
> > On Wed, Feb 01, 2017 at 12:14:12AM +0530, Nayna wrote:
> > > > I already sent my pull request to 4.11 and even today I found something
> > > > fishy. You declared a function local array by using a variable in "tpm:
> > > > enhance TPM 2.0 PCR extend to support multiple banks" (max_active_banks
> > > > or something). And the event log patches have just passed the review.
> > > 
> > > Yes. I have checked using clang and it has passed the clang.. and I also
> > > verified there were no complains during build.
> > 
> > What we can deduce from that is that they didn't expose the issue in
> > question.
> > 
> > I found this by running sparse with make C=2 M=drives/char/tpm
> > 
> > > What type of problem do you see ?
> > 
> > It is disallowed to do stack allocation in the kernel code even if C
> > standard would allow it. Stack is scarce resource so you need to know
> > its usage at compile time.
> > 
> > In this case you actually know the allocation because the value is not
> > changed during the course of the function but it is still bad. Probably
> > compiler will optimize it out. Still it is not a good practice.
> > 
> > > Also, to understand, this is related to multi-bank patchset. I mean how does
> > > it affect for event log patchset ?
> > 
> > Well in both cases these have landed fairly late but I asked from James
> > whether I'll have to postpone these to 4.12.
> > 
> > Usually when I've sent my release pull request I do not want to make any
> > radical changes to the codebase because they always require extra QA and
> > thus take extra time.
> 
> rc = tpm_transmit_cmd(chip, buf.data, tpm_buf_length(&buf), 0, 0,
> 		      "attempting extend a PCR value");
> 
> This should be
> 
> rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, 0, 0,
> 		      "attempting extend a PCR value");
> 
> The second parameter is the size of the buffer, not length of the input
> data.
> 
> /Jarkko

As a sanity check can you test these commits and see if they still
work for you as I've done now some updates to them? Thanks.

/Jarkko

Re: Fwd: Re: [tpmdd-devel] [PATCH v9 2/2] tpm: add securityfs support,for TPM 2.0 firmware event log

[Jarkko Sakkinen]

On Wed, Feb 01, 2017 at 08:49:30AM -0500, Mimi Zohar wrote:
> On Wed, 2017-02-01 at 12:30 +0200, Jarkko Sakkinen wrote:
> > On Tue, Jan 31, 2017 at 05:31:50PM -0500, Mimi Zohar wrote:
> > > On Wed, 2017-02-01 at 08:46 +1100, James Morris wrote:
> > > > On Tue, 31 Jan 2017, Jarkko Sakkinen wrote:
> > > > 
> > > > > James,
> > > > > 
> > > > > The discussion is about two features:
> > > > > 
> > > > > 1. Extension to tpm_pcr_extend() (used by IMA) to extend all PCR banks
> > > > >    instead of just SHA-1 banks. It is recommended by TCG to do so in
> > > > >    order to prevent malicious use of PCRs.
> > > > > 2. TPM 2.0 event log with backend support for OF device tree (for
> > > > >    getting address where you can grab it).
> > > > > 
> > > > > These are required as baseline to implement full TPM 2.0 support for
> > > > > IMA. The commits are fairly well baked and went through many iterations.
> > > > > I've tested tpm_pcr_extend() patches. I haven't tested event log patches
> > > > > but have extensively reviewed them and Ken Goldman has tested them with
> > > > > POWER hardware.
> > > > > 
> > > > > I don't believe that there is major risk to put them already into 4.11
> > > > > but it is fairly late so I just want a second opinion before putting
> > > > > them into pull request.
> > > > > 
> > > > 
> > > > I'll take this for 4.11.  IMA + TPM 2.0 is still developmental and not in 
> > > > wide use, afaik.
> > > 
> > > Thank you!   FYI, I've been running with them as well.
> > > 
> > > Mimi
> > 
> > OK, so can I also add your Tested-by to these four commits?
> 
> I provided Ken with the firmware event log, IMA measurement list, and
> the PCRs for testing.   Ken's tested-by is fine.  Missing is Stefan's
> tested-by - https://sourceforge.net/p/tpmdd/mailman/message/35602588/
> I would really appreciate your adding his tested-by.
> 
> thanks,
> 
> Mimi

Sure, I'll add it.

/Jarkko

Re: Fwd: Re: [tpmdd-devel] [PATCH v9 2/2] tpm: add securityfs support,for TPM 2.0 firmware event log

[Jarkko Sakkinen]

On Wed, Feb 01, 2017 at 01:01:06PM +0530, Nayna wrote:
> 
> 
> On 02/01/2017 02:20 AM, Jarkko Sakkinen wrote:
> > On Wed, Feb 01, 2017 at 12:14:12AM +0530, Nayna wrote:
> > > > I already sent my pull request to 4.11 and even today I found something
> > > > fishy. You declared a function local array by using a variable in "tpm:
> > > > enhance TPM 2.0 PCR extend to support multiple banks" (max_active_banks
> > > > or something). And the event log patches have just passed the review.
> > > 
> > > Yes. I have checked using clang and it has passed the clang.. and I also
> > > verified there were no complains during build.
> > 
> > What we can deduce from that is that they didn't expose the issue in
> > question.
> > 
> > I found this by running sparse with make C=2 M=drives/char/tpm
> > 
> > > What type of problem do you see ?
> > 
> > It is disallowed to do stack allocation in the kernel code even if C
> > standard would allow it. Stack is scarce resource so you need to know
> > its usage at compile time.
> > 
> > In this case you actually know the allocation because the value is not
> > changed during the course of the function but it is still bad. Probably
> > compiler will optimize it out. Still it is not a good practice.
> 
> Thanks Jarkko for explaining it.
> 
> Hmm, do you want me to send a patch for this ?
> I think what we want is actually define it just array of size as 7.

No but please test my tree and check that it still works.

/Jarkko

Re: Fwd: Re: [tpmdd-devel] [PATCH v9 2/2] tpm: add securityfs support,for TPM 2.0 firmware event log

[Nayna]

On 02/01/2017 08:24 PM, Jarkko Sakkinen wrote:
> On Wed, Feb 01, 2017 at 04:48:37PM +0200, Jarkko Sakkinen wrote:
>> On Tue, Jan 31, 2017 at 10:50:06PM +0200, Jarkko Sakkinen wrote:
>>> On Wed, Feb 01, 2017 at 12:14:12AM +0530, Nayna wrote:
>>>>> I already sent my pull request to 4.11 and even today I found something
>>>>> fishy. You declared a function local array by using a variable in "tpm:
>>>>> enhance TPM 2.0 PCR extend to support multiple banks" (max_active_banks
>>>>> or something). And the event log patches have just passed the review.
>>>>
>>>> Yes. I have checked using clang and it has passed the clang.. and I also
>>>> verified there were no complains during build.
>>>
>>> What we can deduce from that is that they didn't expose the issue in
>>> question.
>>>
>>> I found this by running sparse with make C=2 M=drives/char/tpm
>>>
>>>> What type of problem do you see ?
>>>
>>> It is disallowed to do stack allocation in the kernel code even if C
>>> standard would allow it. Stack is scarce resource so you need to know
>>> its usage at compile time.
>>>
>>> In this case you actually know the allocation because the value is not
>>> changed during the course of the function but it is still bad. Probably
>>> compiler will optimize it out. Still it is not a good practice.
>>>
>>>> Also, to understand, this is related to multi-bank patchset. I mean how does
>>>> it affect for event log patchset ?
>>>
>>> Well in both cases these have landed fairly late but I asked from James
>>> whether I'll have to postpone these to 4.12.
>>>
>>> Usually when I've sent my release pull request I do not want to make any
>>> radical changes to the codebase because they always require extra QA and
>>> thus take extra time.
>>
>> rc = tpm_transmit_cmd(chip, buf.data, tpm_buf_length(&buf), 0, 0,
>> 		      "attempting extend a PCR value");
>>
>> This should be
>>
>> rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, 0, 0,
>> 		      "attempting extend a PCR value");
>>
>> The second parameter is the size of the buffer, not length of the input
>> data.
>>
>> /Jarkko
>
> As a sanity check can you test these commits and see if they still
> work for you as I've done now some updates to them? Thanks.

Thanks Jarkko, yes I tested for both multi-bank patches and event log.
Its working fine.

Thanks & Regards,
    - Nayna

>
> /Jarkko
>

Re: Fwd: Re: [tpmdd-devel] [PATCH v9 2/2] tpm: add securityfs support,for TPM 2.0 firmware event log

[Jarkko Sakkinen]

On Thu, Feb 02, 2017 at 12:55:41AM +0530, Nayna wrote:
> 
> 
> On 02/01/2017 08:24 PM, Jarkko Sakkinen wrote:
> > On Wed, Feb 01, 2017 at 04:48:37PM +0200, Jarkko Sakkinen wrote:
> > > On Tue, Jan 31, 2017 at 10:50:06PM +0200, Jarkko Sakkinen wrote:
> > > > On Wed, Feb 01, 2017 at 12:14:12AM +0530, Nayna wrote:
> > > > > > I already sent my pull request to 4.11 and even today I found something
> > > > > > fishy. You declared a function local array by using a variable in "tpm:
> > > > > > enhance TPM 2.0 PCR extend to support multiple banks" (max_active_banks
> > > > > > or something). And the event log patches have just passed the review.
> > > > > 
> > > > > Yes. I have checked using clang and it has passed the clang.. and I also
> > > > > verified there were no complains during build.
> > > > 
> > > > What we can deduce from that is that they didn't expose the issue in
> > > > question.
> > > > 
> > > > I found this by running sparse with make C=2 M=drives/char/tpm
> > > > 
> > > > > What type of problem do you see ?
> > > > 
> > > > It is disallowed to do stack allocation in the kernel code even if C
> > > > standard would allow it. Stack is scarce resource so you need to know
> > > > its usage at compile time.
> > > > 
> > > > In this case you actually know the allocation because the value is not
> > > > changed during the course of the function but it is still bad. Probably
> > > > compiler will optimize it out. Still it is not a good practice.
> > > > 
> > > > > Also, to understand, this is related to multi-bank patchset. I mean how does
> > > > > it affect for event log patchset ?
> > > > 
> > > > Well in both cases these have landed fairly late but I asked from James
> > > > whether I'll have to postpone these to 4.12.
> > > > 
> > > > Usually when I've sent my release pull request I do not want to make any
> > > > radical changes to the codebase because they always require extra QA and
> > > > thus take extra time.
> > > 
> > > rc = tpm_transmit_cmd(chip, buf.data, tpm_buf_length(&buf), 0, 0,
> > > 		      "attempting extend a PCR value");
> > > 
> > > This should be
> > > 
> > > rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, 0, 0,
> > > 		      "attempting extend a PCR value");
> > > 
> > > The second parameter is the size of the buffer, not length of the input
> > > data.
> > > 
> > > /Jarkko
> > 
> > As a sanity check can you test these commits and see if they still
> > work for you as I've done now some updates to them? Thanks.
> 
> Thanks Jarkko, yes I tested for both multi-bank patches and event log.
> Its working fine.
> 
> Thanks & Regards,
>    - Nayna

OK, good, thanks.

/Jarkko