> On May 12, 2020, at 4:56 AM, Stefan Hajnoczi <stefa...@redhat.com> wrote:
>
> On Wed, Apr 22, 2020 at 09:13:46PM -0700, elena.ufimts...@oracle.com wrote:
>> From: Jagannathan Raman <jag.ra...@oracle.com>
>>
>> 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 <jag.ra...@oracle.com>
>> Signed-off-by: John G Johnson <john.g.john...@oracle.com>
>> Signed-off-by: Elena Ufimtseva <elena.ufimts...@oracle.com>
>
> 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.
OK, got it. Thanks!
>
>> +/**
>> + * 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.
