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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170131205006.fljtxsy4s6lyhkvv@intel.com>
Date:   Tue, 31 Jan 2017 22:50:06 +0200
From:   Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>
To:     Nayna <nayna@...ux.vnet.ibm.com>
Cc:     Kenneth Goldman <kgoldman@...ibm.com>,
        "moderated list:TPM DEVICE DRIVER" 
        <tpmdd-devel@...ts.sourceforge.net>,
        open list <linux-kernel@...r.kernel.org>,
        linux-security-module@...r.kernel.org
Subject: Re: Fwd: Re: [tpmdd-devel] [PATCH v9 2/2] tpm: add securityfs
 support,for TPM 2.0 firmware event log

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