zhenwei pi <[email protected]> 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 <[email protected]>
>> ---
>> 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);
}
