[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: RE: [Qemu-devel] [PATCH v7 08/12] virtio-crypto: add control queue handler
Hi Stefan,
Thanks for your comments!
> -----Original Message-----
> From: Stefan Hajnoczi [mailto:stefanha@gmail.com]
> Sent: Sunday, October 16, 2016 9:03 PM
> Subject: Re: [Qemu-devel] [PATCH v7 08/12] virtio-crypto: add control queue
> handler
>
> On Thu, Oct 13, 2016 at 03:12:02PM +0800, Gonglei wrote:
> > +static int64_t
> > +virtio_crypto_create_sym_session(VirtIOCrypto *vcrypto,
> > + struct virtio_crypto_sym_create_session_req *sess_req,
> > + uint32_t queue_id,
> > + uint32_t opcode,
> > + struct iovec *iov, unsigned int out_num)
> > +{
> > + VirtIODevice *vdev = VIRTIO_DEVICE(vcrypto);
> > + CryptoDevBackendSymSessionInfo info;
> > + int64_t session_id;
> > + int queue_index;
> > + uint32_t op_type;
> > + Error *local_err = NULL;
> > + int ret;
> > +
> > + memset(&info, 0, sizeof(info));
> > + op_type = virtio_ldl_p(vdev, &sess_req->op_type);
> > + info.op_type = op_type;
> > + info.op_code = opcode;
> > +
> > + if (op_type == VIRTIO_CRYPTO_SYM_OP_CIPHER) {
> > + ret = virtio_crypto_cipher_session_helper(vdev, &info,
> > + &sess_req->u.cipher.para,
> > + &iov, &out_num);
> > + if (ret < 0) {
> > + return -EFAULT;
>
> info.cipher_key is leaked here.
>
Will fix it.
> > + }
> > + } else if (op_type ==
> VIRTIO_CRYPTO_SYM_OP_ALGORITHM_CHAINING) {
> > + size_t s;
> > + /* cipher part */
> > + ret = virtio_crypto_cipher_session_helper(vdev, &info,
> > + &sess_req->u.chain.para.cipher_param,
> > + &iov, &out_num);
> > + if (ret < 0) {
> > + return -EFAULT;
>
> info.cipher_key is leaked here.
Will fix it.
>
> > + }
> > + /* hash part */
> > + info.alg_chain_order = virtio_ldl_p(vdev,
> > +
> &sess_req->u.chain.para.alg_chain_order);
> > + info.add_len = virtio_ldl_p(vdev,
> &sess_req->u.chain.para.aad_len);
> > + info.hash_mode = virtio_ldl_p(vdev,
> &sess_req->u.chain.para.hash_mode);
> > + if (info.hash_mode == VIRTIO_CRYPTO_SYM_HASH_MODE_AUTH)
> {
> > + info.hash_alg = virtio_ldl_p(vdev,
> > +
> &sess_req->u.chain.para.u.mac_param.algo);
> > + info.auth_key_len = virtio_ldl_p(vdev,
> > +
> &sess_req->u.chain.para.u.mac_param.auth_key_len);
> > + info.hash_result_len = virtio_ldl_p(vdev,
> > +
> &sess_req->u.chain.para.u.mac_param.hash_result_len);
> > + /* get auth key */
> > + if (info.auth_key_len > 0) {
> > + DPRINTF("auth_keylen=%" PRIu32 "\n",
> info.auth_key_len);
> > + info.auth_key = g_malloc(info.auth_key_len);
> > + s = iov_to_buf(iov, out_num, 0, info.auth_key,
> > + info.auth_key_len);
> > + if (unlikely(s != info.auth_key_len)) {
> > + virtio_error(vdev,
> > + "virtio-crypto authenticated key incorrect");
> > + ret = -EFAULT;
> > + goto err;
> > + }
> > + iov_discard_front(&iov, &out_num, info.auth_key_len);
> > + }
> > + } else if (info.hash_mode ==
> VIRTIO_CRYPTO_SYM_HASH_MODE_PLAIN) {
> > + info.hash_alg = virtio_ldl_p(vdev,
> > +
> &sess_req->u.chain.para.u.hash_param.algo);
> > + info.hash_result_len = virtio_ldl_p(vdev,
> > +
> &sess_req->u.chain.para.u.hash_param.hash_result_len);
> > + } else {
> > + /* VIRTIO_CRYPTO_SYM_HASH_MODE_NESTED */
> > + error_report("unsupported hash mode");
>
> Why is error_report() used instead of virtio_error()? The same applies
> elsewhere.
>
For this kind of error, we don't need to require the driver reset the virtio crypto
device, but receive a VIRTIO_CRYPTO_NOTSUPP because
the virtio crypto spec defines the error status, and the device need to report
an error.
But when we encounter an EFAULT error, we need to use virtio_error() to require
a reset of the device.
> > + ret = -VIRTIO_CRYPTO_NOTSUPP;
> > + goto err;
> > + }
> > + } else {
> > + /* VIRTIO_CRYPTO_SYM_OP_NONE */
> > + error_report("unsupported cipher op_type:
> VIRTIO_CRYPTO_SYM_OP_NONE");
> > + ret = -VIRTIO_CRYPTO_NOTSUPP;
> > + goto err;
> > + }
> > +
> > + queue_index = virtio_crypto_vq2q(queue_id);
> > + session_id = cryptodev_backend_sym_create_session(
> > + vcrypto->cryptodev,
> > + &info, queue_index,
> &local_err);
> > + if (session_id >= 0) {
> > + DPRINTF("create session_id=%" PRIu64 " successfully\n",
> > + session_id);
> > +
> > + ret = session_id;
> > + } else {
> > + if (local_err) {
> > + error_report_err(local_err);
> > + }
> > + ret = -VIRTIO_CRYPTO_ERR;
> > + }
> > +
> > +err:
> > + g_free(info.cipher_key);
> > + g_free(info.auth_key);
> > + return ret;
> > +}
> > +
> > +static uint32_t
> > +virtio_crypto_handle_close_session(VirtIOCrypto *vcrypto,
> > + struct virtio_crypto_destroy_session_req *close_sess_req,
> > + uint32_t queue_id)
> > +{
> > + VirtIODevice *vdev = VIRTIO_DEVICE(vcrypto);
> > + int ret;
> > + uint64_t session_id;
> > + uint32_t status;
> > + Error *local_err = NULL;
> > +
> > + session_id = virtio_ldq_p(vdev, &close_sess_req->session_id);
> > + DPRINTF("close session, id=%" PRIu64 "\n", session_id);
> > +
> > + ret = cryptodev_backend_sym_close_session(
> > + vcrypto->cryptodev, session_id, queue_id, &local_err);
> > + if (ret == 0) {
> > + status = VIRTIO_CRYPTO_OK;
> > + } else {
> > + if (local_err) {
> > + error_report_err(local_err);
> > + } else {
> > + error_report("destroy session failed");
> > + }
> > + status = VIRTIO_CRYPTO_ERR;
> > + }
> > +
> > + return status;
> > +}
> > +
> > +static void virtio_crypto_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
> > +{
> > + VirtIOCrypto *vcrypto = VIRTIO_CRYPTO(vdev);
> > + struct virtio_crypto_op_ctrl_req ctrl;
> > + VirtQueueElement *elem;
> > + size_t in_len;
> > + struct iovec *in_iov;
> > + struct iovec *out_iov;
> > + unsigned in_num;
> > + unsigned out_num;
> > + uint32_t queue_id;
> > + uint32_t opcode;
> > + struct virtio_crypto_session_input *input;
> > + int64_t session_id;
> > + uint32_t status;
> > + size_t s;
> > +
> > + for (;;) {
> > + elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
> > + if (!elem) {
> > + break;
> > + }
> > + if (elem->out_num < 1 || elem->in_num < 1) {
> > + virtio_error(vdev, "virtio-crypto ctrl missing headers");
> > + virtqueue_detach_element(vq, elem, 0);
> > + g_free(elem);
> > + break;
> > + }
> > +
> > + out_num = elem->out_num;
> > + out_iov = elem->out_sg;
> > + in_num = elem->in_num;
> > + in_iov = elem->in_sg;
> > + if (unlikely(iov_to_buf(out_iov, out_num, 0, &ctrl, sizeof(ctrl))
> > + != sizeof(ctrl))) {
> > + virtio_error(vdev, "virtio-crypto request ctrl_hdr too short");
> > + virtqueue_detach_element(vq, elem, 0);
> > + g_free(elem);
> > + break;
> > + }
> > + iov_discard_front(&out_iov, &out_num, sizeof(ctrl));
> > +
> > + opcode = virtio_ldl_p(vdev, &ctrl.header.opcode);
> > + queue_id = virtio_ldl_p(vdev, &ctrl.header.queue_id);
> > +
> > + switch (opcode) {
> > + case VIRTIO_CRYPTO_CIPHER_CREATE_SESSION:
> > + in_len = iov_size(in_iov, in_num);
> > + input = (void *)in_iov[in_num - 1].iov_base
> > + + in_iov[in_num - 1].iov_len
> > + - sizeof(*input);
>
> This type of calculation is dangerous. It is only safe if struct
> virtio_crypto_session_input is 1 byte in size. Otherwise the descriptor
> boundary could split the struct across two iovecs and QEMU will corrupt
> arbitrary memory.
>
> Please use the iov_*() functions on in_iov/in_num instead of directly
> accessing the iovecs.
>
> There are other instances of this problem in the code, please address
> them too.
>
OK.
> > + iov_discard_back(in_iov, &in_num, sizeof(*input));
> > +
> > + session_id = virtio_crypto_create_sym_session(vcrypto,
> > + &ctrl.u.sym_create_session,
> > + queue_id, opcode,
> > + out_iov, out_num);
> > + /* Serious errors, need to reset virtio crypto device */
> > + if (session_id == -EFAULT) {
> > + virtqueue_detach_element(vq, elem, 0);
>
> The device should enter the broken state using virtio_error() if we
> detach a buffer. Something is broken and the guest may even hang
> waiting for the descriptor to complete...
>
Right, and I have invoked virtio_error() before initial returning a -EFAULT in both
virtio_crypto_cipher_session_helper() and virtio_crypto_create_sym_session().
> > + break;
> > + } else if (session_id == -VIRTIO_CRYPTO_NOTSUPP) {
> > + input->status = VIRTIO_CRYPTO_NOTSUPP;
>
> Needs to be virtio_stl_p() so it works correctly on big-endian hosts.
>
Will fix it.
> > + } else if (session_id == -VIRTIO_CRYPTO_ERR) {
> > + input->status = VIRTIO_CRYPTO_ERR;
> > + } else {
> > + input->session_id = session_id;
> > + input->status = VIRTIO_CRYPTO_OK;
> > + }
> > +
> > + virtqueue_push(vq, elem, in_len);
> > + virtio_notify(vdev, vq);
> > + break;
> > + case VIRTIO_CRYPTO_CIPHER_DESTROY_SESSION:
> > + case VIRTIO_CRYPTO_HASH_DESTROY_SESSION:
> > + case VIRTIO_CRYPTO_MAC_DESTROY_SESSION:
> > + case VIRTIO_CRYPTO_AEAD_DESTROY_SESSION:
> > + status = virtio_crypto_handle_close_session(vcrypto,
> > + &ctrl.u.destroy_session, queue_id);
>
> Missing virtio_stl_p() or equivalent byteswap. This will break on
> big-endian hosts.
>
> I won't mention endianness anymore, please check that all virtio-crypto
> struct fields are accessed safely.
I'll change the status here into one bytes, so that we can use it directly.
And I will recheck all of them for endianness. Thanks!
Regards,
-Gonglei
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]