Am Dienstag, 30. Januar 2018, 16:29:52 CET schrieb Jonathan Cameron:

Hi Jonathan,

> +     /* Special path for single element SGLs with small packets. */
> +     if (sg_is_last(sgl) && sgl->length <= SEC_SMALL_PACKET_SIZE) {

This looks strangely familiar. Is this code affected by a similar issue fixed 
in https://patchwork.kernel.org/patch/10173981?

> +static int sec_alg_skcipher_setkey(struct crypto_skcipher *tfm,
> +                                  const u8 *key, unsigned int keylen,
> +                                  enum sec_cipher_alg alg)
> +{
> +       struct sec_alg_skcipher_ctx *ctx = crypto_skcipher_ctx(tfm);
> +       struct device *dev;
> +
> +       spin_lock(&ctx->lock);
> +       if (ctx->enc_key) {
> +               /* rekeying */
> +               dev = SEC_Q_DEV(ctx->queue);
> +               memset(ctx->enc_key, 0, SEC_MAX_CIPHER_KEY);
> +               memset(ctx->dec_key, 0, SEC_MAX_CIPHER_KEY);
> +               memset(&ctx->enc_req, 0, sizeof(ctx->enc_req));
> +               memset(&ctx->dec_req, 0, sizeof(ctx->dec_req));
> +       } else {
> +               /* new key */
> +               dev = SEC_Q_DEV(ctx->queue);
> +               ctx->enc_key = dma_zalloc_coherent(dev, SEC_MAX_CIPHER_KEY,
> +                                                  &ctx->enc_pkey,
> GFP_ATOMIC); +               if (!ctx->enc_key) {
> +                       spin_unlock(&ctx->lock);
> +                       return -ENOMEM;
> +               }
> +               ctx->dec_key = dma_zalloc_coherent(dev, SEC_MAX_CIPHER_KEY,
> +                                                  &ctx->dec_pkey,
> GFP_ATOMIC); +               if (!ctx->dec_key) {
> +                       spin_unlock(&ctx->lock);
> +                       goto out_free_enc;
> +               }
> +       }
> +       spin_unlock(&ctx->lock);
> +       if (sec_alg_skcipher_init_context(tfm, key, keylen, alg))
> +               goto out_free_all;
> +
> +       return 0;
> +
> +out_free_all:
> +       memset(ctx->dec_key, 0, SEC_MAX_CIPHER_KEY);
> +       dma_free_coherent(dev, SEC_MAX_CIPHER_KEY,
> +                         ctx->dec_key, ctx->dec_pkey);
> +       ctx->dec_key = NULL;
> +out_free_enc:
> +       memset(ctx->enc_key, 0, SEC_MAX_CIPHER_KEY);
> +       dma_free_coherent(dev, SEC_MAX_CIPHER_KEY,
> +                         ctx->enc_key, ctx->enc_pkey);
> +       ctx->enc_key = NULL;

Please use memzero_explicit.
> +
> +       return -ENOMEM;
> +}

> +static int sec_alg_skcipher_setkey_aes_xts(struct crypto_skcipher *tfm,
> +                                          const u8 *key, unsigned int
> keylen) +{
> +       enum sec_cipher_alg alg;
> +
> +       switch (keylen) {
> +       case AES_KEYSIZE_128 * 2:
> +               alg = SEC_AES_XTS_128;
> +               break;
> +       case AES_KEYSIZE_256 * 2:
> +               alg = SEC_AES_XTS_256;
> +               break;
> +       default:
> +               return -EINVAL;
> +       }

Please add xts_check_key()
> +
> +       return sec_alg_skcipher_setkey(tfm, key, keylen, alg);

> +static int sec_alg_skcipher_setkey_3des_ecb(struct crypto_skcipher *tfm,
> +                                           const u8 *key, unsigned int
> keylen) +{
> +       if (keylen != DES_KEY_SIZE * 3)
> +               return -EINVAL;
> +
> +       return sec_alg_skcipher_setkey(tfm, key, keylen,
> SEC_3DES_ECB_192_3KEY); +}
> +
> +static int sec_alg_skcipher_setkey_3des_cbc(struct crypto_skcipher *tfm,
> +                                           const u8 *key, unsigned int
> keylen) +{
> +       if (keylen != DES3_EDE_KEY_SIZE)
> +               return -EINVAL;
> +
> +       return sec_alg_skcipher_setkey(tfm, key, keylen,
> SEC_3DES_CBC_192_3KEY); +}

Please use __des3_ede_setkey

> +static void sec_skcipher_alg_callback(struct sec_bd_info *sec_resp,
> +                                   struct skcipher_request *skreq)
> +{
> +     struct sec_crypto_request *sec_req = skcipher_request_ctx(skreq);
> +     struct sec_alg_skcipher_ctx *ctx = sec_req->skcipher_ctx;
> +     struct crypto_skcipher *atfm = crypto_skcipher_reqtfm(skreq);
> +     struct crypto_async_request *nextrequest;
> +     struct sec_crypto_request *nextsec_req;
> +     struct skcipher_request *nextskreq;
> +     int icv_or_skey_en;
> +     int err = 0;
> +
> +     icv_or_skey_en = (sec_resp->w0 & SEC_BD_W0_ICV_OR_SKEY_EN_M) >>
> +             SEC_BD_W0_ICV_OR_SKEY_EN_S;
> +     if (sec_resp->w1 & SEC_BD_W1_BD_INVALID || icv_or_skey_en == 3) {
> +             dev_err(ctx->queue->dev_info->dev,
> +                     "Got an invalid answer %lu %d\n",
> +                     sec_resp->w1 & SEC_BD_W1_BD_INVALID,
> +                     icv_or_skey_en);
> +             err = -EINVAL;
> +             /*
> +              * We need to muddle on to avoid getting stuck with elements
> +              * on the queue. Error will be reported so userspace should
> +              * know a mess has occurred.
> +              */
> +     }
> +
> +     spin_lock(&ctx->queue->queuelock);
> +     sec_free_opdata(ctx->queue, skreq->src, skreq->dst, skreq);
> +     /* Put the IV in place for chained cases */
> +     switch (ctx->cipher_alg) {
> +     case SEC_AES_CBC_128:
> +     case SEC_AES_CBC_192:
> +     case SEC_AES_CBC_256:
> +             if (sec_req->req.w0 & SEC_BD_W0_DE)
> +                     sg_pcopy_to_buffer(skreq->dst, sec_req->len_out,
> +                                        skreq->iv,
> +                                        crypto_skcipher_ivsize(atfm),
> +                                        skreq->cryptlen -
> +                                        crypto_skcipher_ivsize(atfm));
> +             else
> +                     sg_pcopy_to_buffer(skreq->src, sec_req->len_in,
> +                                        skreq->iv,
> +                                        crypto_skcipher_ivsize(atfm),
> +                                        skreq->cryptlen - 16);
> +             break;
> +     case SEC_AES_CTR_128:
> +     case SEC_AES_CTR_192:
> +     case SEC_AES_CTR_256:
> +             crypto_inc(skreq->iv, 16);
> +             break;
> +     default:
> +             /* Do not update */
> +             break;
> +     }
> +
> +     if (ctx->queue->havesoftqueue &&
> +         !list_empty(&ctx->queue->softqueue.list) &&
> +         sec_queue_empty(ctx->queue)) {
> +             nextrequest = crypto_dequeue_request(&ctx->queue->softqueue);
> +             nextskreq = container_of(nextrequest, struct skcipher_request,
> +                                      base);
> +             nextsec_req = skcipher_request_ctx(nextskreq);
> +             /* We know there is space so this cannot fail */
> +             sec_queue_send(ctx->queue, &nextsec_req->req, nextskreq);


Looking at that code and considering what you said that only for CTR and CBC 
you need to apply proper IV dependency handling, I am wondering why XTS is not 
covered (there is an IV). What about the DES/3DES ciphers?

> +     /*
> +      * Add to hardware queue only under following circumstances
> +      * 1) Software and hardware queue empty so no chain dependencies
> +      * 2) No dependencies as new IV - (check software queue empty
> +      *    to maintain order)
> +      * 3) No dependencies because the mode does no chaining.
> +      *
> +      * In other cases first insert onto the software queue which is then
> +      * emptied as requests complete
> +      */
> +     if (!ctx->queue->havesoftqueue ||
> +         (list_empty(&ctx->queue->softqueue.list) &&
> +          (sec_queue_empty(ctx->queue) ||
> +           ctx->prev_iv_address != skreq->iv))) {

Maybe you want to rely on the flag given by the upper layer that I will 
propose shortly.

> +
> +static void sec_alg_skcipher_exit(struct crypto_skcipher *tfm)
> +{
> +     struct sec_alg_skcipher_ctx *ctx = crypto_skcipher_ctx(tfm);
> +     struct device *dev = SEC_Q_DEV(ctx->queue);
> +
> +     if (ctx->enc_key) {
> +             memset(ctx->enc_key, 0, SEC_MAX_CIPHER_KEY);
> +             dma_free_coherent(dev, SEC_MAX_CIPHER_KEY,
> +                               ctx->enc_key, ctx->enc_pkey);
> +     }
> +     if (ctx->dec_key) {
> +             memset(ctx->dec_key, 0, SEC_MAX_CIPHER_KEY);
> +             dma_free_coherent(dev, SEC_MAX_CIPHER_KEY,
> +                               ctx->dec_key, ctx->dec_pkey);

Please use memzero_explicit.

> +
> +     /* Get the first idle queue in SEC device */
> +     for (i = 0; i < SEC_Q_NUM; i++)

I think you should use curly braces here too. Consider what checkpatch.pl 
thinks.

> +             if (!sec_queue_in_use_get(&info->queues[i])) {
> +                     sec_queue_in_use(&info->queues[i], 1);
> +                     spin_unlock(&info->dev_lock);
> +                     *q = &info->queues[i];
> +                     return 0;
> +             }
> +     spin_unlock(&info->dev_lock);
> +
> +     return -ENODEV;
> +}
> +
> +static int sec_count_queues_in_use(struct sec_dev_info *info)
> +{
> +     int i, count = 0;
> +
> +     spin_lock(&info->dev_lock);
> +     for (i = 0; i < SEC_Q_NUM; i++)

dto.

Ciao
Stephan

Reply via email to