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

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

[kernel test robot]

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: sparc-allyesconfig (https://download.01.org/0day-ci/archive/20230125/202301250432.kZ4b6794-lkp@intel.com/config)
compiler: sparc64-linux-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=sparc olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sparc 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:352: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
    * tpm_buf_append_hmac_session() append a TPM session element


vim +/tpm2_create_null_primary +1184 drivers/char/tpm/tpm2-sessions.c

  1183	
> 1184	int tpm2_create_null_primary(struct tpm_chip *chip) {
  1185		u32 nullkey;
  1186		int rc;
  1187	
  1188		rc = tpm2_create_primary(chip, TPM2_RH_NULL, &nullkey);
  1189	
  1190		if (rc == TPM2_RC_SUCCESS) {
  1191			unsigned int offset = 0; /* dummy offset for tpmkeycontext */
  1192	
  1193			rc = tpm2_save_context(chip, nullkey, chip->tpmkeycontext,
  1194					       sizeof(chip->tpmkeycontext), &offset);
  1195			tpm2_flush_context(chip, nullkey);
  1196		}
  1197	
  1198		return rc;
  1199	}
  1200	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

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

[kernel test robot]

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.deGvd0yq-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 | }
         | ^


vim +831 drivers/char/tpm/tpm2-sessions.c

   676	
   677	/**
   678	 * tpm_buf_check_hmac_response() - check the TPM return HMAC for correctness
   679	 * @buf: the original command buffer (which now contains the response)
   680	 * @auth: the auth structure allocated by tpm2_start_auth_session()
   681	 * @rc: the return code from tpm_transmit_cmd
   682	 *
   683	 * If @rc is non zero, @buf may not contain an actual return, so @rc
   684	 * is passed through as the return and the session cleaned up and
   685	 * de-allocated if required (this is required if
   686	 * TPM2_SA_CONTINUE_SESSION was not specified as a session flag).
   687	 *
   688	 * If @rc is zero, the response HMAC is computed against the returned
   689	 * @buf and matched to the TPM one in the session area.  If there is a
   690	 * mismatch, an error is logged and -EINVAL returned.
   691	 *
   692	 * The reason for this is that the command issue and HMAC check
   693	 * sequence should look like:
   694	 *
   695	 *	rc = tpm_transmit_cmd(...);
   696	 *	rc = tpm_buf_check_hmac_response(&buf, auth, rc);
   697	 *	if (rc)
   698	 *		...
   699	 *
   700	 * Which is easily layered into the current contrl flow.
   701	 *
   702	 * Returns: 0 on success or an error.
   703	 */
   704	int tpm_buf_check_hmac_response(struct tpm_buf *buf, struct tpm2_auth *auth,
   705					int rc)
   706	{
   707		struct tpm_header *head = (struct tpm_header *)buf->data;
   708		struct tpm_chip *chip = auth->chip;
   709		const u8 *s, *p;
   710		u8 rphash[SHA256_DIGEST_SIZE];
   711		u32 attrs;
   712		SHASH_DESC_ON_STACK(desc, sha256_hash);
   713		u16 tag = be16_to_cpu(head->tag);
   714		u32 cc = be32_to_cpu(auth->ordinal);
   715		int parm_len, len, i, handles;
   716	
   717		if (auth->session >= TPM_HEADER_SIZE) {
   718			WARN(1, "tpm session not filled correctly\n");
   719			goto out;
   720		}
   721	
   722		if (rc != 0)
   723			/* pass non success rc through and close the session */
   724			goto out;
   725	
   726		rc = -EINVAL;
   727		if (tag != TPM2_ST_SESSIONS) {
   728			dev_err(&chip->dev, "TPM: HMAC response check has no sessions tag\n");
   729			goto out;
   730		}
   731	
   732		i = tpm2_find_cc(chip, cc);
   733		if (i < 0)
   734			goto out;
   735		attrs = chip->cc_attrs_tbl[i];
   736		handles = (attrs >> TPM2_CC_ATTR_RHANDLE) & 1;
   737	
   738		/* point to area beyond handles */
   739		s = &buf->data[TPM_HEADER_SIZE + handles * 4];
   740		parm_len = tpm_get_inc_u32(&s);
   741		p = s;
   742		s += parm_len;
   743		/* skip over any sessions before ours */
   744		for (i = 0; i < auth->session - 1; i++) {
   745			len = tpm_get_inc_u16(&s);
   746			s += len + 1;
   747			len = tpm_get_inc_u16(&s);
   748			s += len;
   749		}
   750		/* TPM nonce */
   751		len = tpm_get_inc_u16(&s);
   752		if (s - buf->data + len > tpm_buf_length(buf))
   753			goto out;
   754		if (len != SHA256_DIGEST_SIZE)
   755			goto out;
   756		memcpy(auth->tpm_nonce, s, len);
   757		s += len;
   758		attrs = *s++;
   759		len = tpm_get_inc_u16(&s);
   760		if (s - buf->data + len != tpm_buf_length(buf))
   761			goto out;
   762		if (len != SHA256_DIGEST_SIZE)
   763			goto out;
   764		/*
   765		 * s points to the HMAC. now calculate comparison, beginning
   766		 * with rphash
   767		 */
   768		desc->tfm = sha256_hash;
   769		crypto_shash_init(desc);
   770		/* yes, I know this is now zero, but it's what the standard says */
   771		crypto_shash_update(desc, (u8 *)&head->return_code,
   772				    sizeof(head->return_code));
   773		/* ordinal is already BE */
   774		crypto_shash_update(desc, (u8 *)&auth->ordinal, sizeof(auth->ordinal));
   775		crypto_shash_update(desc, p, parm_len);
   776		crypto_shash_final(desc, rphash);
   777	
   778		/* now calculate the hmac */
   779		hmac_init(desc, auth->session_key, sizeof(auth->session_key)
   780			  + auth->passphraselen);
   781		crypto_shash_update(desc, rphash, sizeof(rphash));
   782		crypto_shash_update(desc, auth->tpm_nonce, sizeof(auth->tpm_nonce));
   783		crypto_shash_update(desc, auth->our_nonce, sizeof(auth->our_nonce));
   784		crypto_shash_update(desc, &auth->attrs, 1);
   785		/* we're done with the rphash, so put our idea of the hmac there */
   786		hmac_final(desc, auth->session_key, sizeof(auth->session_key)
   787			   + auth->passphraselen, rphash);
   788		if (memcmp(rphash, s, SHA256_DIGEST_SIZE) == 0) {
   789			rc = 0;
   790		} else {
   791			dev_err(&auth->chip->dev, "TPM: HMAC check failed\n");
   792			goto out;
   793		}
   794	
   795		/* now do response decryption */
   796		if (auth->attrs & TPM2_SA_ENCRYPT) {
   797			struct scatterlist sg[1];
   798			SYNC_SKCIPHER_REQUEST_ON_STACK(req, auth->aes);
   799			DECLARE_CRYPTO_WAIT(wait);
   800	
   801			skcipher_request_set_sync_tfm(req, auth->aes);
   802			skcipher_request_set_callback(req, CRYPTO_TFM_REQ_MAY_SLEEP,
   803						      crypto_req_done, &wait);
   804	
   805			/* need key and IV */
   806			KDFa(auth->session_key, SHA256_DIGEST_SIZE
   807			     + auth->passphraselen, "CFB", auth->tpm_nonce,
   808			     auth->our_nonce, AES_KEYBYTES + AES_BLOCK_SIZE,
   809			     auth->scratch);
   810			crypto_sync_skcipher_setkey(auth->aes, auth->scratch, AES_KEYBYTES);
   811			len = tpm_get_inc_u16(&p);
   812			sg_init_one(sg, p, len);
   813			skcipher_request_set_crypt(req, sg, sg, len,
   814						   auth->scratch + AES_KEYBYTES);
   815			crypto_wait_req(crypto_skcipher_decrypt(req), &wait);
   816		}
   817	
   818	 out:
   819		if ((auth->attrs & TPM2_SA_CONTINUE_SESSION) == 0) {
   820			/* manually close the session if it wasn't consumed */
   821			if (rc)
   822				tpm2_flush_context(chip, auth->handle);
   823			crypto_free_sync_skcipher(auth->aes);
   824			kfree(auth);
   825		} else {
   826			/* reset for next use  */
   827			auth->session = TPM_HEADER_SIZE;
   828		}
   829	
   830		return rc;
 > 831	}
   832	EXPORT_SYMBOL(tpm_buf_check_hmac_response);
   833	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

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

