Re: Bad error handling in cryptodev_lkcf_execute_task(), need advice

[zhenwei pi <pizhenwei@bytedance.com>]

On 3/12/25 16:10, Markus Armbruster wrote:
> scripts/coccinelle/error-use-after-free.cocci led me to this function:
> 
>      static void cryptodev_lkcf_execute_task(CryptoDevLKCFTask *task)
>      {
>          CryptoDevBackendLKCFSession *session = task->sess;
>          CryptoDevBackendAsymOpInfo *asym_op_info;
>          bool kick = false;
>          int ret, status, op_code = task->op_info->op_code;
>          size_t p8info_len;
>          g_autofree uint8_t *p8info = NULL;
>          Error *local_error = NULL;
>          key_serial_t key_id = INVALID_KEY_ID;
>          char op_desc[64];
>          g_autoptr(QCryptoAkCipher) akcipher = NULL;
> 
>          /**
>           * We only offload private key session:
>           * 1. currently, the Linux kernel can only accept public key wrapped
>           * with X.509 certificates, but unfortunately the cost of making a
>           * ceritificate with public key is too expensive.
>           * 2. generally, public key related compution is fast, just compute it with
>           * thread-pool.
>           */
>          if (session->keytype == QCRYPTO_AK_CIPHER_KEY_TYPE_PRIVATE) {
>              if (qcrypto_akcipher_export_p8info(&session->akcipher_opts,
>                                                 session->key, session->keylen,
>                                                 &p8info, &p8info_len,
>                                                 &local_error) != 0 ||
>                  cryptodev_lkcf_set_op_desc(&session->akcipher_opts, op_desc,
>                                             sizeof(op_desc), &local_error) != 0) {
>                  error_report_err(local_error);
> 
> Reporting an error, but continue anyway.  This is suspicious.
> 
> Note for later: @local_error is now non-null.
> 
>              } else {
>                  key_id = add_key(KCTL_KEY_TYPE_PKEY, "lkcf-backend-priv-key",
>                                   p8info, p8info_len, KCTL_KEY_RING);
>              }
>          }
> 
>          if (key_id < 0) {
>              if (!qcrypto_akcipher_supports(&session->akcipher_opts)) {
>                  status = -VIRTIO_CRYPTO_NOTSUPP;
>                  goto out;
>              }
>              akcipher = qcrypto_akcipher_new(&session->akcipher_opts,
>                                              session->keytype,
>                                              session->key, session->keylen,
>                                              &local_error);
> 
> Passing non-null @local_error to qcrypto_akcipher_new().  This is wrong.
> When qcrypto_akcipher_new() fails and passes &local_error to
> error_setg(), error_setv()'s assertion will fail.
> 
> Two possible fixes:
> 
> 1. If continuing after cryptodev_lkcf_set_op_desc() is correct, we need
> to clear @local_error there.  Since it's not actually an error then, we
> should almost certainly not use error_report_err() there.  *Maybe*
> warn_report_err().
> 
> 2. If continuing is wrong, we probably need set @status (to what?) and
> goto out.
> 
> What is the correct fix?
> 

Hi,

It's fatal error of a crypto task, so it should not continue. setting 
status as VIRTIO_CRYPTO_ERR should be fine.

Thanks!

[remove Lei He from cc list, he has already resigned from bytedance]

>              if (!akcipher) {
>                  status = -VIRTIO_CRYPTO_ERR;
>                  goto out;
>              }
>          }
> 
>      [...]
> 
>      out:
>          if (key_id >= 0) {
>              keyctl_unlink(key_id, KCTL_KEY_RING);
>          }
>          task->status = status;
> 
>          qemu_mutex_lock(&task->lkcf->rsp_mutex);
>          if (QSIMPLEQ_EMPTY(&task->lkcf->responses)) {
>              kick = true;
>          }
>          QSIMPLEQ_INSERT_TAIL(&task->lkcf->responses, task, queue);
>          qemu_mutex_unlock(&task->lkcf->rsp_mutex);
> 
>          if (kick) {
>              eventfd_write(task->lkcf->eventfd, 1);
>          }
>      }
>