From: zhenwei pi <pizhenwei@bytedance.com>
To: Markus Armbruster <armbru@redhat.com>
Cc: qemu-devel@nongnu.org, arei.gonglei@huawei.com
Subject: Re: Re: [PATCH] cryptodev: Fix error handling in cryptodev_lkcf_execute_task()
Date: Fri, 14 Mar 2025 16:34:44 +0800 [thread overview]
Message-ID: <44db6eee-2cba-43f0-8977-382e44af3b6f@bytedance.com> (raw)
In-Reply-To: <87r032ihj2.fsf@pond.sub.org>
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 <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);
> }
>
next prev parent reply other threads:[~2025-03-14 8:36 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-03-12 10:11 [PATCH] cryptodev: Fix error handling in cryptodev_lkcf_execute_task() Markus Armbruster
2025-03-12 10:28 ` zhenwei pi
2025-03-12 12:02 ` Markus Armbruster
2025-03-14 8:34 ` zhenwei pi [this message]
2025-03-18 13:21 ` Markus Armbruster
2025-03-19 2:21 ` zhenwei pi
2025-03-19 5:12 ` Markus Armbruster
2025-03-19 6:29 ` zhenwei pi
2025-03-19 8:37 ` Markus Armbruster
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=44db6eee-2cba-43f0-8977-382e44af3b6f@bytedance.com \
--to=pizhenwei@bytedance.com \
--cc=arei.gonglei@huawei.com \
--cc=armbru@redhat.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).