From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48684) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bvl5h-0005YS-Kn for qemu-devel@nongnu.org; Sun, 16 Oct 2016 09:03:02 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bvl5d-0002oX-Q6 for qemu-devel@nongnu.org; Sun, 16 Oct 2016 09:03:01 -0400 Received: from mail-lf0-x243.google.com ([2a00:1450:4010:c07::243]:35568) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1bvl5d-0002nT-D9 for qemu-devel@nongnu.org; Sun, 16 Oct 2016 09:02:57 -0400 Received: by mail-lf0-x243.google.com with SMTP id x79so21477130lff.2 for ; Sun, 16 Oct 2016 06:02:57 -0700 (PDT) Date: Sun, 16 Oct 2016 14:02:52 +0100 From: Stefan Hajnoczi Message-ID: <20161016130252.GA13294@stefanha-x1.localdomain> References: <1476342726-104488-1-git-send-email-arei.gonglei@huawei.com> <1476342726-104488-9-git-send-email-arei.gonglei@huawei.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="M9NhX3UHpAaciwkO" Content-Disposition: inline In-Reply-To: <1476342726-104488-9-git-send-email-arei.gonglei@huawei.com> Subject: Re: [Qemu-devel] [PATCH v7 08/12] virtio-crypto: add control queue handler List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Gonglei Cc: qemu-devel@nongnu.org, virtio-dev@lists.oasis-open.org, weidong.huang@huawei.com, claudio.fontana@huawei.com, mst@redhat.com, xin.zeng@intel.com, hanweidong@huawei.com, luonengjun@huawei.com, agraf@suse.de, nmorey@kalray.eu, mike.caraman@nxp.com, stefanha@redhat.com, jianjay.zhou@huawei.com, pbonzini@redhat.com, peter.huangpeng@huawei.com, vincent.jardin@6wind.com, wu.wubin@huawei.com, arei.gonglei@hotmail.com --M9NhX3UHpAaciwkO Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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. > + } > + } 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. > + } > + /* 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. > + 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. > + 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... > + 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. > + } 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. --M9NhX3UHpAaciwkO Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEcBAEBAgAGBQJYA3p8AAoJEJykq7OBq3PIlaEH/1jF7usbKCWSt/jQ8tF1QjVf mFzK7p4Jsho+uHqi+L/atKc+WWIY//Rns6xHNGhghAgLmuXBH2OpUJOWjs9muLHZ OeG+y2uSR4Rikz01e2SRKL0BY/eOoeDGVe5Qthx1+/5EQRdYhrN0Vm9xD4ntLEyQ ymf/qYriiexhFft0BRGjWzARxx7vVyAbrHPraYVBIT109vQCtMb+LWnLwD7L4cAm xx+Z0t5mjtboTfVEVdsbobZA7TOUBn/8ojic2TMB0ost73boAaCIudm3BpECQAxd 1YOY9VbqBvZ6mObbcvefLQ4cU+I2nV209lEAxGpgFUOy8Smm4Hm5l/wkh4V0hF0= =Kgtw -----END PGP SIGNATURE----- --M9NhX3UHpAaciwkO--