Re: [PATCH v3 4/7] vhost-pci-slave: add vhost-pci slave implementation

[Stefan Hajnoczi <stefanha@redhat.com>]

On Tue, Dec 05, 2017 at 11:33:13AM +0800, Wei Wang wrote:
> +static int vp_slave_write(CharBackend *chr_be, VhostUserMsg *msg)
> +{
> +    int size;
> +
> +    if (!msg) {
> +        return 0;
> +    }
> +
> +    /* The payload size has been assigned, plus the header size here */
> +    size = msg->size + VHOST_USER_HDR_SIZE;
> +    msg->flags &= ~VHOST_USER_VERSION_MASK;
> +    msg->flags |= VHOST_USER_VERSION;
> +
> +    return qemu_chr_fe_write_all(chr_be, (const uint8_t *)msg, size)
> +           == size ? 0 : -1;
> +}

qemu_chr_fe_write_all() is a blocking operation.  If the socket fd
cannot accept more data then this thread will block!

This is a reliability problem.  If the vhost-user master process hangs
then the vhost-pci vhost-user slave will also hang :(.

Please implement vhost-pci so that it does not hang.  A guest with
multiple vhost-pci devices should work even if one or more of them
cannot communicate with the vhost-pci master.  This is necessary for
preventing denial-of-service on a software-defined network switch, for
example.

> +static void vp_slave_set_vring_num(VhostPCINet *vpnet, VhostUserMsg *msg)
> +{
> +    struct vhost_vring_state *state = &msg->payload.state;
> +
> +    vpnet->metadata->vq[state->index].vring_num = state->num;

The vhost-pci code cannot trust vhost-user input.  This function allows
the vhost-user master to perform out-of-bounds memory stores by setting
state->index outside the vq[] array.

All input must be validated!  The security model is:

1. Guest must be able to corrupt QEMU memory or execute arbitrary code.
2. vhost-user master guest must not be able to corrupt vhost-user slave
   guest memory or execute arbitrary code.
3. vhost-user master must not be able to corrupt vhost-user memory or
   execute arbitrary code in vhost-user slave.
4. vhost-user slave must not be able to corrupt vhost-user memory or
   execute arbitrary code in vhost-user master.

The only thing that is allowed is vhost-user slave QEMU and guest may
write to vhost-user master guest memory.

> +void vp_slave_read(void *opaque, const uint8_t *buf, int size)
> +{
> +    int ret, fd_num, fds[VHOST_MEMORY_MAX_NREGIONS];
> +    VhostUserMsg msg;
> +    uint8_t *p = (uint8_t *) &msg;
> +    VhostPCINet *vpnet = (VhostPCINet *)opaque;
> +    CharBackend *chr_be = &vpnet->chr_be;
> +
> +    if (size != VHOST_USER_HDR_SIZE) {
> +        error_report("%s: wrong message size received %d", __func__, size);
> +        return;
> +    }
> +
> +    memcpy(p, buf, VHOST_USER_HDR_SIZE);
> +
> +    if (msg.size) {
> +        p += VHOST_USER_HDR_SIZE;
> +        size = qemu_chr_fe_read_all(chr_be, p, msg.size);

This is a blocking operation.  See my comment about
qemu_chr_fe_write_all().

This is also a buffer overflow since msg.size is not validated.  All
input from the vhost-user master needs to be validated.

Attachment: signature.asc
Description: PGP signature