Hi Stefan, Thanks for your comments!
> -----Original Message----- > From: Stefan Hajnoczi [mailto:stefa...@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
