Re: [PATCH v8 3/4] crypto: AF_ALG -- add asymmetric cipher

[Stephan Müller]

Am Freitag, 11. August 2017, 14:51:10 CEST schrieb Tudor Ambarus:

Hi Tudor,

I have covered all your requests
> 
> > +   size_t used = 0;
> 
> initialization to zero not needed. You can directly initialize to
> ctx->used or don't initialize at all.

It is not initialized now. We cannot use ctx->used here as the socket (and 
thus the ctx data structure) is not locked yet.

> > +
> > +   /*
> > +* This error covers -EIOCBQUEUED which implies that we can
> > +* only handle one AIO request. If the caller wants to have
> > +* multiple AIO requests in parallel, he must make multiple
> > +* separate AIO calls.
> > +*/
> > +   if (err <= 0) {
> 
> why the equal?

We must get something out of the cipher operation as otherwise something is 
wrong. In this case I would like to error out to prevent an endless loop here.

> > +static int akcipher_setprivkey(void *private, const u8 *key,
> > +  unsigned int keylen)
> > +{
> > +   struct akcipher_tfm *tfm = private;
> > +   struct crypto_akcipher *akcipher = tfm->akcipher;
> > +   int err;
> > +
> > +   err = crypto_akcipher_set_priv_key(akcipher, key, keylen);
> > +   tfm->has_key = !err;
> > +
> > +   /* Return the maximum size of the akcipher operation. */
> > +   if (!err)
> > +   err = crypto_akcipher_maxsize(akcipher);
> 
> crypto subsystem returns zero when setkey is successful and introduces
> a new function for determining the maxsize. Should we comply with that?

The idea is that only when the the setting of the priv key fails, it returns 
the size of the expected privkey.

Which new function are you referring to?

Ciao
Stephan

Re: [dm-devel] [PATCH v5 12/19] dm: move dm-verity to generic async completion

[Mikulas Patocka]

On Mon, 14 Aug 2017, Gilad Ben-Yossef wrote:

> dm-verity is starting async. crypto ops and waiting for them to complete.
> Move it over to generic code doing the same.
> 
> This also fixes a possible data coruption bug created by the
> use of wait_for_completion_interruptible() without dealing
> correctly with an interrupt aborting the wait prior to the
> async op finishing.

What is the exact problem there? The interruptible sleep is called from a 
workqueue and workqueues have all signals blocked. Are signals unblocked 
for some reason there?

Should there be another patch for stable kernels that fixes the 
interruptible sleep?

Mikulas

> Signed-off-by: Gilad Ben-Yossef 
> ---
>  drivers/md/dm-verity-target.c | 81 
> +++
>  drivers/md/dm-verity.h|  5 ---
>  2 files changed, 20 insertions(+), 66 deletions(-)
> 
> diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
> index 79f18d4..8df08a8 100644
> --- a/drivers/md/dm-verity-target.c
> +++ b/drivers/md/dm-verity-target.c
> @@ -92,74 +92,33 @@ static sector_t verity_position_at_level(struct dm_verity 
> *v, sector_t block,
>   return block >> (level * v->hash_per_block_bits);
>  }
>  
> -/*
> - * Callback function for asynchrnous crypto API completion notification
> - */
> -static void verity_op_done(struct crypto_async_request *base, int err)
> -{
> - struct verity_result *res = (struct verity_result *)base->data;
> -
> - if (err == -EINPROGRESS)
> - return;
> -
> - res->err = err;
> - complete(&res->completion);
> -}
> -
> -/*
> - * Wait for async crypto API callback
> - */
> -static inline int verity_complete_op(struct verity_result *res, int ret)
> -{
> - switch (ret) {
> - case 0:
> - break;
> -
> - case -EINPROGRESS:
> - case -EBUSY:
> - ret = wait_for_completion_interruptible(&res->completion);
> - if (!ret)
> - ret = res->err;
> - reinit_completion(&res->completion);
> - break;
> -
> - default:
> - DMERR("verity_wait_hash: crypto op submission failed: %d", ret);
> - }
> -
> - if (unlikely(ret < 0))
> - DMERR("verity_wait_hash: crypto op failed: %d", ret);
> -
> - return ret;
> -}
> -
>  static int verity_hash_update(struct dm_verity *v, struct ahash_request *req,
>   const u8 *data, size_t len,
> - struct verity_result *res)
> + struct crypto_wait *wait)
>  {
>   struct scatterlist sg;
>  
>   sg_init_one(&sg, data, len);
>   ahash_request_set_crypt(req, &sg, NULL, len);
>  
> - return verity_complete_op(res, crypto_ahash_update(req));
> + return crypto_wait_req(crypto_ahash_update(req), wait);
>  }
>  
>  /*
>   * Wrapper for crypto_ahash_init, which handles verity salting.
>   */
>  static int verity_hash_init(struct dm_verity *v, struct ahash_request *req,
> - struct verity_result *res)
> + struct crypto_wait *wait)
>  {
>   int r;
>  
>   ahash_request_set_tfm(req, v->tfm);
>   ahash_request_set_callback(req, CRYPTO_TFM_REQ_MAY_SLEEP |
>   CRYPTO_TFM_REQ_MAY_BACKLOG,
> - verity_op_done, (void *)res);
> - init_completion(&res->completion);
> + crypto_req_done, (void *)wait);
> + crypto_init_wait(wait);
>  
> - r = verity_complete_op(res, crypto_ahash_init(req));
> + r = crypto_wait_req(crypto_ahash_init(req), wait);
>  
>   if (unlikely(r < 0)) {
>   DMERR("crypto_ahash_init failed: %d", r);
> @@ -167,18 +126,18 @@ static int verity_hash_init(struct dm_verity *v, struct 
> ahash_request *req,
>   }
>  
>   if (likely(v->salt_size && (v->version >= 1)))
> - r = verity_hash_update(v, req, v->salt, v->salt_size, res);
> + r = verity_hash_update(v, req, v->salt, v->salt_size, wait);
>  
>   return r;
>  }
>  
>  static int verity_hash_final(struct dm_verity *v, struct ahash_request *req,
> -  u8 *digest, struct verity_result *res)
> +  u8 *digest, struct crypto_wait *wait)
>  {
>   int r;
>  
>   if (unlikely(v->salt_size && (!v->version))) {
> - r = verity_hash_update(v, req, v->salt, v->salt_size, res);
> + r = verity_hash_update(v, req, v->salt, v->salt_size, wait);
>  
>   if (r < 0) {
>   DMERR("verity_hash_final failed updating salt: %d", r);
> @@ -187,7 +146,7 @@ static int verity_hash_final(struct dm_verity *v, struct 
> ahash_request *req,
>   }
>  
>   ahash_request_set_crypt(req, NULL, digest, 0);
> - r = verity_complete_op(res, crypto_ahash_final(req));
> + r = crypto_wait_req(crypto_ahash_final(req), wait);
>  out:
>   return r;