On Sun, Nov 13, 2016 at 06:10:29PM -0800, Eric Biggers wrote: > > There's duplicated code for encryption and decryption here. AFAICS, the only > difference between XTS encryption and decryption is whether the block cipher > is > used in encryption or decryption mode for the ECB step. So I suggest storing > a > function pointer in 'struct rctx' to either crypto_skcipher_encrypt or > crypto_skcipher_decrypt, then calling through it for the ECB step. Then this > code can be shared. In other words I'd like the top-level functions to look > like this:
I'm all for getting rid of duplicate code. However, I'd also prefer to do it without using indirect function calls in this case. > I'm also wondering about the line which does > 'subreq->base.flags &= CRYPTO_TFM_REQ_MAY_BACKLOG;'. > Is the real intent of that to clear the CRYPTO_TFM_REQ_MAY_SLEEP flag because > the completion callback may be called in an atomic context, and if so > shouldn't > it just clear that flag only, rather than all flags except > CRYPTO_TFM_REQ_MAY_BACKLOG? The intent here is that this is the only flag that we want to pass along. Of course in the absence of any other flags it's a moot point. > > + if (req->cryptlen > XTS_BUFFER_SIZE) { > > + subreq->cryptlen = min(req->cryptlen, (unsigned)PAGE_SIZE); > > + rctx->ext = kmalloc(subreq->cryptlen, gfp); > > + } > > There's no check for failure to allocate the 'rctx->ext' memory. The code is supposed to handle a NULL rctx->ext gracefully. Did you find a spot where it is used without checking? > > + if (snprintf(inst->alg.base.cra_name, CRYPTO_MAX_ALG_NAME, > > + "xts(%s)", ctx->name) >= CRYPTO_MAX_ALG_NAME) > > + return -ENAMETOOLONG; > > + } else > > + goto err_drop_spawn; > > There should be a line which sets 'err = -EINVAL' before here. Indeed. Fixed here as well as in lrw. > > +static int init_crypt(struct skcipher_request *req, crypto_completion_t > > done) > > { > > - struct priv *ctx = crypto_blkcipher_ctx(desc->tfm); > > - struct blkcipher_walk w; > > + struct priv *ctx = crypto_skcipher_ctx(crypto_skcipher_reqtfm(req)); > > + struct rctx *rctx = skcipher_request_ctx(req); > > + struct skcipher_request *subreq; > > + be128 *buf; > ... > > + /* calculate first value of T */ > > + buf = rctx->ext ?: rctx->buf; > > + crypto_cipher_encrypt_one(ctx->tweak, (u8 *)&rctx->t, req->iv); > > + > > + return 0; > > The 'buf' variable is assigned to but never used. OK, deleted. > > int xts_crypt(struct blkcipher_desc *desc, struct scatterlist *sdst, > > @@ -233,112 +416,167 @@ int xts_crypt(struct blkcipher_desc *desc, struct > > scatterlist *sdst, > > } > > EXPORT_SYMBOL_GPL(xts_crypt); > > xts_crypt() is still here. Is there a plan for its removal, since now the > generic XTS algorithm can use an accelerated ECB algorithm? It will be removed once all the arch code that uses it are converted. Thanks, -- Email: Herbert Xu <herb...@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html