Hi Markus,
Current code style seems buggy, I think the main reason is that the
Error *errp is not generated at right place. keyctl_pkey_XXX fails
without new error, qcrypto_akcipher_XXX fails with new error, but they
are in the same switch-case code block. If we can separate crypto
operations into two functions - cryptodev_lkcf_keyctl_op and
cryptodev_lkcf_qakcipher_op, and the error is generate inside the
functions, it may be handled easily. Then applying your changes, it seem
more clear. What do you think?
+static inline int cryptodev_lkcf_keyctl_op(key_serial_t key_id, int
op_code, char *op_desc,
+ CryptoDevBackendAsymOpInfo *asym_op_info, Error
**errp)
+{
+ int ret = -1;
+
+ switch (op_code) {
+ case VIRTIO_CRYPTO_AKCIPHER_ENCRYPT:
+ 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);
+ break;
+
+ case VIRTIO_CRYPTO_AKCIPHER_DECRYPT:
+ ret = keyctl_pkey_decrypt(key_id, op_desc,
+ asym_op_info->src, asym_op_info->src_len,
+ asym_op_info->dst, asym_op_info->dst_len);
+ break;
+
+ case VIRTIO_CRYPTO_AKCIPHER_SIGN:
+ ret = keyctl_pkey_sign(key_id, op_desc,
+ asym_op_info->src, asym_op_info->src_len,
+ asym_op_info->dst, asym_op_info->dst_len);
+ break;
+
+ case VIRTIO_CRYPTO_AKCIPHER_VERIFY:
+ ret = keyctl_pkey_verify(key_id, op_desc,
+ asym_op_info->src, asym_op_info->src_len,
+ asym_op_info->dst, asym_op_info->dst_len);
+ break;
+
+ default:
+ error_setg(errp, "Unknown opcode: %u", op_code);
+ }
+
+ if (ret && !*errp) {
+ error_setg_errno(errp, errno, "Failed do operation with keyctl");
+ }
+
+ return ret;
+}
+
+static inline int cryptodev_lkcf_qakcipher_op(QCryptoAkCipher
*akcipher, int op_code, char *op_desc,
+ CryptoDevBackendAsymOpInfo *asym_op_info, Error
**errp)
+{
+ int ret = -1;
+
+ switch (op_code) {
+ case VIRTIO_CRYPTO_AKCIPHER_ENCRYPT:
+ ret = qcrypto_akcipher_encrypt(akcipher,
+ asym_op_info->src, asym_op_info->src_len,
+ asym_op_info->dst, asym_op_info->dst_len, errp);
+ break;
+
+ case VIRTIO_CRYPTO_AKCIPHER_DECRYPT:
+ ret = qcrypto_akcipher_decrypt(akcipher,
+ asym_op_info->src, asym_op_info->src_len,
+ asym_op_info->dst, asym_op_info->dst_len, errp);
+ break;
+
+ case VIRTIO_CRYPTO_AKCIPHER_SIGN:
+ ret = qcrypto_akcipher_sign(akcipher,
+ asym_op_info->src, asym_op_info->src_len,
+ asym_op_info->dst, asym_op_info->dst_len, errp);
+ break;
+
+ case VIRTIO_CRYPTO_AKCIPHER_VERIFY:
+ ret = qcrypto_akcipher_verify(akcipher,
+ asym_op_info->src, asym_op_info->src_len,
+ asym_op_info->dst, asym_op_info->dst_len, errp);
+ break;
+
+ default:
+ error_setg(errp, "Unknown opcode: %u", op_code);
+ }
+
+ return ret;
+}
+
static void cryptodev_lkcf_execute_task(CryptoDevLKCFTask *task)
{
CryptoDevBackendLKCFSession *session = task->sess;
@@ -336,6 +414,7 @@ static void
cryptodev_lkcf_execute_task(CryptoDevLKCFTask *task)
}
}
+ asym_op_info = task->op_info->u.asym_op_info;
if (key_id < 0) {
if (!qcrypto_akcipher_supports(&session->akcipher_opts)) {
status = -VIRTIO_CRYPTO_NOTSUPP;
@@ -349,72 +428,13 @@ static void
cryptodev_lkcf_execute_task(CryptoDevLKCFTask *task)
status = -VIRTIO_CRYPTO_ERR;
goto out;
}
- }
-
- 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);
- } 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;
- case VIRTIO_CRYPTO_AKCIPHER_DECRYPT:
- if (key_id >= 0) {
- ret = keyctl_pkey_decrypt(key_id, op_desc,
- asym_op_info->src, asym_op_info->src_len,
- asym_op_info->dst, asym_op_info->dst_len);
- } else {
- ret = qcrypto_akcipher_decrypt(akcipher,
- asym_op_info->src, asym_op_info->src_len,
- asym_op_info->dst, asym_op_info->dst_len, &local_error);
- }
- break;
-
- case VIRTIO_CRYPTO_AKCIPHER_SIGN:
- if (key_id >= 0) {
- ret = keyctl_pkey_sign(key_id, op_desc,
- asym_op_info->src, asym_op_info->src_len,
- asym_op_info->dst, asym_op_info->dst_len);
- } else {
- ret = qcrypto_akcipher_sign(akcipher,
- asym_op_info->src, asym_op_info->src_len,
- asym_op_info->dst, asym_op_info->dst_len, &local_error);
- }
- break;
-
- case VIRTIO_CRYPTO_AKCIPHER_VERIFY:
- if (key_id >= 0) {
- ret = keyctl_pkey_verify(key_id, op_desc,
- asym_op_info->src, asym_op_info->src_len,
- asym_op_info->dst, asym_op_info->dst_len);
- } else {
- ret = qcrypto_akcipher_verify(akcipher,
- asym_op_info->src, asym_op_info->src_len,
- asym_op_info->dst, asym_op_info->dst_len, &local_error);
- }
- break;
-
- default:
- error_setg(&local_error, "Unknown opcode: %u", op_code);
- status = -VIRTIO_CRYPTO_ERR;
- goto out;
+ ret = cryptodev_lkcf_qakcipher_op(akcipher, op_code, op_desc,
asym_op_info, &local_error);
+ } else {
+ ret = cryptodev_lkcf_keyctl_op(key_id, op_code, op_desc,
asym_op_info, &local_error);
}
if (ret < 0) {
- if (!local_error) {
- if (errno != EKEYREJECTED) {
- error_report("Failed do operation with keyctl: %d", errno);
- }
- } else {
- error_report_err(local_error);
- }
status = op_code == VIRTIO_CRYPTO_AKCIPHER_VERIFY ?
-VIRTIO_CRYPTO_KEY_REJECTED : -VIRTIO_CRYPTO_ERR;
} else {
On 3/12/25 20:02, Markus Armbruster wrote:
zhenwei pi <pizhen...@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 <arm...@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);
}