[kernel test robot]

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: i386-randconfig-s002-20230123 (https://download.01.org/0day-ci/archive/20230125/202301251326.r0t0nGZc-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.4-39-gce1a6720-dirty
        # 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
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=i386 olddefconfig
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=i386 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

sparse warnings: (new ones prefixed by >>)
>> drivers/char/tpm/tpm2-sessions.c:1184:5: sparse: sparse: symbol 'tpm2_create_null_primary' was not declared. Should it be static?

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

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

[James Bottomley]

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.deGvd0
> 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?

James

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

[Yujie Liu]

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.deGvd0
> > 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

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

[Jarkko Sakkinen]

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.deGvd0
> > > 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.

BR, Jarkko

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

[James Bottomley]

On Fri, 2023-02-03 at 14:06 +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.deGv
> > > d0
> > > 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.

Is there something weird about arc, though, since a quick trawl through
defconfigs shows quite a few systems (including i386) have it set to
1024:

mips/configs/lemote2f_defconfig:CONFIG_FRAME_WARN=1024
mips/configs/loongson2k_defconfig:CONFIG_FRAME_WARN=1024
powerpc/configs/fsl-emb-nonhw.config:CONFIG_FRAME_WARN=1024
um/configs/x86_64_defconfig:CONFIG_FRAME_WARN=1024
x86/configs/i386_defconfig:CONFIG_FRAME_WARN=1024

That also seems to show, by the way, that the default on arc should be
the standard 2048.

James

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

[James Bottomley]

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

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

[Jarkko Sakkinen]

On Fri, Feb 10, 2023 at 09:48:15AM -0500, James Bottomley wrote:
> 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).

