linux-kernel - Re: [PATCH V8 1/5] crypto: Multi-buffer encryption infrastructure support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 18 Jan 2018 22:39:06 +1100
From:   Herbert Xu <herbert@...dor.apana.org.au>
To:     Megha Dey <megha.dey@...ux.intel.com>
Cc:     linux-kernel@...r.kernel.org, linux-crypto@...r.kernel.org,
        davem@...emloft.net, megha.dey@...el.com
Subject: Re: [PATCH V8 1/5] crypto: Multi-buffer encryption infrastructure
 support

On Tue, Jan 09, 2018 at 04:09:04PM -0800, Megha Dey wrote:
>
> +static void mcryptd_skcipher_encrypt(struct crypto_async_request *base,
> +								int err)
> +{
> +	struct skcipher_request *req = skcipher_request_cast(base);
> +	struct mcryptd_skcipher_request_ctx *rctx = skcipher_request_ctx(req);
> +	struct crypto_skcipher *tfm = crypto_skcipher_reqtfm(req);
> +	struct mcryptd_skcipher_ctx *ctx = crypto_skcipher_ctx(tfm);
> +	struct crypto_skcipher *child = ctx->child;
> +	struct skcipher_request subreq;
> +
> +	if (unlikely(err == -EINPROGRESS))
> +		goto out;
> +
> +	/* set up the skcipher request to work on */
> +	skcipher_request_set_tfm(&subreq, child);
> +	skcipher_request_set_callback(&subreq,
> +					CRYPTO_TFM_REQ_MAY_SLEEP, 0, 0);
> +	skcipher_request_set_crypt(&subreq, req->src, req->dst,
> +					req->cryptlen, req->iv);
> +
> +	/*
> +	 * pass addr of descriptor stored in the request context
> +	 * so that the callee can get to the request context
> +	 */
> +	rctx->desc = subreq;
> +	err = crypto_skcipher_encrypt(&rctx->desc);
> +
> +	if (err) {
> +		req->base.complete = rctx->complete;
> +		goto out;
> +	}
> +	return;
> +
> +out:
> +	mcryptd_skcipher_complete(req, err);
> +}

OK this looks better but it's still abusing the crypto API interface.
In particular, you're sharing data with the underlying algorithm
behind the crypto API's back.  Also, the underlying algorithm does
callback completion behind the API's back through the shared data
context.

It seems to me that the current mcryptd scheme is flawed.  You
want to batch multiple requests and yet this isn't actually being
done by mcryptd at all.  The actual batching happens at the very
lowest level, i.e., in the crypto algorithm below mcryptd.  For
example, with your patch, the batching appears to happen in
aes_cbc_job_mgr_submit.

So the mcryptd template is in fact completely superfluous.  You
can remove it and just have all the main encrypt/decrypt functions
invoke the underlying encrypt/decrypt function directly and achieve
the same result.

Am I missing something?

Cheers,
-- 
Email: Herbert Xu <herbert@...dor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt