Re: [PATCH] cryptodev: Fix error handling in cryptodev_lkcf_execute_task()

[Markus Armbruster <armbru@redhat.com>]

zhenwei pi <pizhenwei@bytedance.com> writes:

> On 3/12/25 18:11, Markus Armbruster wrote:
>> When cryptodev_lkcf_set_op_desc() fails, we report an error, but
>> continue anyway.  This is wrong.  We then pass a non-null @local_error
>> to various functions, which could easily fail error_setv()'s assertion
>> on failure.
>> 
>> Fail the function instead.
>> 
>> When qcrypto_akcipher_new() fails, we fail the function without
>> reporting the error.  This leaks the Error object.
>> 
>> Add the missing error reporting.  This also frees the Error object.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>   backends/cryptodev-lkcf.c | 3 +++
>>   1 file changed, 3 insertions(+)
>> 
>> diff --git a/backends/cryptodev-lkcf.c b/backends/cryptodev-lkcf.c
>> index 41cf24b737..352c3e8958 100644
>> --- a/backends/cryptodev-lkcf.c
>> +++ b/backends/cryptodev-lkcf.c
>> @@ -330,6 +330,8 @@ static void cryptodev_lkcf_execute_task(CryptoDevLKCFTask *task)
>>               cryptodev_lkcf_set_op_desc(&session->akcipher_opts, op_desc,
>>                                          sizeof(op_desc), &local_error) != 0) {
>>               error_report_err(local_error);
>> +            status = -VIRTIO_CRYPTO_ERR;
>> +            goto out;
>>           } else {
>>               key_id = add_key(KCTL_KEY_TYPE_PKEY, "lkcf-backend-priv-key",
>>                                p8info, p8info_len, KCTL_KEY_RING);
>> @@ -346,6 +348,7 @@ static void cryptodev_lkcf_execute_task(CryptoDevLKCFTask *task)
>>                                           session->key, session->keylen,
>>                                           &local_error);
>>           if (!akcipher) {
>> +            error_report_err(local_error);
>>               status = -VIRTIO_CRYPTO_ERR;
>>               goto out;
>>           }
>
> What about moving several 'error_report_err(local_error);' to:
>
> out:
> if (local_error) {
>      error_report_err(local_error);
> }

I figure you suggest something like the appended patch.  But this led me
to another question.  Consider:

        asym_op_info = task->op_info->u.asym_op_info;
        switch (op_code) {
        case VIRTIO_CRYPTO_AKCIPHER_ENCRYPT:
            if (key_id >= 0) {
                ret = keyctl_pkey_encrypt(key_id, op_desc,
                    asym_op_info->src, asym_op_info->src_len,
                    asym_op_info->dst, asym_op_info->dst_len);

When keyctl_pkey_encrypt() fails, @local_error remains unset.

            } else {
                ret = qcrypto_akcipher_encrypt(akcipher,
                    asym_op_info->src, asym_op_info->src_len,
                    asym_op_info->dst, asym_op_info->dst_len, &local_error);
            }
            break;

        [More cases that can also fail without setting @local_error]

        default:
            error_setg(&local_error, "Unknown opcode: %u", op_code);
            status = -VIRTIO_CRYPTO_ERR;
            goto out;
        }

        if (ret < 0) {

The switch failed.

            if (!local_error) {

If it failed without setting @local_error, we report a generic error
*unless* errno is EKEYREJECTED.

Aside: checking errno this far from whatever set it is asking for
trouble.  It gets overwritten easily.

                if (errno != EKEYREJECTED) {
                    error_report("Failed do operation with keyctl: %d", errno);
                }

If it failed with setting @local_error, we report that error.

            } else {
                error_report_err(local_error);
            }
            status = op_code == VIRTIO_CRYPTO_AKCIPHER_VERIFY ?
                -VIRTIO_CRYPTO_KEY_REJECTED : -VIRTIO_CRYPTO_ERR;

Status set to negative value.  This will be assigned to task->status
below.

It can therefore become negative *silently* (i.e. without an error
report).  Why is this okay?

        } else {
            status = VIRTIO_CRYPTO_OK;
            asym_op_info->dst_len = ret;
        }

    out:
        if (key_id >= 0) {
            keyctl_unlink(key_id, KCTL_KEY_RING);
        }
        task->status = status;



diff --git a/backends/cryptodev-lkcf.c b/backends/cryptodev-lkcf.c
index 41cf24b737..0e20797cb3 100644
--- a/backends/cryptodev-lkcf.c
+++ b/backends/cryptodev-lkcf.c
@@ -329,7 +329,8 @@ static void cryptodev_lkcf_execute_task(CryptoDevLKCFTask *task)
                                            &local_error) != 0 ||
             cryptodev_lkcf_set_op_desc(&session->akcipher_opts, op_desc,
                                        sizeof(op_desc), &local_error) != 0) {
-            error_report_err(local_error);
+            status = -VIRTIO_CRYPTO_ERR;
+            goto out;
         } else {
             key_id = add_key(KCTL_KEY_TYPE_PKEY, "lkcf-backend-priv-key",
                              p8info, p8info_len, KCTL_KEY_RING);
@@ -410,10 +411,9 @@ static void cryptodev_lkcf_execute_task(CryptoDevLKCFTask *task)
     if (ret < 0) {
         if (!local_error) {
             if (errno != EKEYREJECTED) {
-                error_report("Failed do operation with keyctl: %d", errno);
+                error_setg_errno(&local_error,
+                                 "Failed do operation with keyctl: %d");
             }
-        } else {
-            error_report_err(local_error);
         }
         status = op_code == VIRTIO_CRYPTO_AKCIPHER_VERIFY ?
             -VIRTIO_CRYPTO_KEY_REJECTED : -VIRTIO_CRYPTO_ERR;
@@ -423,6 +423,9 @@ static void cryptodev_lkcf_execute_task(CryptoDevLKCFTask *task)
     }
 
 out:
+    if (local_error) {
+        error_report_err(local_error);
+    }
     if (key_id >= 0) {
         keyctl_unlink(key_id, KCTL_KEY_RING);
     }