From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751433AbdAaUuT (ORCPT ); Tue, 31 Jan 2017 15:50:19 -0500 Received: from mga05.intel.com ([192.55.52.43]:3523 "EHLO mga05.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750863AbdAaUuK (ORCPT ); Tue, 31 Jan 2017 15:50:10 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,316,1477983600"; d="scan'208";a="59675615" Date: Tue, 31 Jan 2017 22:50:06 +0200 From: Jarkko Sakkinen To: Nayna Cc: Kenneth Goldman , "moderated list:TPM DEVICE DRIVER" , open list , linux-security-module@vger.kernel.org Subject: Re: Fwd: Re: [tpmdd-devel] [PATCH v9 2/2] tpm: add securityfs support,for TPM 2.0 firmware event log Message-ID: <20170131205006.fljtxsy4s6lyhkvv@intel.com> References: <588F09A2.4090502@linux.vnet.ibm.com> <20170131174659.b6njebycqzd5ur6f@intel.com> <5890DAFC.9030407@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5890DAFC.9030407@linux.vnet.ibm.com> Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo User-Agent: Mutt/1.6.2-neo (2016-08-21) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jarkko Sakkinen Subject: Re: Fwd: Re: [tpmdd-devel] [PATCH v9 2/2] tpm: add securityfs support,for TPM 2.0 firmware event log Date: Tue, 31 Jan 2017 22:50:06 +0200 Message-ID: <20170131205006.fljtxsy4s6lyhkvv@intel.com> References: <588F09A2.4090502@linux.vnet.ibm.com> <20170131174659.b6njebycqzd5ur6f@intel.com> <5890DAFC.9030407@linux.vnet.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <5890DAFC.9030407@linux.vnet.ibm.com> Sender: owner-linux-security-module@vger.kernel.org To: Nayna Cc: Kenneth Goldman , "moderated list:TPM DEVICE DRIVER" , open list , linux-security-module@vger.kernel.org List-Id: tpmdd-devel@lists.sourceforge.net 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