Re: [PATCH RESEND v6 11/36] multi-process: define mpqemu-link object

[Stefan Hajnoczi]

On Wed, Apr 22, 2020 at 09:13:46PM -0700, address@hidden wrote:
> From: Jagannathan Raman <address@hidden>
> 
> Defines mpqemu-link object which forms the communication link between
> QEMU & emulation program.
> Adds functions to configure members of mpqemu-link object instance.
> Adds functions to send and receive messages over the communication
> channel.
> Adds GMainLoop to handle events received on the communication channel.
> 
> Signed-off-by: Jagannathan Raman <address@hidden>
> Signed-off-by: John G Johnson <address@hidden>
> Signed-off-by: Elena Ufimtseva <address@hidden>

This will change a lot when integrated into the QEMU event loop so I've
skipped a lot of the code.

QIOChannel is probably the appropriate object to use instead of directly
accessing a file descriptor.

> +/**
> + * mpqemu_cmd_t:
> + *
> + * proc_cmd_t enum type to specify the command to be executed on the remote
> + * device.
> + */
> +typedef enum {
> +    INIT = 0,
> +    MAX,
> +} mpqemu_cmd_t;
> +
> +/**
> + * MPQemuMsg:
> + * @cmd: The remote command
> + * @bytestream: Indicates if the data to be shared is structured (data1)
> + *              or unstructured (data2)
> + * @size: Size of the data to be shared
> + * @data1: Structured data
> + * @fds: File descriptors to be shared with remote device
> + * @data2: Unstructured data
> + *
> + * MPQemuMsg Format of the message sent to the remote device from QEMU.
> + *
> + */
> +typedef struct {
> +    mpqemu_cmd_t cmd;

Please use an int field on the wire because the C standard says:

  Each enumerated type shall be compatible with char, a signed integer
  type, or an unsigned integer type. The choice of type is
  implementation-defined, but shall be capable of representing the
  values of all the members of the enumeration.

So the compiler may make this a char field (which would introduce
padding before the bytestream field) but if a new enum constant FOO =
0x100 is added then the compiler might change the size to 16-bit.

> +int mpqemu_msg_recv(MPQemuMsg *msg, MPQemuChannel *chan)
> +{
> +    int rc;
> +    uint8_t *data;
> +    union {
> +        char control[CMSG_SPACE(REMOTE_MAX_FDS * sizeof(int))];
> +        struct cmsghdr align;
> +    } u;
> +    struct msghdr hdr;
> +    struct cmsghdr *chdr;
> +    size_t fdsize;
> +    int sock = chan->sock;
> +    QemuMutex *lock = &chan->recv_lock;
> +
> +    struct iovec iov = {
> +        .iov_base = (char *) msg,
> +        .iov_len = MPQEMU_MSG_HDR_SIZE,
> +    };
> +
> +    memset(&hdr, 0, sizeof(hdr));
> +    memset(&u, 0, sizeof(u));
> +
> +    hdr.msg_iov = &iov;
> +    hdr.msg_iovlen = 1;
> +    hdr.msg_control = &u;
> +    hdr.msg_controllen = sizeof(u);
> +
> +    WITH_QEMU_LOCK_GUARD(lock) {
> +        do {
> +            rc = recvmsg(sock, &hdr, 0);
> +        } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
> +
> +        if (rc < 0) {

Missing rc != MPQEMU_MSG_HDR_SIZE check. If this was a short read we
should not attempt to parse uninitialized bytes in msg.

This is more defensive than relying on catching bogus input values later
on and also protects against accidentally revealing uninitialized memory
contents by observing our error handling response.

> +            qemu_log_mask(LOG_REMOTE_DEBUG, "%s - recvmsg rc is %d, "
> +                          "errno is %d, sock %d\n", __func__, rc, errno, 
> sock);
> +            return rc;
> +        }
> +
> +        msg->num_fds = 0;
> +        for (chdr = CMSG_FIRSTHDR(&hdr); chdr != NULL;
> +             chdr = CMSG_NXTHDR(&hdr, chdr)) {
> +            if ((chdr->cmsg_level == SOL_SOCKET) &&
> +                (chdr->cmsg_type == SCM_RIGHTS)) {
> +                fdsize = chdr->cmsg_len - CMSG_LEN(0);
> +                msg->num_fds = fdsize / sizeof(int);
> +                if (msg->num_fds > REMOTE_MAX_FDS) {
> +                    qemu_log_mask(LOG_REMOTE_DEBUG,
> +                                  "%s: Max FDs exceeded\n", __func__);
> +                    return -ERANGE;
> +                }
> +
> +                memcpy(msg->fds, CMSG_DATA(chdr), fdsize);
> +                break;
> +            }
> +        }
> +
> +        if (msg->bytestream) {
> +            if (!msg->size) {
> +                qemu_mutex_unlock(lock);

Duplicate unlock, we're already inside WITH_QEMU_LOCK_GUARD().

> +                return -EINVAL;
> +            }
> +
> +            msg->data2 = calloc(1, msg->size);

What is the maximum message size? Please pick one and enforce it to
protect against huge allocations that cause us to run out of memory.

> +            data = msg->data2;
> +        } else {
> +            data = (uint8_t *)&msg->data1;

Adding a uint8_t member to the union eliminates the need for a cast:

  union {
      uint64_t u64;
      uint8_t u8;
  } data1;

  ...

  data = &msg->data1.u8;

> +        }
> +
> +        if (msg->size) {
> +            do {
> +                rc = read(sock, data, msg->size);
> +            } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
> +        }

Short reads are an error. Please check that the sum of rc values is
equal to msg->size.

> +    }
> +    return rc;
> +}
...
> +bool mpqemu_msg_valid(MPQemuMsg *msg)
> +{
> +    if (msg->cmd >= MAX) {
> +        return false;
> +    }

Checking msg->cmd < 0 is also useful here, especially when the field
type is changed to int.

signature.asc
Description: PGP signature