On Tue, 14 Feb 2023 at 15:28, James Bottomley <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx> wrote: > > On Tue, 2023-02-14 at 14:54 +0100, Ard Biesheuvel wrote: > > On Mon, 13 Feb 2023 at 08:45, Jarkko Sakkinen <jarkko@xxxxxxxxxx> > > 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@xxxxxxxxx/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@xxxxxxxxx> > > > > > > > > > > > > > > > > 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 >
