Re: [PATCH v2 06/11] tpm: Add full HMAC and encrypt/decrypt session handling code

From: James Bottomley <James.Bottomley@HansenPartnership.com>
To: Jarkko Sakkinen <jarkko@kernel.org>, Yujie Liu <yujie.liu@intel.com>
Cc: kernel test robot <lkp@intel.com>,
	linux-integrity@vger.kernel.org,  oe-kbuild-all@lists.linux.dev,
	keyrings@vger.kernel.org, Ard Biesheuvel <ardb@kernel.org>
Subject: Re: [PATCH v2 06/11] tpm: Add full HMAC and encrypt/decrypt session handling code
Date: Fri, 10 Feb 2023 09:48:15 -0500	[thread overview]
Message-ID: <3109ff421139af6b0d9e66a06d8399135e546fa7.camel@HansenPartnership.com> (raw)
In-Reply-To: <Y+MNxmzlILarAlZA@kernel.org>

On Wed, 2023-02-08 at 04:49 +0200, Jarkko Sakkinen wrote:
> On Fri, Feb 03, 2023 at 02:06:48PM +0800, Yujie Liu wrote:
> > Hi James,
> > 
> > On Wed, Jan 25, 2023 at 07:59:09AM -0500, James Bottomley wrote:
> > > On Wed, 2023-01-25 at 07:11 +0800, kernel test robot wrote:
> > > > Hi James,
> > > > 
> > > > I love your patch! Perhaps something to improve:
> > > > 
> > > > [auto build test WARNING on char-misc/char-misc-testing]
> > > > [also build test WARNING on char-misc/char-misc-next char-
> > > > misc/char-
> > > > misc-linus zohar-integrity/next-integrity linus/master v6.2-rc5
> > > > next-
> > > > 20230124]
> > > > [If your patch is applied to the wrong git tree, kindly drop us
> > > > a
> > > > note.
> > > > And when submitting patch, we suggest to use '--base' as
> > > > documented
> > > > in
> > > > https://git-scm.com/docs/git-format-patch#_base_tree_information
> > > > ]
> > > > 
> > > > url:   
> > > > https://github.com/intel-lab-lkp/linux/commits/James-Bottomley/tpm-move-buffer-handling-from-static-inlines-to-real-functions/20230125-020146
> > > > patch link:   
> > > > https://lore.kernel.org/r/20230124175516.5984-7-James.Bottomley%40HansenPartnership.com
> > > > patch subject: [PATCH v2 06/11] tpm: Add full HMAC and
> > > > encrypt/decrypt session handling code
> > > > config: arc-allyesconfig
> > > > (
> > > > https://download.01.org/0day-ci/archive/20230125/202301250706.de
> > > > Gvd0
> > > > yq-lkp@intel.com/config)
> > > > compiler: arceb-elf-gcc (GCC) 12.1.0
> > > > reproduce (this is a W=1 build):
> > > >         wget
> > > > https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross
> > > >  -O ~/bin/make.cross
> > > >         chmod +x ~/bin/make.cross
> > > >         #
> > > > https://github.com/intel-lab-lkp/linux/commit/dc0fc74718b4a786aba4a954233e8ab3afdcc03c
> > > >         git remote add linux-review
> > > > https://github.com/intel-lab-lkp/linux
> > > >         git fetch --no-tags linux-review James-Bottomley/tpm-
> > > > move-
> > > > buffer-handling-from-static-inlines-to-real-functions/20230125-
> > > > 020146
> > > >         git checkout dc0fc74718b4a786aba4a954233e8ab3afdcc03c
> > > >         # save the config file
> > > >         mkdir build_dir && cp config build_dir/.config
> > > >         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0
> > > > make.cross W=1 O=build_dir ARCH=arc olddefconfig
> > > >         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0
> > > > make.cross W=1 O=build_dir ARCH=arc SHELL=/bin/bash
> > > > drivers/char/tpm/
> > > > 
> > > > If you fix the issue, kindly add following tag where applicable
> > > > > Reported-by: kernel test robot <lkp@intel.com>
> > > > 
> > > > All warnings (new ones prefixed by >>):
> > > > 
> > > >    drivers/char/tpm/tpm2-sessions.c:1184:5: warning: no
> > > > previous
> > > > prototype for 'tpm2_create_null_primary' [-Wmissing-prototypes]
> > > >     1184 | int tpm2_create_null_primary(struct tpm_chip *chip)
> > > > {
> > > >          |     ^~~~~~~~~~~~~~~~~~~~~~~~
> > > >    drivers/char/tpm/tpm2-sessions.c: In function
> > > > 'tpm_buf_check_hmac_response':
> > > > > > drivers/char/tpm/tpm2-sessions.c:831:1: warning: the frame
> > > > > > size
> > > > > > of 1132 bytes is larger than 1024 bytes [-Wframe-larger-
> > > > > > than=]
> > > >      831 | }
> > > >          | ^
> > > >    drivers/char/tpm/tpm2-sessions.c: In function
> > > > 'tpm_buf_fill_hmac_session':
> > > >    drivers/char/tpm/tpm2-sessions.c:579:1: warning: the frame
> > > > size of
> > > > 1132 bytes is larger than 1024 bytes [-Wframe-larger-than=]
> > > >      579 | }
> > > >          | ^
> > > 
> > > Is this a test problem?  I can't see why the code would only blow
> > > the
> > > stack on the arc architecture and not on any other ... does it
> > > have
> > > something funny with on stack crypto structures?
> > 
> > This warning is controlled by the value of CONFIG_FRAME_WARN.
> > 
> > For "make ARCH=arc allyesconfig", the default value is 1024, so
> > this frame warning shows up during the build.
> > 
> > For other arch such as "make ARCH=x86_64 allyesconfig", the default
> > value would be 2048 and won't have this warning.
> > 
> > Not sure if this is a real problem that need to be fixed, here just
> > providing above information for your reference.
> > 
> > --
> > Best Regards,
> > Yujie
> 
> *Must* be fixed given that it is how the default value is set now.
> This is wrong place to reconsider.
>
> 
> And we do not want to add functions that bloat the stack this way.
> 
> Shash just needs to be allocated from heap instead of stack.

On x86_64 the stack usage is measured at 984 bytes, so rather than
jumping to conclusions let's root cause why this is a problem only on
the arc architecture.  I suspect it's something to do with the
alignment constraints of shash.  I've also noted it shouldn't actually
warn on arc because the default stack warning size there should be 2048
(like x86_64).

James