Message ID | 20230124175516.5984-7-James.Bottomley@HansenPartnership.com (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | add integrity and security to TPM2 transactions | expand |
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
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
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?
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
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
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
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
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
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
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
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
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
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
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 >
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.
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.
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
diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig index 927088b2c3d3..74bd174f1a7b 100644 --- a/drivers/char/tpm/Kconfig +++ b/drivers/char/tpm/Kconfig @@ -9,6 +9,9 @@ menuconfig TCG_TPM imply SECURITYFS select CRYPTO select CRYPTO_HASH_INFO + select CRYPTO_ECDH + select CRYPTO_AES + select CRYPTO_CFB help If you have a TPM security chip in your system, which implements the Trusted Computing Group's specification, @@ -27,6 +30,16 @@ menuconfig TCG_TPM if TCG_TPM +config TPM_BUS_SECURITY + bool "Use secure transactions on the TPM bus" + default y + help + Setting this causes us to deploy a tamper resistent scheme + for communicating with the TPM to prevent or detect bus + snooping and iterposer attacks like TPM Genie. Saying Y here + adds some encryption overhead to all kernel to TPM + transactions. + config HW_RANDOM_TPM bool "TPM HW Random Number Generator support" depends on TCG_TPM && HW_RANDOM && !(TCG_TPM=y && HW_RANDOM=m) diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile index ad3594e383e1..10dc214aa093 100644 --- a/drivers/char/tpm/Makefile +++ b/drivers/char/tpm/Makefile @@ -17,6 +17,7 @@ tpm-y += eventlog/tpm1.o tpm-y += eventlog/tpm2.o tpm-y += tpm-buf.o +tpm-$(CONFIG_TPM_BUS_SECURITY) += tpm2-sessions.o tpm-$(CONFIG_ACPI) += tpm_ppi.o eventlog/acpi.o tpm-$(CONFIG_EFI) += eventlog/efi.o tpm-$(CONFIG_OF) += eventlog/of.o diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c index b054aa2fc84b..fe3320731217 100644 --- a/drivers/char/tpm/tpm-buf.c +++ b/drivers/char/tpm/tpm-buf.c @@ -27,6 +27,7 @@ void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal) head->tag = cpu_to_be16(tag); head->length = cpu_to_be32(sizeof(*head)); head->ordinal = cpu_to_be32(ordinal); + buf->handles = 0; } EXPORT_SYMBOL_GPL(tpm_buf_reset); diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h index a5fe37977103..b3eb0f31bfd9 100644 --- a/drivers/char/tpm/tpm.h +++ b/drivers/char/tpm/tpm.h @@ -246,4 +246,14 @@ void tpm_bios_log_setup(struct tpm_chip *chip); void tpm_bios_log_teardown(struct tpm_chip *chip); int tpm_dev_common_init(void); void tpm_dev_common_exit(void); + +#ifdef CONFIG_TPM_BUS_SECURITY +int tpm2_sessions_init(struct tpm_chip *chip); +#else +static inline int tpm2_sessions_init(struct tpm_chip *chip) +{ + return 0; +} +#endif + #endif diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c index 65d03867e114..056dad3dd5c9 100644 --- a/drivers/char/tpm/tpm2-cmd.c +++ b/drivers/char/tpm/tpm2-cmd.c @@ -759,6 +759,11 @@ int tpm2_auto_startup(struct tpm_chip *chip) rc = 0; } + if (rc) + goto out; + + rc = tpm2_sessions_init(chip); + out: /* * Infineon TPM in field upgrade mode will return no data for the number diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c new file mode 100644 index 000000000000..23a4aa8874ab --- /dev/null +++ b/drivers/char/tpm/tpm2-sessions.c @@ -0,0 +1,1216 @@ +// SPDX-License-Identifier: GPL-2.0 + +/* + * Copyright (C) 2018 James.Bottomley@HansenPartnership.com + * + * Cryptographic helper routines for handling TPM2 sessions for + * authorization HMAC and request response encryption. + * + * The idea is to ensure that every TPM command is HMAC protected by a + * session, meaning in-flight tampering would be detected and in + * addition all sensitive inputs and responses should be encrypted. + * + * The basic way this works is to use a TPM feature called salted + * sessions where a random secret used in session construction is + * encrypted to the public part of a known TPM key. The problem is we + * have no known keys, so initially a primary Elliptic Curve key is + * derived from the NULL seed (we use EC because most TPMs generate + * these keys much faster than RSA ones). The curve used is NIST_P256 + * because that's now mandated to be present in 'TCG TPM v2.0 + * Provisioning Guidance' + * + * Threat problems: the initial TPM2_CreatePrimary is not (and cannot + * be) session protected, so a clever Man in the Middle could return a + * public key they control to this command and from there intercept + * and decode all subsequent session based transactions. The kernel + * cannot mitigate this threat but, after boot, userspace can get + * proof this has not happened by asking the TPM to certify the NULL + * key. This certification would chain back to the TPM Endorsement + * Certificate and prove the NULL seed primary had not been tampered + * with and thus all sessions must have been cryptographically secure. + * To assist with this, the initial NULL seed public key name is made + * available in a sysfs file. + * + * Use of these functions: + * + * The design is all the crypto, hash and hmac gunk is confined in this + * file and never needs to be seen even by the kernel internal user. To + * the user there's an init function tpm2_sessions_init() that needs to + * be called once per TPM which generates the NULL seed primary key. + * + * Then there are six usage functions: + * + * tpm2_start_auth_session() which allocates the opaque auth structure + * and gets a session from the TPM. This must be called before + * any of the following functions. The session is protected by a + * session_key which is derived from a random salt value + * encrypted to the NULL seed. + * tpm2_end_auth_session() kills the session and frees the resources. + * Under normal operation this function is done by + * tpm_buf_check_hmac_response(), so this is only to be used on + * error legs where the latter is not executed. + * tpm_buf_append_name() to add a handle to the buffer. This must be + * used in place of the usual tpm_buf_append_u32() for adding + * handles because handles have to be processed specially when + * calculating the HMAC. In particular, for NV, volatile and + * permanent objects you now need to provide the name. + * tpm_buf_append_hmac_session() which appends the hmac session to the + * buf in the same way tpm_buf_append_auth does(). + * tpm_buf_fill_hmac_session() This calculates the correct hash and + * places it in the buffer. It must be called after the complete + * command buffer is finalized so it can fill in the correct HMAC + * based on the parameters. + * tpm_buf_check_hmac_response() which checks the session response in + * the buffer and calculates what it should be. If there's a + * mismatch it will log a warning and return an error. If + * tpm_buf_append_hmac_session() did not specify + * TPM_SA_CONTINUE_SESSION then the session will be closed (if it + * hasn't been consumed) and the auth structure freed. + */ + +#include "tpm.h" + +#include <linux/random.h> +#include <linux/scatterlist.h> + +#include <asm/unaligned.h> + +#include <crypto/aes.h> +#include <crypto/kpp.h> +#include <crypto/ecdh.h> +#include <crypto/hash.h> +#include <crypto/hmac.h> +#include <crypto/skcipher.h> + +/* if you change to AES256, you only need change this */ +#define AES_KEYBYTES AES_KEYSIZE_128 + +#define AES_KEYBITS (AES_KEYBYTES*8) +#define AUTH_MAX_NAMES 3 + +/* + * This is the structure that carries all the auth information (like + * session handle, nonces, session key and auth) from use to use it is + * designed to be opaque to anything outside. + */ +struct tpm2_auth { + u32 handle; + /* + * This has two meanings: before tpm_buf_fill_hmac_session() + * it marks the offset in the buffer of the start of the + * sessions (i.e. after all the handles). Once the buffer has + * been filled it markes the session number of our auth + * session so we can find it again in the response buffer. + * + * The two cases are distinguished because the first offset + * must always be greater than TPM_HEADER_SIZE and the second + * must be less than or equal to 5. + */ + u32 session; + /* + * the size here is variable and set by the size of our_nonce + * which must be between 16 and the name hash length. we set + * the maximum sha256 size for the greatest protection + */ + u8 our_nonce[SHA256_DIGEST_SIZE]; + u8 tpm_nonce[SHA256_DIGEST_SIZE]; + /* + * the salt is only used across the session command/response + * after that it can be used as a scratch area + */ + union { + u8 salt[EC_PT_SZ]; + /* scratch for key + IV */ + u8 scratch[AES_KEYBYTES + AES_BLOCK_SIZE]; + }; + /* + * the session key and passphrase are the same size as the + * name digest (sha256 again). The session key is constant + * for the use of the session and the passphrase can change + * with every invocation. + * + * Note: these fields must be adjacent and in this order + * because several HMAC/KDF schemes use the combination of the + * session_key and passphrase. + */ + u8 session_key[SHA256_DIGEST_SIZE]; + u8 passphrase[SHA256_DIGEST_SIZE]; + int passphraselen; + /* saved session attributes */ + u8 attrs; + __be32 ordinal; + struct crypto_sync_skcipher *aes; + struct tpm_chip *chip; + /* 3 names of handles: name_h is handle, name is name of handle */ + u32 name_h[AUTH_MAX_NAMES]; + u8 name[AUTH_MAX_NAMES][2 + SHA512_DIGEST_SIZE]; +}; + +/* + * this is our static crypto shash. This is possible because the hash + * is multi-threaded and all the state stored in the desc + */ +static struct crypto_shash *sha256_hash; + +/* + * Name Size based on TPM algorithm (assumes no hash bigger than 255) + */ +static u8 name_size(const u8 *name) +{ + static u8 size_map[] = { + [TPM_ALG_SHA1] = SHA1_DIGEST_SIZE, + [TPM_ALG_SHA256] = SHA256_DIGEST_SIZE, + [TPM_ALG_SHA384] = SHA384_DIGEST_SIZE, + [TPM_ALG_SHA512] = SHA512_DIGEST_SIZE, + }; + u16 alg = get_unaligned_be16(name); + return size_map[alg] + 2; +} + +/* + * It turns out the crypto hmac(sha256) is hard for us to consume + * because it assumes a fixed key and the TPM seems to change the key + * on every operation, so we weld the hmac init and final functions in + * here to give it the same usage characteristics as a regular hash + */ +static void hmac_init(struct shash_desc *desc, u8 *key, int keylen) +{ + u8 pad[SHA256_BLOCK_SIZE]; + int i; + + desc->tfm = sha256_hash; + crypto_shash_init(desc); + for (i = 0; i < sizeof(pad); i++) { + if (i < keylen) + pad[i] = key[i]; + else + pad[i] = 0; + pad[i] ^= HMAC_IPAD_VALUE; + } + crypto_shash_update(desc, pad, sizeof(pad)); +} + +static void hmac_final(struct shash_desc *desc, u8 *key, int keylen, u8 *out) +{ + u8 pad[SHA256_BLOCK_SIZE]; + int i; + + for (i = 0; i < sizeof(pad); i++) { + if (i < keylen) + pad[i] = key[i]; + else + pad[i] = 0; + pad[i] ^= HMAC_OPAD_VALUE; + } + + /* collect the final hash; use out as temporary storage */ + crypto_shash_final(desc, out); + + /* reuse the desc */ + crypto_shash_init(desc); + crypto_shash_update(desc, pad, sizeof(pad)); + crypto_shash_update(desc, out, SHA256_DIGEST_SIZE); + crypto_shash_final(desc, out); +} + +/* + * assume hash sha256 and nonces u, v of size SHA256_DIGEST_SIZE but + * otherwise standard KDFa. Note output is in bytes not bits. + */ +static void KDFa(u8 *key, int keylen, const char *label, u8 *u, + u8 *v, int bytes, u8 *out) +{ + u32 counter; + const __be32 bits = cpu_to_be32(bytes * 8); + + for (counter = 1; bytes > 0; bytes -= SHA256_DIGEST_SIZE, counter++, + out += SHA256_DIGEST_SIZE) { + SHASH_DESC_ON_STACK(desc, sha256_hash); + __be32 c = cpu_to_be32(counter); + + hmac_init(desc, key, keylen); + crypto_shash_update(desc, (u8 *)&c, sizeof(c)); + crypto_shash_update(desc, label, strlen(label)+1); + crypto_shash_update(desc, u, SHA256_DIGEST_SIZE); + crypto_shash_update(desc, v, SHA256_DIGEST_SIZE); + crypto_shash_update(desc, (u8 *)&bits, sizeof(bits)); + hmac_final(desc, key, keylen, out); + } +} + +/* + * Somewhat of a bastardization of the real KDFe. We're assuming + * we're working with known point sizes for the input parameters and + * the hash algorithm is fixed at sha256. Because we know that the + * point size is 32 bytes like the hash size, there's no need to loop + * in this KDF. + */ +static void KDFe(u8 z[EC_PT_SZ], const char *str, u8 *pt_u, u8 *pt_v, + u8 *keyout) +{ + SHASH_DESC_ON_STACK(desc, sha256_hash); + /* + * this should be an iterative counter, but because we know + * we're only taking 32 bytes for the point using a sha256 + * hash which is also 32 bytes, there's only one loop + */ + __be32 c = cpu_to_be32(1); + + desc->tfm = sha256_hash; + + crypto_shash_init(desc); + /* counter (BE) */ + crypto_shash_update(desc, (u8 *)&c, sizeof(c)); + /* secret value */ + crypto_shash_update(desc, z, EC_PT_SZ); + /* string including trailing zero */ + crypto_shash_update(desc, str, strlen(str)+1); + crypto_shash_update(desc, pt_u, EC_PT_SZ); + crypto_shash_update(desc, pt_v, EC_PT_SZ); + crypto_shash_final(desc, keyout); +} + +static void tpm_buf_append_salt(struct tpm_buf *buf, struct tpm_chip *chip, + struct tpm2_auth *auth) +{ + struct crypto_kpp *kpp; + struct kpp_request *req; + struct scatterlist s[2], d[1]; + struct ecdh p = {0}; + u8 encoded_key[EC_PT_SZ], *x, *y; + unsigned int buf_len; + u8 *secret; + + secret = kmalloc(EC_PT_SZ, GFP_KERNEL); + if (!secret) + return; + + /* secret is two sized points */ + tpm_buf_append_u16(buf, (EC_PT_SZ + 2)*2); + /* + * we cheat here and append uninitialized data to form + * the points. All we care about is getting the two + * co-ordinate pointers, which will be used to overwrite + * the uninitialized data + */ + tpm_buf_append_u16(buf, EC_PT_SZ); + x = &buf->data[tpm_buf_length(buf)]; + tpm_buf_append(buf, encoded_key, EC_PT_SZ); + tpm_buf_append_u16(buf, EC_PT_SZ); + y = &buf->data[tpm_buf_length(buf)]; + tpm_buf_append(buf, encoded_key, EC_PT_SZ); + sg_init_table(s, 2); + sg_set_buf(&s[0], x, EC_PT_SZ); + sg_set_buf(&s[1], y, EC_PT_SZ); + + kpp = crypto_alloc_kpp("ecdh-nist-p256", CRYPTO_ALG_INTERNAL, 0); + if (IS_ERR(kpp)) { + dev_err(&chip->dev, "crypto ecdh allocation failed\n"); + goto out_free; + } + + buf_len = crypto_ecdh_key_len(&p); + if (sizeof(encoded_key) < buf_len) { + dev_err(&chip->dev, "salt buffer too small needs %d\n", + buf_len); + goto out; + } + crypto_ecdh_encode_key(encoded_key, buf_len, &p); + /* this generates a random private key */ + crypto_kpp_set_secret(kpp, encoded_key, buf_len); + + /* salt is now the public point of this private key */ + req = kpp_request_alloc(kpp, GFP_KERNEL); + if (!req) + goto out; + kpp_request_set_input(req, NULL, 0); + kpp_request_set_output(req, s, EC_PT_SZ*2); + crypto_kpp_generate_public_key(req); + /* + * we're not done: now we have to compute the shared secret + * which is our private key multiplied by the tpm_key public + * point, we actually only take the x point and discard the y + * point and feed it through KDFe to get the final secret salt + */ + sg_set_buf(&s[0], chip->ec_point_x, EC_PT_SZ); + sg_set_buf(&s[1], chip->ec_point_y, EC_PT_SZ); + kpp_request_set_input(req, s, EC_PT_SZ*2); + sg_init_one(d, secret, EC_PT_SZ); + kpp_request_set_output(req, d, EC_PT_SZ); + crypto_kpp_compute_shared_secret(req); + kpp_request_free(req); + + /* pass the shared secret through KDFe for salt */ + KDFe(secret, "SECRET", x, chip->ec_point_x, auth->salt); + out: + crypto_free_kpp(kpp); + out_free: + kfree(secret); +} + +/** + * tpm_buf_append_hmac_session() append a TPM session element + * @buf: The buffer to be appended + * @auth: the auth structure allocated by tpm2_start_auth_session() + * @attributes: The session attributes + * @passphrase: The session authority (NULL if none) + * @passphraselen: The length of the session authority (0 if none) + * + * This fills in a session structure in the TPM command buffer, except + * for the HMAC which cannot be computed until the command buffer is + * complete. The type of session is controlled by the @attributes, + * the main ones of which are TPM2_SA_CONTINUE_SESSION which means the + * session won't terminate after tpm_buf_check_hmac_response(), + * TPM2_SA_DECRYPT which means this buffers first parameter should be + * encrypted with a session key and TPM2_SA_ENCRYPT, which means the + * response buffer's first parameter needs to be decrypted (confusing, + * but the defines are written from the point of view of the TPM). + * + * Any session appended by this command must be finalized by calling + * tpm_buf_fill_hmac_session() otherwise the HMAC will be incorrect + * and the TPM will reject the command. + * + * As with most tpm_buf operations, success is assumed because failure + * will be caused by an incorrect programming model and indicated by a + * kernel message. + */ +void tpm_buf_append_hmac_session(struct tpm_buf *buf, struct tpm2_auth *auth, + u8 attributes, u8 *passphrase, + int passphraselen) +{ + u8 nonce[SHA256_DIGEST_SIZE]; + u32 len; + + /* + * The Architecture Guide requires us to strip trailing zeros + * before computing the HMAC + */ + while (passphrase && passphraselen > 0 + && passphrase[passphraselen - 1] == '\0') + passphraselen--; + + auth->attrs = attributes; + auth->passphraselen = passphraselen; + if (passphraselen) + memcpy(auth->passphrase, passphrase, passphraselen); + + if (auth->session != tpm_buf_length(buf)) { + /* we're not the first session */ + len = get_unaligned_be32(&buf->data[auth->session]); + if (4 + len + auth->session != tpm_buf_length(buf)) { + WARN(1, "session length mismatch, cannot append"); + return; + } + + /* add our new session */ + len += 9 + 2 * SHA256_DIGEST_SIZE; + put_unaligned_be32(len, &buf->data[auth->session]); + } else { + tpm_buf_append_u32(buf, 9 + 2 * SHA256_DIGEST_SIZE); + } + + /* random number for our nonce */ + get_random_bytes(nonce, sizeof(nonce)); + memcpy(auth->our_nonce, nonce, sizeof(nonce)); + tpm_buf_append_u32(buf, auth->handle); + /* our new nonce */ + tpm_buf_append_u16(buf, SHA256_DIGEST_SIZE); + tpm_buf_append(buf, nonce, SHA256_DIGEST_SIZE); + tpm_buf_append_u8(buf, auth->attrs); + /* and put a placeholder for the hmac */ + tpm_buf_append_u16(buf, SHA256_DIGEST_SIZE); + tpm_buf_append(buf, nonce, SHA256_DIGEST_SIZE); +} +EXPORT_SYMBOL(tpm_buf_append_hmac_session); + +/** + * tpm_buf_fill_hmac_session() - finalize the session HMAC + * @buf: The buffer to be appended + * @auth: the auth structure allocated by tpm2_start_auth_session() + * + * This command must not be called until all of the parameters have + * been appended to @buf otherwise the computed HMAC will be + * incorrect. + * + * This function computes and fills in the session HMAC using the + * session key and, if TPM2_SA_DECRYPT was specified, computes the + * encryption key and encrypts the first parameter of the command + * buffer with it. + * + * As with most tpm_buf operations, success is assumed because failure + * will be caused by an incorrect programming model and indicated by a + * kernel message. + */ +void tpm_buf_fill_hmac_session(struct tpm_buf *buf, struct tpm2_auth *auth) +{ + u32 cc, handles, val; + struct tpm_chip *chip = auth->chip; + int i; + struct tpm_header *head = (struct tpm_header *)buf->data; + const u8 *s, *p; + u8 *hmac = NULL; + u32 attrs; + u8 cphash[SHA256_DIGEST_SIZE]; + SHASH_DESC_ON_STACK(desc, sha256_hash); + + /* save the command code in BE format */ + auth->ordinal = head->ordinal; + + desc->tfm = sha256_hash; + + cc = be32_to_cpu(head->ordinal); + + i = tpm2_find_cc(chip, cc); + if (i < 0) { + dev_err(&chip->dev, "Command 0x%x not found in TPM\n", cc); + return; + } + attrs = chip->cc_attrs_tbl[i]; + + handles = (attrs >> TPM2_CC_ATTR_CHANDLES) & GENMASK(2, 0); + + s = &buf->data[TPM_HEADER_SIZE]; + /* + * just check the names, it's easy to make mistakes. This + * would happen if someone added a handle via + * tpm_buf_append_u32() instead of tpm_buf_append_name() + */ + for (i = 0; i < handles; i++) { + u32 handle = tpm_get_inc_u32(&s); + + if (auth->name_h[i] != handle) { + dev_err(&chip->dev, "TPM: handle %d wrong for name\n", + i); + return; + } + } + /* point s to the start of the sessions */ + val = tpm_get_inc_u32(&s); + /* point p to the start of the parameters */ + p = s + val; + for (i = 1; s < p; i++) { + u32 handle = tpm_get_inc_u32(&s); + u16 len; + u8 a; + + /* nonce (already in auth) */ + len = tpm_get_inc_u16(&s); + s += len; + + a = *s++; + + len = tpm_get_inc_u16(&s); + if (handle == auth->handle && auth->attrs == a) { + hmac = (u8 *)s; + /* + * save our session number so we know which + * session in the response belongs to us + */ + auth->session = i; + } + + s += len; + } + if (s != p) { + dev_err(&chip->dev, "TPM session length is incorrect\n"); + return; + } + if (!hmac) { + dev_err(&chip->dev, "TPM could not find HMAC session\n"); + return; + } + + /* encrypt before HMAC */ + if (auth->attrs & TPM2_SA_DECRYPT) { + struct scatterlist sg[1]; + u16 len; + SYNC_SKCIPHER_REQUEST_ON_STACK(req, auth->aes); + DECLARE_CRYPTO_WAIT(wait); + + skcipher_request_set_sync_tfm(req, auth->aes); + skcipher_request_set_callback(req, CRYPTO_TFM_REQ_MAY_SLEEP, + crypto_req_done, &wait); + + /* need key and IV */ + KDFa(auth->session_key, SHA256_DIGEST_SIZE + + auth->passphraselen, "CFB", auth->our_nonce, + auth->tpm_nonce, AES_KEYBYTES + AES_BLOCK_SIZE, + auth->scratch); + crypto_sync_skcipher_setkey(auth->aes, auth->scratch, AES_KEYBYTES); + len = tpm_get_inc_u16(&p); + sg_init_one(sg, p, len); + skcipher_request_set_crypt(req, sg, sg, len, + auth->scratch + AES_KEYBYTES); + crypto_wait_req(crypto_skcipher_encrypt(req), &wait); + /* reset p to beginning of parameters for HMAC */ + p -= 2; + } + + crypto_shash_init(desc); + /* ordinal is already BE */ + crypto_shash_update(desc, (u8 *)&head->ordinal, sizeof(head->ordinal)); + /* add the handle names */ + for (i = 0; i < handles; i++) { + u8 mso = auth->name_h[i] >> 24; + + if (mso == 0x81 || mso == 0x80 || mso == 0x01) { + crypto_shash_update(desc, auth->name[i], + name_size(auth->name[i])); + } else { + __be32 h = cpu_to_be32(auth->name_h[i]); + + crypto_shash_update(desc, (u8 *)&h, 4); + } + } + if (buf->data - s != tpm_buf_length(buf)) + crypto_shash_update(desc, s, buf->data + + tpm_buf_length(buf) - s); + crypto_shash_final(desc, cphash); + + /* now calculate the hmac */ + hmac_init(desc, auth->session_key, sizeof(auth->session_key) + + auth->passphraselen); + crypto_shash_update(desc, cphash, sizeof(cphash)); + crypto_shash_update(desc, auth->our_nonce, sizeof(auth->our_nonce)); + crypto_shash_update(desc, auth->tpm_nonce, sizeof(auth->tpm_nonce)); + crypto_shash_update(desc, &auth->attrs, 1); + hmac_final(desc, auth->session_key, sizeof(auth->session_key) + + auth->passphraselen, hmac); +} +EXPORT_SYMBOL(tpm_buf_fill_hmac_session); + +static int parse_read_public(char *name, const u8 *data) +{ + struct tpm_header *head = (struct tpm_header *)data; + u32 tot_len = be32_to_cpu(head->length); + u32 val; + + data += TPM_HEADER_SIZE; + /* we're starting after the header so adjust the length */ + tot_len -= TPM_HEADER_SIZE; + + /* skip public */ + val = tpm_get_inc_u16(&data); + if (val > tot_len) + return -EINVAL; + data += val; + /* name */ + val = tpm_get_inc_u16(&data); + if (val != name_size(data)) + return -EINVAL; + memcpy(name, data, name_size(data)); + /* forget the rest */ + return 0; +} + +static int tpm2_readpublic(struct tpm_chip *chip, u32 handle, char *name) +{ + struct tpm_buf buf; + int rc; + + rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_READ_PUBLIC); + if (rc) + return rc; + + tpm_buf_append_u32(&buf, handle); + rc = tpm_transmit_cmd(chip, &buf, 0, "read public"); + if (rc == TPM2_RC_SUCCESS) + rc = parse_read_public(name, buf.data); + + tpm_buf_destroy(&buf); + + return rc; +} + +/** + * tpm_buf_append_name() - add a handle area to the buffer + * @buf: The buffer to be appended + * @auth: the auth structure allocated by tpm2_start_auth_session() + * @handle: The handle to be appended + * @name: The name of the handle (may be NULL) + * + * In order to compute session HMACs, we need to know the names of the + * objects pointed to by the handles. For most objects, this is simly + * the actual 4 byte handle or an empty buf (in these cases @name + * should be NULL) but for volatile objects, permanent objects and NV + * areas, the name is defined as the hash (according to the name + * algorithm which should be set to sha256) of the public area to + * which the two byte algorithm id has been appended. For these + * objects, the @name pointer should point to this. If a name is + * required but @name is NULL, then TPM2_ReadPublic() will be called + * on the handle to obtain the name. + * + * As with most tpm_buf operations, success is assumed because failure + * will be caused by an incorrect programming model and indicated by a + * kernel message. + */ +void tpm_buf_append_name(struct tpm_buf *buf, struct tpm2_auth *auth, + u32 handle, u8 *name) +{ + int slot; + u8 mso = handle >> 24; + + slot = (tpm_buf_length(buf) - TPM_HEADER_SIZE)/4; + if (slot >= AUTH_MAX_NAMES) { + dev_err(&auth->chip->dev, "TPM: too many handles\n"); + return; + } + WARN(auth->session != tpm_buf_length(buf), + "name added in wrong place\n"); + tpm_buf_append_u32(buf, handle); + auth->session += 4; + + if (mso == 0x81 || mso == 0x80 || mso == 0x01) { + if (!name) + tpm2_readpublic(auth->chip, handle, auth->name[slot]); + } else { + if (name) + dev_err(&auth->chip->dev, "TPM: Handle does not require name but one is specified\n"); + } + + auth->name_h[slot] = handle; + if (name) + memcpy(auth->name[slot], name, name_size(name)); +} +EXPORT_SYMBOL(tpm_buf_append_name); + +/** + * tpm_buf_check_hmac_response() - check the TPM return HMAC for correctness + * @buf: the original command buffer (which now contains the response) + * @auth: the auth structure allocated by tpm2_start_auth_session() + * @rc: the return code from tpm_transmit_cmd + * + * If @rc is non zero, @buf may not contain an actual return, so @rc + * is passed through as the return and the session cleaned up and + * de-allocated if required (this is required if + * TPM2_SA_CONTINUE_SESSION was not specified as a session flag). + * + * If @rc is zero, the response HMAC is computed against the returned + * @buf and matched to the TPM one in the session area. If there is a + * mismatch, an error is logged and -EINVAL returned. + * + * The reason for this is that the command issue and HMAC check + * sequence should look like: + * + * rc = tpm_transmit_cmd(...); + * rc = tpm_buf_check_hmac_response(&buf, auth, rc); + * if (rc) + * ... + * + * Which is easily layered into the current contrl flow. + * + * Returns: 0 on success or an error. + */ +int tpm_buf_check_hmac_response(struct tpm_buf *buf, struct tpm2_auth *auth, + int rc) +{ + struct tpm_header *head = (struct tpm_header *)buf->data; + struct tpm_chip *chip = auth->chip; + const u8 *s, *p; + u8 rphash[SHA256_DIGEST_SIZE]; + u32 attrs; + SHASH_DESC_ON_STACK(desc, sha256_hash); + u16 tag = be16_to_cpu(head->tag); + u32 cc = be32_to_cpu(auth->ordinal); + int parm_len, len, i, handles; + + if (auth->session >= TPM_HEADER_SIZE) { + WARN(1, "tpm session not filled correctly\n"); + goto out; + } + + if (rc != 0) + /* pass non success rc through and close the session */ + goto out; + + rc = -EINVAL; + if (tag != TPM2_ST_SESSIONS) { + dev_err(&chip->dev, "TPM: HMAC response check has no sessions tag\n"); + goto out; + } + + i = tpm2_find_cc(chip, cc); + if (i < 0) + goto out; + attrs = chip->cc_attrs_tbl[i]; + handles = (attrs >> TPM2_CC_ATTR_RHANDLE) & 1; + + /* point to area beyond handles */ + s = &buf->data[TPM_HEADER_SIZE + handles * 4]; + parm_len = tpm_get_inc_u32(&s); + p = s; + s += parm_len; + /* skip over any sessions before ours */ + for (i = 0; i < auth->session - 1; i++) { + len = tpm_get_inc_u16(&s); + s += len + 1; + len = tpm_get_inc_u16(&s); + s += len; + } + /* TPM nonce */ + len = tpm_get_inc_u16(&s); + if (s - buf->data + len > tpm_buf_length(buf)) + goto out; + if (len != SHA256_DIGEST_SIZE) + goto out; + memcpy(auth->tpm_nonce, s, len); + s += len; + attrs = *s++; + len = tpm_get_inc_u16(&s); + if (s - buf->data + len != tpm_buf_length(buf)) + goto out; + if (len != SHA256_DIGEST_SIZE) + goto out; + /* + * s points to the HMAC. now calculate comparison, beginning + * with rphash + */ + desc->tfm = sha256_hash; + crypto_shash_init(desc); + /* yes, I know this is now zero, but it's what the standard says */ + crypto_shash_update(desc, (u8 *)&head->return_code, + sizeof(head->return_code)); + /* ordinal is already BE */ + crypto_shash_update(desc, (u8 *)&auth->ordinal, sizeof(auth->ordinal)); + crypto_shash_update(desc, p, parm_len); + crypto_shash_final(desc, rphash); + + /* now calculate the hmac */ + hmac_init(desc, auth->session_key, sizeof(auth->session_key) + + auth->passphraselen); + crypto_shash_update(desc, rphash, sizeof(rphash)); + crypto_shash_update(desc, auth->tpm_nonce, sizeof(auth->tpm_nonce)); + crypto_shash_update(desc, auth->our_nonce, sizeof(auth->our_nonce)); + crypto_shash_update(desc, &auth->attrs, 1); + /* we're done with the rphash, so put our idea of the hmac there */ + hmac_final(desc, auth->session_key, sizeof(auth->session_key) + + auth->passphraselen, rphash); + if (memcmp(rphash, s, SHA256_DIGEST_SIZE) == 0) { + rc = 0; + } else { + dev_err(&auth->chip->dev, "TPM: HMAC check failed\n"); + goto out; + } + + /* now do response decryption */ + if (auth->attrs & TPM2_SA_ENCRYPT) { + struct scatterlist sg[1]; + SYNC_SKCIPHER_REQUEST_ON_STACK(req, auth->aes); + DECLARE_CRYPTO_WAIT(wait); + + skcipher_request_set_sync_tfm(req, auth->aes); + skcipher_request_set_callback(req, CRYPTO_TFM_REQ_MAY_SLEEP, + crypto_req_done, &wait); + + /* need key and IV */ + KDFa(auth->session_key, SHA256_DIGEST_SIZE + + auth->passphraselen, "CFB", auth->tpm_nonce, + auth->our_nonce, AES_KEYBYTES + AES_BLOCK_SIZE, + auth->scratch); + crypto_sync_skcipher_setkey(auth->aes, auth->scratch, AES_KEYBYTES); + len = tpm_get_inc_u16(&p); + sg_init_one(sg, p, len); + skcipher_request_set_crypt(req, sg, sg, len, + auth->scratch + AES_KEYBYTES); + crypto_wait_req(crypto_skcipher_decrypt(req), &wait); + } + + out: + if ((auth->attrs & TPM2_SA_CONTINUE_SESSION) == 0) { + /* manually close the session if it wasn't consumed */ + if (rc) + tpm2_flush_context(chip, auth->handle); + crypto_free_sync_skcipher(auth->aes); + kfree(auth); + } else { + /* reset for next use */ + auth->session = TPM_HEADER_SIZE; + } + + return rc; +} +EXPORT_SYMBOL(tpm_buf_check_hmac_response); + +/** + * tpm2_end_auth_session - kill the allocated auth session + * @auth: the auth structure allocated by tpm2_start_auth_session() + * + * ends the session started by tpm2_start_auth_session and frees all + * the resources. Under normal conditions, + * tpm_buf_check_hmac_response() will correctly end the session if + * required, so this function is only for use in error legs that will + * bypass the normal invocation of tpm_buf_check_hmac_respons(). + */ +void tpm2_end_auth_session(struct tpm2_auth *auth) +{ + tpm2_flush_context(auth->chip, auth->handle); + crypto_free_sync_skcipher(auth->aes); + kfree(auth); +} +EXPORT_SYMBOL(tpm2_end_auth_session); + +static int parse_start_auth_session(struct tpm2_auth *auth, const u8 *data) +{ + struct tpm_header *head = (struct tpm_header *)data; + u32 tot_len = be32_to_cpu(head->length); + u32 val; + + data += TPM_HEADER_SIZE; + /* we're starting after the header so adjust the length */ + tot_len -= TPM_HEADER_SIZE; + + /* should have handle plus nonce */ + if (tot_len != 4 + 2 + sizeof(auth->tpm_nonce)) + return -EINVAL; + + auth->handle = tpm_get_inc_u32(&data); + val = tpm_get_inc_u16(&data); + if (val != sizeof(auth->tpm_nonce)) + return -EINVAL; + memcpy(auth->tpm_nonce, data, sizeof(auth->tpm_nonce)); + /* now compute the session key from the nonces */ + KDFa(auth->salt, sizeof(auth->salt), "ATH", auth->tpm_nonce, + auth->our_nonce, sizeof(auth->session_key), auth->session_key); + + return 0; +} + +/** + * tpm2_start_auth_session - create a HMAC authentication session with the TPM + * @chip: the TPM chip structure to create the session with + * @authp: A pointer to an opaque tpm2_auth structure to be allocated + * + * This function loads the NULL seed from its saved context and starts + * an authentication session on the null seed, allocates a tpm2_auth + * structure to contain all the session details necessary for + * performing the HMAC, encrypt and decrypt operations, fills it in + * and returns. The NULL seed is flushed before this function returns. + * + * Return: zero on success or actual error encountered. If return is + * zero, @authp will be allocated. + */ +int tpm2_start_auth_session(struct tpm_chip *chip, struct tpm2_auth **authp) +{ + struct tpm_buf buf; + struct tpm2_auth *auth; + int rc; + unsigned int offset = 0; /* dummy offset for null seed context */ + u32 nullkey; + + auth = kmalloc(sizeof(**authp), GFP_KERNEL); + if (!auth) + return -ENOMEM; + + rc = tpm2_load_context(chip, chip->tpmkeycontext, &offset, + &nullkey); + if (rc) + goto out; + + auth->chip = chip; + auth->session = TPM_HEADER_SIZE; + + rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_START_AUTH_SESS); + if (rc) + goto out; + + /* salt key handle */ + tpm_buf_append_u32(&buf, nullkey); + /* bind key handle */ + tpm_buf_append_u32(&buf, TPM2_RH_NULL); + /* nonce caller */ + get_random_bytes(auth->our_nonce, sizeof(auth->our_nonce)); + tpm_buf_append_u16(&buf, sizeof(auth->our_nonce)); + tpm_buf_append(&buf, auth->our_nonce, sizeof(auth->our_nonce)); + + /* append encrypted salt and squirrel away unencrypted in auth */ + tpm_buf_append_salt(&buf, chip, auth); + /* session type (HMAC, audit or policy) */ + tpm_buf_append_u8(&buf, TPM2_SE_HMAC); + + /* symmetric encryption parameters */ + /* symmetric algorithm */ + tpm_buf_append_u16(&buf, TPM_ALG_AES); + /* bits for symmetric algorithm */ + tpm_buf_append_u16(&buf, AES_KEYBITS); + /* symmetric algorithm mode (must be CFB) */ + tpm_buf_append_u16(&buf, TPM_ALG_CFB); + /* hash algorithm for session */ + tpm_buf_append_u16(&buf, TPM_ALG_SHA256); + + rc = tpm_transmit_cmd(chip, &buf, 0, "start auth session"); + tpm2_flush_context(chip, nullkey); + + if (rc == TPM2_RC_SUCCESS) + rc = parse_start_auth_session(auth, buf.data); + + tpm_buf_destroy(&buf); + + if (rc) + goto out; + + auth->aes = crypto_alloc_sync_skcipher("cfb(aes)", 0, 0); + if (IS_ERR(auth->aes)) { + rc = PTR_ERR(auth->aes); + dev_err(&chip->dev, "TPM: error getting cfb(aes): %d\n", rc); + } + out: + if (rc) + kfree(auth); + else + *authp = auth; + + return rc; +} +EXPORT_SYMBOL(tpm2_start_auth_session); + +static int parse_create_primary(struct tpm_chip *chip, u8 *data, u32 *nullkey) +{ + struct tpm_header *head = (struct tpm_header *)data; + u16 len; + u32 tot_len = be32_to_cpu(head->length); + u32 val, parm_len; + const u8 *resp, *tmp; + SHASH_DESC_ON_STACK(desc, sha256_hash); + + data += TPM_HEADER_SIZE; + /* we're starting after the header so adjust the length */ + tot_len -= TPM_HEADER_SIZE; + + resp = data; + *nullkey = tpm_get_inc_u32(&resp); + parm_len = tpm_get_inc_u32(&resp); + if (parm_len + 8 > tot_len) + return -EINVAL; + len = tpm_get_inc_u16(&resp); + tmp = resp; + /* now we have the public area, compute the name of the object */ + desc->tfm = sha256_hash; + put_unaligned_be16(TPM_ALG_SHA256, chip->tpmkeyname); + crypto_shash_init(desc); + crypto_shash_update(desc, resp, len); + crypto_shash_final(desc, chip->tpmkeyname + 2); + /* validate the public key */ + val = tpm_get_inc_u16(&tmp); + /* key type (must be what we asked for) */ + if (val != TPM_ALG_ECC) + return -EINVAL; + val = tpm_get_inc_u16(&tmp); + /* name algorithm */ + if (val != TPM_ALG_SHA256) + return -EINVAL; + val = tpm_get_inc_u32(&tmp); + /* object properties */ + if (val != (TPM2_OA_NO_DA | + TPM2_OA_FIXED_TPM | + TPM2_OA_FIXED_PARENT | + TPM2_OA_SENSITIVE_DATA_ORIGIN | + TPM2_OA_USER_WITH_AUTH | + TPM2_OA_DECRYPT | + TPM2_OA_RESTRICTED)) + return -EINVAL; + /* auth policy (empty) */ + val = tpm_get_inc_u16(&tmp); + if (val != 0) + return -EINVAL; + val = tpm_get_inc_u16(&tmp); + /* symmetric key parameters */ + if (val != TPM_ALG_AES) + return -EINVAL; + val = tpm_get_inc_u16(&tmp); + /* symmetric key length */ + if (val != AES_KEYBITS) + return -EINVAL; + val = tpm_get_inc_u16(&tmp); + /* symmetric encryption scheme */ + if (val != TPM_ALG_CFB) + return -EINVAL; + val = tpm_get_inc_u16(&tmp); + /* signing scheme */ + if (val != TPM_ALG_NULL) + return -EINVAL; + val = tpm_get_inc_u16(&tmp); + /* ECC Curve */ + if (val != TPM2_ECC_NIST_P256) + return -EINVAL; + val = tpm_get_inc_u16(&tmp); + /* KDF Scheme */ + if (val != TPM_ALG_NULL) + return -EINVAL; + val = tpm_get_inc_u16(&tmp); + /* x point */ + if (val != 32) + return -EINVAL; + memcpy(chip->ec_point_x, tmp, val); + tmp += val; + val = tpm_get_inc_u16(&tmp); + if (val != 32) + return -EINVAL; + memcpy(chip->ec_point_y, tmp, val); + tmp += val; + resp += len; + /* should have exactly consumed the tpm2b public structure */ + if (tmp != resp) + return -EINVAL; + if (resp - data > parm_len) + return -EINVAL; + /* creation data (skip) */ + len = tpm_get_inc_u16(&resp); + resp += len; + if (resp - data > parm_len) + return -EINVAL; + /* creation digest (must be sha256) */ + len = tpm_get_inc_u16(&resp); + resp += len; + if (len != SHA256_DIGEST_SIZE || resp - data > parm_len) + return -EINVAL; + /* TPMT_TK_CREATION follows */ + /* tag, must be TPM_ST_CREATION (0x8021) */ + val = tpm_get_inc_u16(&resp); + if (val != TPM2_ST_CREATION || resp - data > parm_len) + return -EINVAL; + /* hierarchy (must be NULL) */ + val = tpm_get_inc_u32(&resp); + if (val != TPM2_RH_NULL || resp - data > parm_len) + return -EINVAL; + /* the ticket digest HMAC (might not be sha256) */ + len = tpm_get_inc_u16(&resp); + resp += len; + if (resp - data > parm_len) + return -EINVAL; + /* + * finally we have the name, which is a sha256 digest plus a 2 + * byte algorithm type + */ + len = tpm_get_inc_u16(&resp); + if (resp + len - data != parm_len + 8) + return -EINVAL; + if (len != SHA256_DIGEST_SIZE + 2) + return -EINVAL; + + if (memcmp(chip->tpmkeyname, resp, SHA256_DIGEST_SIZE + 2) != 0) { + dev_err(&chip->dev, "NULL Seed name comparison failed\n"); + return -EINVAL; + } + + return 0; +} + +static int tpm2_create_primary(struct tpm_chip *chip, u32 hierarchy, u32 *handle) +{ + int rc; + struct tpm_buf buf; + struct tpm_buf template; + + rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_CREATE_PRIMARY); + if (rc) + return rc; + + rc = tpm_buf_init_2b(&template); + if (rc) { + tpm_buf_destroy(&buf); + return rc; + } + + /* + * create the template. Note: in order for userspace to + * verify the security of the system, it will have to create + * and certify this NULL primary, meaning all the template + * parameters will have to be identical, so conform exactly to + * the TCG TPM v2.0 Provisioning Guidance for the SRK ECC + * key + */ + + /* key type */ + tpm_buf_append_u16(&template, TPM_ALG_ECC); + /* name algorithm */ + tpm_buf_append_u16(&template, TPM_ALG_SHA256); + /* object properties */ + tpm_buf_append_u32(&template, TPM2_OA_NO_DA | + TPM2_OA_FIXED_TPM | + TPM2_OA_FIXED_PARENT | + TPM2_OA_SENSITIVE_DATA_ORIGIN | + TPM2_OA_USER_WITH_AUTH | + TPM2_OA_DECRYPT | + TPM2_OA_RESTRICTED); + /* sauth policy (empty) */ + tpm_buf_append_u16(&template, 0); + + /* BEGIN parameters: key specific; for ECC*/ + /* symmetric algorithm */ + tpm_buf_append_u16(&template, TPM_ALG_AES); + /* bits for symmetric algorithm */ + tpm_buf_append_u16(&template, 128); + /* algorithm mode (must be CFB) */ + tpm_buf_append_u16(&template, TPM_ALG_CFB); + /* scheme (NULL means any scheme) */ + tpm_buf_append_u16(&template, TPM_ALG_NULL); + /* ECC Curve ID */ + tpm_buf_append_u16(&template, TPM2_ECC_NIST_P256); + /* KDF Scheme */ + tpm_buf_append_u16(&template, TPM_ALG_NULL); + /* unique: key specific; for ECC it is two points */ + tpm_buf_append_u16(&template, 0); + tpm_buf_append_u16(&template, 0); + /* END parameters */ + + /* primary handle */ + tpm_buf_append_u32(&buf, hierarchy); + tpm_buf_append_empty_auth(&buf, TPM2_RS_PW); + /* sensitive create size is 4 for two empty buffers */ + tpm_buf_append_u16(&buf, 4); + /* sensitive create auth data (empty) */ + tpm_buf_append_u16(&buf, 0); + /* sensitive create sensitive data (empty) */ + tpm_buf_append_u16(&buf, 0); + /* the public template */ + tpm_buf_append_2b(&buf, &template); + tpm_buf_destroy(&template); + /* outside info (empty) */ + tpm_buf_append_u16(&buf, 0); + /* creation PCR (none) */ + tpm_buf_append_u32(&buf, 0); + + rc = tpm_transmit_cmd(chip, &buf, 0, + "attempting to create NULL primary"); + + if (rc == TPM2_RC_SUCCESS) + rc = parse_create_primary(chip, buf.data, handle); + + tpm_buf_destroy(&buf); + + return rc; +} + +int tpm2_create_null_primary(struct tpm_chip *chip) { + u32 nullkey; + int rc; + + rc = tpm2_create_primary(chip, TPM2_RH_NULL, &nullkey); + + if (rc == TPM2_RC_SUCCESS) { + unsigned int offset = 0; /* dummy offset for tpmkeycontext */ + + rc = tpm2_save_context(chip, nullkey, chip->tpmkeycontext, + sizeof(chip->tpmkeycontext), &offset); + tpm2_flush_context(chip, nullkey); + } + + return rc; +} + +int tpm2_sessions_init(struct tpm_chip *chip) +{ + int rc; + + sha256_hash = crypto_alloc_shash("sha256", 0, 0); + if (!sha256_hash) { + dev_err(&chip->dev, "TPM: failed to allocate hash\n"); + return -ENOMEM; + } + + rc = tpm2_create_null_primary(chip); + if (rc) + dev_err(&chip->dev, "TPM: security failed (NULL seed derivation): %d\n", rc); + return rc; +} +EXPORT_SYMBOL(tpm2_sessions_init); diff --git a/include/linux/tpm.h b/include/linux/tpm.h index fa8d1f932c0f..bdf19538b103 100644 --- a/include/linux/tpm.h +++ b/include/linux/tpm.h @@ -30,17 +30,28 @@ struct tpm_chip; struct trusted_key_payload; struct trusted_key_options; +/* opaque structure, holds auth session parameters like the session key */ +struct tpm2_auth; + +enum tpm2_session_types { + TPM2_SE_HMAC = 0x00, + TPM2_SE_POLICY = 0x01, + TPM2_SE_TRIAL = 0x02, +}; /* if you add a new hash to this, increment TPM_MAX_HASHES below */ enum tpm_algorithms { TPM_ALG_ERROR = 0x0000, TPM_ALG_SHA1 = 0x0004, + TPM_ALG_AES = 0x0006, TPM_ALG_KEYEDHASH = 0x0008, TPM_ALG_SHA256 = 0x000B, TPM_ALG_SHA384 = 0x000C, TPM_ALG_SHA512 = 0x000D, TPM_ALG_NULL = 0x0010, TPM_ALG_SM3_256 = 0x0012, + TPM_ALG_ECC = 0x0023, + TPM_ALG_CFB = 0x0043, }; /* @@ -49,6 +60,11 @@ enum tpm_algorithms { */ #define TPM_MAX_HASHES 5 +enum tpm2_curves { + TPM2_ECC_NONE = 0x0000, + TPM2_ECC_NIST_P256 = 0x0003, +}; + struct tpm_digest { u16 alg_id; u8 digest[TPM_MAX_DIGEST_SIZE]; @@ -116,6 +132,20 @@ struct tpm_chip_seqops { const struct seq_operations *seqops; }; +/* fixed define for the curve we use which is NIST_P256 */ +#define EC_PT_SZ 32 + +/* + * fixed define for the size of a name. This is actually HASHALG size + * plus 2, so 32 for SHA256 + */ +#define TPM2_NAME_SIZE 34 + +/* + * The maximum size for an object context + */ +#define TPM2_MAX_CONTEXT_SIZE 4096 + struct tpm_chip { struct device dev; struct device devs; @@ -170,6 +200,12 @@ struct tpm_chip { /* active locality */ int locality; + + /* details for communication security via sessions */ + u8 tpmkeycontext[TPM2_MAX_CONTEXT_SIZE]; /* context for NULL seed */ + u8 tpmkeyname[TPM2_NAME_SIZE]; /* name of NULL seed */ + u8 ec_point_x[EC_PT_SZ]; + u8 ec_point_y[EC_PT_SZ]; }; #define TPM_HEADER_SIZE 10 @@ -194,6 +230,7 @@ enum tpm2_timeouts { enum tpm2_structures { TPM2_ST_NO_SESSIONS = 0x8001, TPM2_ST_SESSIONS = 0x8002, + TPM2_ST_CREATION = 0x8021, }; /* Indicates from what layer of the software stack the error comes from */ @@ -231,6 +268,10 @@ enum tpm2_command_codes { TPM2_CC_CONTEXT_LOAD = 0x0161, TPM2_CC_CONTEXT_SAVE = 0x0162, TPM2_CC_FLUSH_CONTEXT = 0x0165, + TPM2_CC_POLICY_AUTHVALUE = 0x016B, + TPM2_CC_POLICY_COUNTER_TIMER = 0x016D, + TPM2_CC_READ_PUBLIC = 0x0173, + TPM2_CC_START_AUTH_SESS = 0x0176, TPM2_CC_VERIFY_SIGNATURE = 0x0177, TPM2_CC_GET_CAPABILITY = 0x017A, TPM2_CC_GET_RANDOM = 0x017B, @@ -243,6 +284,7 @@ enum tpm2_command_codes { }; enum tpm2_permanent_handles { + TPM2_RH_NULL = 0x40000007, TPM2_RS_PW = 0x40000009, }; @@ -306,16 +348,30 @@ enum tpm_buf_flags { struct tpm_buf { unsigned int flags; u8 *data; + u8 handles; }; enum tpm2_object_attributes { TPM2_OA_FIXED_TPM = BIT(1), + TPM2_OA_ST_CLEAR = BIT(2), TPM2_OA_FIXED_PARENT = BIT(4), + TPM2_OA_SENSITIVE_DATA_ORIGIN = BIT(5), TPM2_OA_USER_WITH_AUTH = BIT(6), + TPM2_OA_ADMIN_WITH_POLICY = BIT(7), + TPM2_OA_NO_DA = BIT(10), + TPM2_OA_ENCRYPTED_DUPLICATION = BIT(11), + TPM2_OA_RESTRICTED = BIT(16), + TPM2_OA_DECRYPT = BIT(17), + TPM2_OA_SIGN = BIT(18), }; enum tpm2_session_attributes { TPM2_SA_CONTINUE_SESSION = BIT(0), + TPM2_SA_AUDIT_EXCLUSIVE = BIT(1), + TPM2_SA_AUDIT_RESET = BIT(3), + TPM2_SA_DECRYPT = BIT(5), + TPM2_SA_ENCRYPT = BIT(6), + TPM2_SA_AUDIT = BIT(7), }; struct tpm2_hash { @@ -369,6 +425,15 @@ extern int tpm_send(struct tpm_chip *chip, void *cmd, size_t buflen); extern int tpm_get_random(struct tpm_chip *chip, u8 *data, size_t max); extern struct tpm_chip *tpm_default_chip(void); void tpm2_flush_context(struct tpm_chip *chip, u32 handle); +static inline void tpm_buf_append_empty_auth(struct tpm_buf *buf, u32 handle) +{ + /* simple authorization for empty auth */ + tpm_buf_append_u32(buf, 9); /* total length of auth */ + tpm_buf_append_u32(buf, handle); + tpm_buf_append_u16(buf, 0); /* nonce len */ + tpm_buf_append_u8(buf, 0); /* attributes */ + tpm_buf_append_u16(buf, 0); /* hmac len */ +} #else static inline int tpm_is_tpm2(struct tpm_chip *chip) { @@ -399,5 +464,100 @@ static inline struct tpm_chip *tpm_default_chip(void) { return NULL; } + +static inline void tpm_buf_append_empty_auth(struct tpm_buf *buf, u32 handle) +{ +} #endif +#ifdef CONFIG_TPM_BUS_SECURITY + +int tpm2_start_auth_session(struct tpm_chip *chip, struct tpm2_auth **authp); +void tpm_buf_append_name(struct tpm_buf *buf, struct tpm2_auth *auth, + u32 handle, u8 *name); +void tpm_buf_append_hmac_session(struct tpm_buf *buf, struct tpm2_auth *auth, + u8 attributes, u8 *passphrase, + int passphraselen); +static inline void tpm_buf_append_hmac_session_opt(struct tpm_buf *buf, + struct tpm2_auth *auth, + u8 attributes, + u8 *passphrase, + int passphraselen) +{ + tpm_buf_append_hmac_session(buf, auth, attributes, passphrase, + passphraselen); +} +void tpm_buf_fill_hmac_session(struct tpm_buf *buf, struct tpm2_auth *auth); +int tpm_buf_check_hmac_response(struct tpm_buf *buf, struct tpm2_auth *auth, + int rc); +void tpm2_end_auth_session(struct tpm2_auth *auth); +#else +#include <asm/unaligned.h> + +static inline int tpm2_start_auth_session(struct tpm_chip *chip, + struct tpm2_auth **authp) +{ + return 0; +} +static inline void tpm2_end_auth_session(struct tpm2_auth *auth) +{ +} +static inline void tpm_buf_append_name(struct tpm_buf *buf, + struct tpm2_auth *auth, + u32 handle, u8 *name) +{ + tpm_buf_append_u32(buf, handle); + /* count the number of handles in the upper bits of flags */ + buf->handles++; +} +static inline void tpm_buf_append_hmac_session(struct tpm_buf *buf, + struct tpm2_auth *auth, + u8 attributes, u8 *passphrase, + int passphraselen) +{ + /* offset tells us where the sessions area begins */ + int offset = buf->handles * 4 + TPM_HEADER_SIZE; + u32 len = 9 + passphraselen; + if (tpm_buf_length(buf) != offset) { + /* not the first session so update the existing length */ + len += get_unaligned_be32(&buf->data[offset]); + put_unaligned_be32(len, &buf->data[offset]); + } else { + tpm_buf_append_u32(buf, len); + } + /* auth handle */ + tpm_buf_append_u32(buf, TPM2_RS_PW); + /* nonce */ + tpm_buf_append_u16(buf, 0); + /* attributes */ + tpm_buf_append_u8(buf, 0); + /* passphrase */ + tpm_buf_append_u16(buf, passphraselen); + tpm_buf_append(buf, passphrase, passphraselen); +} +static inline void tpm_buf_append_hmac_session_opt(struct tpm_buf *buf, + struct tpm2_auth *auth, + u8 attributes, + u8 *passphrase, + int passphraselen) +{ + int offset = buf->handles * 4 + TPM_HEADER_SIZE; + struct tpm_header *head = (struct tpm_header *) buf->data; + + /* if the only sessions are optional, the command tag + * must change to TPM2_ST_NO_SESSIONS */ + if (tpm_buf_length(buf) == offset) + head->tag = cpu_to_be16(TPM2_ST_NO_SESSIONS); +} +static inline void tpm_buf_fill_hmac_session(struct tpm_buf *buf, + struct tpm2_auth *auth) +{ +} +static inline int tpm_buf_check_hmac_response(struct tpm_buf *buf, + struct tpm2_auth *auth, + int rc) +{ + return rc; +} +#endif /* CONFIG_TPM_BUS_SECURITY */ + #endif