* Bad error handling in cryptodev_lkcf_execute_task(), need advice
@ 2025-03-12 8:10 Markus Armbruster
2025-03-12 9:01 ` zhenwei pi
0 siblings, 1 reply; 3+ messages in thread
From: Markus Armbruster @ 2025-03-12 8:10 UTC (permalink / raw)
To: qemu-devel; +Cc: Lei He, Michael S. Tsirkin, Gonglei (Arei), Zhenwei Pi
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?
if (!akcipher) {
status = -VIRTIO_CRYPTO_ERR;
goto out;
}
}
[...]
out:
if (key_id >= 0) {
keyctl_unlink(key_id, KCTL_KEY_RING);
}
task->status = status;
qemu_mutex_lock(&task->lkcf->rsp_mutex);
if (QSIMPLEQ_EMPTY(&task->lkcf->responses)) {
kick = true;
}
QSIMPLEQ_INSERT_TAIL(&task->lkcf->responses, task, queue);
qemu_mutex_unlock(&task->lkcf->rsp_mutex);
if (kick) {
eventfd_write(task->lkcf->eventfd, 1);
}
}
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: Bad error handling in cryptodev_lkcf_execute_task(), need advice
2025-03-12 8:10 Bad error handling in cryptodev_lkcf_execute_task(), need advice Markus Armbruster
@ 2025-03-12 9:01 ` zhenwei pi
2025-03-12 9:48 ` Markus Armbruster
0 siblings, 1 reply; 3+ messages in thread
From: zhenwei pi @ 2025-03-12 9:01 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: Michael S. Tsirkin, Gonglei (Arei)
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.
Thanks!
[remove Lei He from cc list, he has already resigned from bytedance]
> if (!akcipher) {
> status = -VIRTIO_CRYPTO_ERR;
> goto out;
> }
> }
>
> [...]
>
> out:
> if (key_id >= 0) {
> keyctl_unlink(key_id, KCTL_KEY_RING);
> }
> task->status = status;
>
> qemu_mutex_lock(&task->lkcf->rsp_mutex);
> if (QSIMPLEQ_EMPTY(&task->lkcf->responses)) {
> kick = true;
> }
> QSIMPLEQ_INSERT_TAIL(&task->lkcf->responses, task, queue);
> qemu_mutex_unlock(&task->lkcf->rsp_mutex);
>
> if (kick) {
> eventfd_write(task->lkcf->eventfd, 1);
> }
> }
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: Bad error handling in cryptodev_lkcf_execute_task(), need advice
2025-03-12 9:01 ` zhenwei pi
@ 2025-03-12 9:48 ` Markus Armbruster
0 siblings, 0 replies; 3+ messages in thread
From: Markus Armbruster @ 2025-03-12 9:48 UTC (permalink / raw)
To: zhenwei pi; +Cc: qemu-devel, Michael S. Tsirkin, Gonglei (Arei)
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!
[...]
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-03-12 9:49 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-03-12 8:10 Bad error handling in cryptodev_lkcf_execute_task(), need advice Markus Armbruster
2025-03-12 9:01 ` zhenwei pi
2025-03-12 9:48 ` Markus Armbruster
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).