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 <[email protected]>
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
