Re: [Qemu-devel] [PATCH v7 08/12] virtio-crypto: add control queue handler

[Stefan Hajnoczi]

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.

signature.asc
Description: PGP signature