Would it such a big deal to allocate shash from heap? That would
be IMHO more robust in the end.

BR, Jarkko

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

[Yujie Liu]

On Mon, Feb 13, 2023 at 09:45:40AM +0200, Jarkko Sakkinen wrote:
> On Fri, Feb 10, 2023 at 09:48:15AM -0500, James Bottomley wrote:
> > 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:
> > > > > > 
> > > > > > 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).

The stack usage varies on different architectures, so does the default
value of CONFIG_FRAME_WARN. The warning shows up when the stack usage
exceeds the default warning size.

For arc arch, I reconfirmed that the default stack warning size is 1024
no matter allyesconfig or defconfig, while the usage could reach 1132
bytes.

$ make ARCH=arc allyesconfig
$ grep FRAME_WARN .config
CONFIG_FRAME_WARN=1024

$ make ARCH=arc defconfig
$ grep FRAME_WARN .config
CONFIG_FRAME_WARN=1024

> Would it such a big deal to allocate shash from heap? That would
> be IMHO more robust in the end.

Thanks Jarkko for the suggestion. This would be a faster and better fix
without having to look into this arc-specific problem.

Best Regards,
Yujie
 
> BR, Jarkko

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

[Ard Biesheuvel]

On Mon, 13 Feb 2023 at 08:45, Jarkko Sakkinen <jarkko@kernel.org> wrote:
>
> On Fri, Feb 10, 2023 at 09:48:15AM -0500, James Bottomley wrote:
> > 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).
>
> Would it such a big deal to allocate shash from heap? That would
> be IMHO more robust in the end.
>

Can we avoid shashes and sync skciphers at all? We have sha256 and AES
library routines these days, and AES in CFB mode seems like a good
candidate for a library implementation as well - it uses AES
encryption only, and is quite straight forward to implement. [0]

The crypto API is far too clunky for synchronous operations of
algorithms that are known at compile time, and the requirement to use
scatterlists for skciphers is especially horrid.

