Re: [PATCH v3 1/6] drivers: crypto: qce: sha: Restore/save ahash state with custom struct in export/import
Hi Thara, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on cryptodev/master] [also build test WARNING on crypto/master v5.11-rc6 next-20210125] [cannot apply to sparc-next/master] [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] url: https://github.com/0day-ci/linux/commits/Thara-Gopinath/Regression-fixes-clean-ups-in-the-Qualcomm-crypto-engine-driver/20210121-032302 base: https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git master config: arm64-randconfig-r024-20210202 (attached as .config) compiler: aarch64-linux-gcc (GCC) 9.3.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/0day-ci/linux/commit/b282823110c3b59ae881393d33df0b0e7e0eb90b git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Thara-Gopinath/Regression-fixes-clean-ups-in-the-Qualcomm-crypto-engine-driver/20210121-032302 git checkout b282823110c3b59ae881393d33df0b0e7e0eb90b # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arm64 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot All warnings (new ones prefixed by >>): drivers/crypto/qce/sha.c: In function 'qce_ahash_import': >> drivers/crypto/qce/sha.c:166:45: warning: initialization discards 'const' >> qualifier from pointer target type [-Wdiscarded-qualifiers] 166 | struct qce_sha_saved_state *import_state = in; | ^~ vim +/const +166 drivers/crypto/qce/sha.c 162 163 static int qce_ahash_import(struct ahash_request *req, const void *in) 164 { 165 struct qce_sha_reqctx *rctx = ahash_request_ctx(req); > 166 struct qce_sha_saved_state *import_state = in; 167 168 memset(rctx, 0, sizeof(*rctx)); 169 rctx->count = import_state->count; 170 rctx->buflen = import_state->pending_buflen; 171 rctx->first_blk = import_state->first_blk; 172 rctx->flags = import_state->flags; 173 memcpy(rctx->buf, import_state->pending_buf, rctx->buflen); 174 memcpy(rctx->digest, import_state->partial_digest, 175 sizeof(rctx->digest)); 176 memcpy(rctx->byte_count, import_state->byte_count, 2); 177 178 return 0; 179 } 180 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org .config.gz Description: application/gzip
Re: [PATCH v3 1/6] drivers: crypto: qce: sha: Restore/save ahash state with custom struct in export/import
On Wed 20 Jan 12:48 CST 2021, Thara Gopinath wrote: Please drop "drivers: " from $subject. > Export and import interfaces save and restore partial transformation > states. The partial states were being stored and restored in struct > sha1_state for sha1/hmac(sha1) transformations and sha256_state for > sha256/hmac(sha256) transformations.This led to a bunch of corner cases > where improper state was being stored and restored. A few of the corner > cases that turned up during testing are: > > - wrong byte_count restored if export/import is called twice without h/w > transaction in between > - wrong buflen restored back if the pending buffer > length is exactly the block size. > - wrong state restored if buffer length is 0. > > To fix these issues, save and restore the partial transformation state > using the newly introduced qce_sha_saved_state struct. This ensures that > all the pieces required to properly restart the transformation is captured > and restored back > > Signed-off-by: Thara Gopinath > --- > > v1->v2: > - Introduced custom struct qce_sha_saved_state to store and > restore partial sha transformation. v1 was re-using > qce_sha_reqctx to save and restore partial states and this > could lead to potential memcpy issues around pointer copying. > > drivers/crypto/qce/sha.c | 122 +++ > 1 file changed, 34 insertions(+), 88 deletions(-) > > diff --git a/drivers/crypto/qce/sha.c b/drivers/crypto/qce/sha.c > index 61c418c12345..08aed03e2b59 100644 > --- a/drivers/crypto/qce/sha.c > +++ b/drivers/crypto/qce/sha.c > @@ -12,9 +12,15 @@ > #include "core.h" > #include "sha.h" > > -/* crypto hw padding constant for first operation */ > -#define SHA_PADDING 64 > -#define SHA_PADDING_MASK (SHA_PADDING - 1) > +struct qce_sha_saved_state { > + u8 pending_buf[QCE_SHA_MAX_BLOCKSIZE]; > + u8 partial_digest[QCE_SHA_MAX_DIGESTSIZE]; > + __be32 byte_count[2]; > + unsigned int pending_buflen; > + unsigned int flags; > + u64 count; > + bool first_blk; > +}; > > static LIST_HEAD(ahash_algs); > > @@ -139,97 +145,37 @@ static int qce_ahash_init(struct ahash_request *req) > > static int qce_ahash_export(struct ahash_request *req, void *out) > { > - struct crypto_ahash *ahash = crypto_ahash_reqtfm(req); > struct qce_sha_reqctx *rctx = ahash_request_ctx(req); > - unsigned long flags = rctx->flags; > - unsigned int digestsize = crypto_ahash_digestsize(ahash); > - unsigned int blocksize = > - crypto_tfm_alg_blocksize(crypto_ahash_tfm(ahash)); > - > - if (IS_SHA1(flags) || IS_SHA1_HMAC(flags)) { > - struct sha1_state *out_state = out; > - > - out_state->count = rctx->count; > - qce_cpu_to_be32p_array((__be32 *)out_state->state, > -rctx->digest, digestsize); > - memcpy(out_state->buffer, rctx->buf, blocksize); > - } else if (IS_SHA256(flags) || IS_SHA256_HMAC(flags)) { > - struct sha256_state *out_state = out; > - > - out_state->count = rctx->count; > - qce_cpu_to_be32p_array((__be32 *)out_state->state, > -rctx->digest, digestsize); > - memcpy(out_state->buf, rctx->buf, blocksize); > - } else { > - return -EINVAL; > - } > - > - return 0; > -} > - > -static int qce_import_common(struct ahash_request *req, u64 in_count, > - const u32 *state, const u8 *buffer, bool hmac) > -{ > - struct crypto_ahash *ahash = crypto_ahash_reqtfm(req); > - struct qce_sha_reqctx *rctx = ahash_request_ctx(req); > - unsigned int digestsize = crypto_ahash_digestsize(ahash); > - unsigned int blocksize; > - u64 count = in_count; > - > - blocksize = crypto_tfm_alg_blocksize(crypto_ahash_tfm(ahash)); > - rctx->count = in_count; > - memcpy(rctx->buf, buffer, blocksize); > - > - if (in_count <= blocksize) { > - rctx->first_blk = 1; > - } else { > - rctx->first_blk = 0; > - /* > - * For HMAC, there is a hardware padding done when first block > - * is set. Therefore the byte_count must be incremened by 64 > - * after the first block operation. > - */ > - if (hmac) > - count += SHA_PADDING; > - } > + struct qce_sha_saved_state *export_state = out; > > - rctx->byte_count[0] = (__force __be32)(count & ~SHA_PADDING_MASK); > - rctx->byte_count[1] = (__force __be32)(count >> 32); > - qce_cpu_to_be32p_array((__be32 *)rctx->digest, (const u8 *)state, > -digestsize); > - rctx->buflen = (unsigned int)(in_count & (blocksize - 1)); > + memcpy(export_state->pending_buf, rctx->buf, rctx->buflen); > + memcpy(export_state->partial_digest, rctx->digest, > +sizeo
[PATCH v3 1/6] drivers: crypto: qce: sha: Restore/save ahash state with custom struct in export/import
Export and import interfaces save and restore partial transformation states. The partial states were being stored and restored in struct sha1_state for sha1/hmac(sha1) transformations and sha256_state for sha256/hmac(sha256) transformations.This led to a bunch of corner cases where improper state was being stored and restored. A few of the corner cases that turned up during testing are: - wrong byte_count restored if export/import is called twice without h/w transaction in between - wrong buflen restored back if the pending buffer length is exactly the block size. - wrong state restored if buffer length is 0. To fix these issues, save and restore the partial transformation state using the newly introduced qce_sha_saved_state struct. This ensures that all the pieces required to properly restart the transformation is captured and restored back Signed-off-by: Thara Gopinath --- v1->v2: - Introduced custom struct qce_sha_saved_state to store and restore partial sha transformation. v1 was re-using qce_sha_reqctx to save and restore partial states and this could lead to potential memcpy issues around pointer copying. drivers/crypto/qce/sha.c | 122 +++ 1 file changed, 34 insertions(+), 88 deletions(-) diff --git a/drivers/crypto/qce/sha.c b/drivers/crypto/qce/sha.c index 61c418c12345..08aed03e2b59 100644 --- a/drivers/crypto/qce/sha.c +++ b/drivers/crypto/qce/sha.c @@ -12,9 +12,15 @@ #include "core.h" #include "sha.h" -/* crypto hw padding constant for first operation */ -#define SHA_PADDING64 -#define SHA_PADDING_MASK (SHA_PADDING - 1) +struct qce_sha_saved_state { + u8 pending_buf[QCE_SHA_MAX_BLOCKSIZE]; + u8 partial_digest[QCE_SHA_MAX_DIGESTSIZE]; + __be32 byte_count[2]; + unsigned int pending_buflen; + unsigned int flags; + u64 count; + bool first_blk; +}; static LIST_HEAD(ahash_algs); @@ -139,97 +145,37 @@ static int qce_ahash_init(struct ahash_request *req) static int qce_ahash_export(struct ahash_request *req, void *out) { - struct crypto_ahash *ahash = crypto_ahash_reqtfm(req); struct qce_sha_reqctx *rctx = ahash_request_ctx(req); - unsigned long flags = rctx->flags; - unsigned int digestsize = crypto_ahash_digestsize(ahash); - unsigned int blocksize = - crypto_tfm_alg_blocksize(crypto_ahash_tfm(ahash)); - - if (IS_SHA1(flags) || IS_SHA1_HMAC(flags)) { - struct sha1_state *out_state = out; - - out_state->count = rctx->count; - qce_cpu_to_be32p_array((__be32 *)out_state->state, - rctx->digest, digestsize); - memcpy(out_state->buffer, rctx->buf, blocksize); - } else if (IS_SHA256(flags) || IS_SHA256_HMAC(flags)) { - struct sha256_state *out_state = out; - - out_state->count = rctx->count; - qce_cpu_to_be32p_array((__be32 *)out_state->state, - rctx->digest, digestsize); - memcpy(out_state->buf, rctx->buf, blocksize); - } else { - return -EINVAL; - } - - return 0; -} - -static int qce_import_common(struct ahash_request *req, u64 in_count, -const u32 *state, const u8 *buffer, bool hmac) -{ - struct crypto_ahash *ahash = crypto_ahash_reqtfm(req); - struct qce_sha_reqctx *rctx = ahash_request_ctx(req); - unsigned int digestsize = crypto_ahash_digestsize(ahash); - unsigned int blocksize; - u64 count = in_count; - - blocksize = crypto_tfm_alg_blocksize(crypto_ahash_tfm(ahash)); - rctx->count = in_count; - memcpy(rctx->buf, buffer, blocksize); - - if (in_count <= blocksize) { - rctx->first_blk = 1; - } else { - rctx->first_blk = 0; - /* -* For HMAC, there is a hardware padding done when first block -* is set. Therefore the byte_count must be incremened by 64 -* after the first block operation. -*/ - if (hmac) - count += SHA_PADDING; - } + struct qce_sha_saved_state *export_state = out; - rctx->byte_count[0] = (__force __be32)(count & ~SHA_PADDING_MASK); - rctx->byte_count[1] = (__force __be32)(count >> 32); - qce_cpu_to_be32p_array((__be32 *)rctx->digest, (const u8 *)state, - digestsize); - rctx->buflen = (unsigned int)(in_count & (blocksize - 1)); + memcpy(export_state->pending_buf, rctx->buf, rctx->buflen); + memcpy(export_state->partial_digest, rctx->digest, + sizeof(rctx->digest)); + memcpy(export_state->byte_count, rctx->byte_count, 2); + export_state->pending_buflen = rctx->buflen; + export_state->count = rctx->count; + export_state->firs