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

[Markus Armbruster <armbru@redhat.com>]

zhenwei pi <pizhenwei@bytedance.com> writes:

> 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.

I'll post the obvious patch.  Thanks for your help!

[...]