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

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org

Reply via email to