[0] https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/?h=crypto-aes-cfb-library

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

[James Bottomley]

On Tue, 2023-02-14 at 14:54 +0100, Ard Biesheuvel wrote:
> On Mon, 13 Feb 2023 at 08:45, Jarkko Sakkinen <jarkko@kernel.org>
> wrote:
> > 
> > On Fri, Feb 10, 2023 at 09:48:15AM -0500, James Bottomley wrote:
> > > 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).
> > 
> > Would it such a big deal to allocate shash from heap? That would
> > be IMHO more robust in the end.

Heap allocation is time indeterminate and eventually Mimi is going to
want me to make this go faster.

> 
> Can we avoid shashes and sync skciphers at all? We have sha256 and
> AES library routines these days, and AES in CFB mode seems like a
> good candidate for a library implementation as well - it uses AES
> encryption only, and is quite straight forward to implement. [0]

Yes, sure.  I originally suggested something like this way back four
years ago, but it got overruled on the grounds that if I didn't use
shashes and skciphers some architectures would be unable to use crypto
acceleration.  If that's no longer a consideration, I'm all for
simplification of static cipher types.

> The crypto API is far too clunky for synchronous operations of
> algorithms that are known at compile time, and the requirement to use
> scatterlists for skciphers is especially horrid.
> 
> [0]
> https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/?h=crypto-aes-cfb-library

OK, let me have a go at respinning based on this.

Regards,

James

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

[James Bottomley]

On Tue, 2023-02-14 at 14:54 +0100, Ard Biesheuvel wrote:
[...]
> and the requirement to use
> scatterlists for skciphers is especially horrid.

Just by way of irony, we do have one place that does use a two element
scatterlist.  It's how I found this bug: 95ec01ba1ef0 ("crypto: ecdh -
fix to allow multi segment scatterlists").  And that does mean I could
do with a library version of ecdh with the nist P-256 curve ... pretty
please ...

Regards,

James

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

[Ard Biesheuvel]

On Tue, 14 Feb 2023 at 15:28, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
>
> On Tue, 2023-02-14 at 14:54 +0100, Ard Biesheuvel wrote:
> > On Mon, 13 Feb 2023 at 08:45, Jarkko Sakkinen <jarkko@kernel.org>
> > wrote:
> > >
> > > On Fri, Feb 10, 2023 at 09:48:15AM -0500, James Bottomley wrote:
> > > > 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).
> > >
> > > Would it such a big deal to allocate shash from heap? That would
> > > be IMHO more robust in the end.
>
> Heap allocation is time indeterminate and eventually Mimi is going to
> want me to make this go faster.
>
> >
> > Can we avoid shashes and sync skciphers at all? We have sha256 and
> > AES library routines these days, and AES in CFB mode seems like a
> > good candidate for a library implementation as well - it uses AES
> > encryption only, and is quite straight forward to implement. [0]
>
> Yes, sure.  I originally suggested something like this way back four
> years ago, but it got overruled on the grounds that if I didn't use
> shashes and skciphers some architectures would be unable to use crypto
> acceleration.  If that's no longer a consideration, I'm all for
> simplification of static cipher types.
>

I don't know if that is a consideration or not. The AES library code
is generic C code that was written to be constant-time, rather than
fast. The fact that CFB only uses the encryption side of it is
fortunate, because decryption is even slower.

So the question is whether this will actually be a bottleneck in this
particular scenario. The synchronous accelerated AES implementations
are all SIMD based, which means there is some overhead, and some
degree of parallelism is also needed to take full advantage, and CFB
only allows this for decryption to begin with, as encryption uses
ciphertext block N-1 as AES input for encrypting block N.

So maybe this is terrible advice, but the code will look so much
better for it, and we can always add back the performance later if it
is really an impediment.


> > The crypto API is far too clunky for synchronous operations of
> > algorithms that are known at compile time, and the requirement to use
> > scatterlists for skciphers is especially horrid.
> >
> > [0]
> > https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/?h=crypto-aes-cfb-library
>
> OK, let me have a go at respinning based on this.
>
> Regards,
>
> James
>

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

[James Bottomley]

On Tue, 2023-02-14 at 15:36 +0100, Ard Biesheuvel wrote:
> On Tue, 14 Feb 2023 at 15:28, James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
> > On Tue, 2023-02-14 at 14:54 +0100, Ard Biesheuvel wrote:
[...]
> > > 
> > > Can we avoid shashes and sync skciphers at all? We have sha256
> > > and AES library routines these days, and AES in CFB mode seems
> > > like a good candidate for a library implementation as well - it
> > > uses AES encryption only, and is quite straight forward to
> > > implement. [0]
> > 
> > Yes, sure.  I originally suggested something like this way back
> > four years ago, but it got overruled on the grounds that if I
> > didn't use shashes and skciphers some architectures would be unable
> > to use crypto acceleration.  If that's no longer a consideration,
> > I'm all for simplification of static cipher types.
> > 

I now have this all implemented, and I looked over your code, so you
can add my tested/reviewed-by to the aescfb implementation.  On the
acceleration issue, I'm happy to ignore external accelerators because
they're a huge pain for small fragments of encryption like the TPM, but
it would be nice if we could integrate CPU instruction acceleration
(like AES-NI on x86) into the library functions. 

I also got a test rig to investigate arc.  It seems there is a huge
problem with the SKCIPHER stack structure on that platform.  For
reasons I still can't fathom, the compiler thinks it needs at least
0.5k of stack for this one structure.  I'm sure its something to do
with an incorrect crypto alignment on arc, but I can't yet find the
root cause.

> I don't know if that is a consideration or not. The AES library code
> is generic C code that was written to be constant-time, rather than
> fast. The fact that CFB only uses the encryption side of it is
> fortunate, because decryption is even slower.

I think for the TPM, since the encryption isn't exactly bulk (it's
really under 1k for command and response encryption) it doesn't matter
... in fact setting up the accelerator is likely a bigger overhead.

> So the question is whether this will actually be a bottleneck in this
> particular scenario. The synchronous accelerated AES implementations
> are all SIMD based, which means there is some overhead, and some
> degree of parallelism is also needed to take full advantage, and CFB
> only allows this for decryption to begin with, as encryption uses
> ciphertext block N-1 as AES input for encrypting block N.
> 
> So maybe this is terrible advice, but the code will look so much
> better for it, and we can always add back the performance later if it
> is really an impediment.

It's definitely smaller and neater, yes.  I'll post a v3 based on this,
but when might it go upstream?  In my post I'll put your aescfb as
patch 1 so the static checkers don't go haywire about missing function
exports, and we can drop that patch when it is upstream.

James

> 
> 
> > > The crypto API is far too clunky for synchronous operations of
> > > algorithms that are known at compile time, and the requirement to
> > > use scatterlists for skciphers is especially horrid.
> > > 
> > > [0]
> > > https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/?h=crypto-aes-cfb-library
> > 
> > OK, let me have a go at respinning based on this.

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

[Ard Biesheuvel]

On Thu, 16 Feb 2023 at 15:52, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
>
> On Tue, 2023-02-14 at 15:36 +0100, Ard Biesheuvel wrote:
> > On Tue, 14 Feb 2023 at 15:28, James Bottomley
> > <James.Bottomley@hansenpartnership.com> wrote:
> > > On Tue, 2023-02-14 at 14:54 +0100, Ard Biesheuvel wrote:
> [...]
> > > >
> > > > Can we avoid shashes and sync skciphers at all? We have sha256
> > > > and AES library routines these days, and AES in CFB mode seems
> > > > like a good candidate for a library implementation as well - it
> > > > uses AES encryption only, and is quite straight forward to
> > > > implement. [0]
> > >
> > > Yes, sure.  I originally suggested something like this way back
> > > four years ago, but it got overruled on the grounds that if I
> > > didn't use shashes and skciphers some architectures would be unable
> > > to use crypto acceleration.  If that's no longer a consideration,
> > > I'm all for simplification of static cipher types.
> > >
>
> I now have this all implemented, and I looked over your code, so you
> can add my tested/reviewed-by to the aescfb implementation.  On the
> acceleration issue, I'm happy to ignore external accelerators because
> they're a huge pain for small fragments of encryption like the TPM, but
> it would be nice if we could integrate CPU instruction acceleration
> (like AES-NI on x86) into the library functions.
>

Agreed that async crypto makes no sense here, and it is rather
unfortunate that even use cases such as this one require the
scatterlist handling, which requires direct mapped memory etc etc

As for the accelerated algos: it wouldn't be too complicated to build
the CFB library interface around the 'AES' crypto cipher, which is
synchronous and operates on virtual addresses directly. But it should
only use ones that are constant time (like AES-NI) and not use generic
AES or the asm accelerated ones, and so this would require an
additional annotation (or an allowlist) which makes things a bit
clunky.

However, doing the math on the back of an envelope: taking arm64 as an
example, which manages ~1 cycle per byte for AES instructions and 25
cycles per byte for AES encryption using this library, processing 1k
of data takes an additional 24k cycles, which comes down to 10
microseconds on a 2.4 GHz CPU.

Given that this particular use case is about communicating with off
chip discrete components, I wonder whether spending 10 microseconds
more is going to have a noticeable impact.

> I also got a test rig to investigate arc.  It seems there is a huge
> problem with the SKCIPHER stack structure on that platform.  For
> reasons I still can't fathom, the compiler thinks it needs at least
> 0.5k of stack for this one structure.  I'm sure its something to do
> with an incorrect crypto alignment on arc, but I can't yet find the
> root cause.
>

Maybe SKCIPHER_ON_STACK() needs the same treatment as
660d2062190db131d2feaf19914e90f868fe285c?

The catch here is that if we reduce the alignment of the buffer, the
req pointer will not have the alignment of the typedef, and so we will
be lying to the compiler.

This is all a result of the way we abuse alignment to pad out data
fields that may be used for inbound non-coherent DMA, and this is
something that is being fixed at the moment.


> > I don't know if that is a consideration or not. The AES library code
> > is generic C code that was written to be constant-time, rather than
> > fast. The fact that CFB only uses the encryption side of it is
> > fortunate, because decryption is even slower.
>
> I think for the TPM, since the encryption isn't exactly bulk (it's
> really under 1k for command and response encryption) it doesn't matter
> ... in fact setting up the accelerator is likely a bigger overhead.
>
> > So the question is whether this will actually be a bottleneck in this
> > particular scenario. The synchronous accelerated AES implementations
> > are all SIMD based, which means there is some overhead, and some
> > degree of parallelism is also needed to take full advantage, and CFB
> > only allows this for decryption to begin with, as encryption uses
> > ciphertext block N-1 as AES input for encrypting block N.
> >
> > So maybe this is terrible advice, but the code will look so much
> > better for it, and we can always add back the performance later if it
> > is really an impediment.
>
> It's definitely smaller and neater, yes.  I'll post a v3 based on this,
> but when might it go upstream?  In my post I'll put your aescfb as
> patch 1 so the static checkers don't go haywire about missing function
> exports, and we can drop that patch when it is upstream.
>

I'll add some test cases and send it to the list.

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

[Jarkko Sakkinen]

On Tue, Feb 14, 2023 at 02:54:02PM +0100, Ard Biesheuvel wrote:
> On Mon, 13 Feb 2023 at 08:45, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> >
> > On Fri, Feb 10, 2023 at 09:48:15AM -0500, James Bottomley wrote:
> > > 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).
> >
> > Would it such a big deal to allocate shash from heap? That would
> > be IMHO more robust in the end.
> >
> 
> Can we avoid shashes and sync skciphers at all? We have sha256 and AES
> library routines these days, and AES in CFB mode seems like a good
> candidate for a library implementation as well - it uses AES
> encryption only, and is quite straight forward to implement. [0]
> 
> The crypto API is far too clunky for synchronous operations of
> algorithms that are known at compile time, and the requirement to use
> scatterlists for skciphers is especially horrid.

I'm cool with any solution not polluting the stack to its limits...

BR, Jarkko