Re: [PATCH RFC v2 06/16] vfio-user: negotiate version with remote server

[Stefan Hajnoczi]

On Mon, Aug 16, 2021 at 09:42:39AM -0700, Elena Ufimtseva wrote:
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 7005d9f891..eae33e746f 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -3397,6 +3397,12 @@ static void vfio_user_pci_realize(PCIDevice *pdev, 
> Error **errp)
>          proxy->flags |= VFIO_PROXY_SECURE;
>      }
>  
> +    vfio_user_validate_version(vbasedev, &err);
> +    if (err != NULL) {
> +        error_propagate(errp, err);
> +        goto error;
> +    }
> +
>      vbasedev->name = g_strdup_printf("VFIO user <%s>", udev->sock_name);
>      vbasedev->dev = DEVICE(vdev);
>      vbasedev->fd = -1;
> @@ -3404,6 +3410,9 @@ static void vfio_user_pci_realize(PCIDevice *pdev, 
> Error **errp)
>      vbasedev->no_mmap = false;
>      vbasedev->ops = &vfio_user_pci_ops;
>  
> +error:

Missing return before error label? We shouldn't disconnect in the
success case.

> +    vfio_user_disconnect(proxy);
> +    error_prepend(errp, VFIO_MSG_PREFIX, vdev->vbasedev.name);
>  }
>  
>  static void vfio_user_instance_finalize(Object *obj)
> diff --git a/hw/vfio/user.c b/hw/vfio/user.c
> index 2fcc77d997..e89464a571 100644
> --- a/hw/vfio/user.c
> +++ b/hw/vfio/user.c
> @@ -23,9 +23,16 @@
>  #include "io/channel-socket.h"
>  #include "io/channel-util.h"
>  #include "sysemu/iothread.h"
> +#include "qapi/qmp/qdict.h"
> +#include "qapi/qmp/qjson.h"
> +#include "qapi/qmp/qnull.h"
> +#include "qapi/qmp/qstring.h"
> +#include "qapi/qmp/qnum.h"
>  #include "user.h"
>  
>  static uint64_t max_xfer_size = VFIO_USER_DEF_MAX_XFER;
> +static uint64_t max_send_fds = VFIO_USER_DEF_MAX_FDS;
> +static int wait_time = 1000;   /* wait 1 sec for replies */
>  static IOThread *vfio_user_iothread;
>  
>  static void vfio_user_shutdown(VFIOProxy *proxy);
> @@ -34,7 +41,14 @@ static void vfio_user_send_locked(VFIOProxy *proxy, 
> VFIOUserHdr *msg,
>                                    VFIOUserFDs *fds);
>  static void vfio_user_send(VFIOProxy *proxy, VFIOUserHdr *msg,
>                             VFIOUserFDs *fds);
> +static void vfio_user_request_msg(VFIOUserHdr *hdr, uint16_t cmd,
> +                                  uint32_t size, uint32_t flags);
> +static void vfio_user_send_recv(VFIOProxy *proxy, VFIOUserHdr *msg,
> +                                VFIOUserFDs *fds, int rsize, int flags);
>  
> +/* vfio_user_send_recv flags */
> +#define NOWAIT          0x1  /* do not wait for reply */
> +#define NOIOLOCK        0x2  /* do not drop iolock */

Please use "BQL", it's a widely used term while "iolock" isn't used:
s/IOLOCK/BQL/

>  
>  /*
>   * Functions called by main, CPU, or iothread threads
> @@ -333,6 +347,79 @@ static void vfio_user_cb(void *opaque)
>   * Functions called by main or CPU threads
>   */
>  
> +static void vfio_user_send_recv(VFIOProxy *proxy, VFIOUserHdr *msg,
> +                                VFIOUserFDs *fds, int rsize, int flags)
> +{
> +    VFIOUserReply *reply;
> +    bool iolock = 0;
> +
> +    if (msg->flags & VFIO_USER_NO_REPLY) {
> +        error_printf("vfio_user_send_recv on async message\n");
> +        return;
> +    }
> +
> +    /*
> +     * We may block later, so use a per-proxy lock and let
> +     * the iothreads run while we sleep unless told no to

s/no/not/

> +int vfio_user_validate_version(VFIODevice *vbasedev, Error **errp)
> +{
> +    g_autofree VFIOUserVersion *msgp;
> +    GString *caps;
> +    int size, caplen;
> +
> +    caps = caps_json();
> +    caplen = caps->len + 1;
> +    size = sizeof(*msgp) + caplen;
> +    msgp = g_malloc0(size);
> +
> +    vfio_user_request_msg(&msgp->hdr, VFIO_USER_VERSION, size, 0);
> +    msgp->major = VFIO_USER_MAJOR_VER;
> +    msgp->minor = VFIO_USER_MINOR_VER;
> +    memcpy(&msgp->capabilities, caps->str, caplen);
> +    g_string_free(caps, true);
> +
> +    vfio_user_send_recv(vbasedev->proxy, &msgp->hdr, NULL, 0, 0);
> +    if (msgp->hdr.flags & VFIO_USER_ERROR) {
> +        error_setg_errno(errp, msgp->hdr.error_reply, "version reply");
> +        return -1;
> +    }
> +
> +    if (msgp->major != VFIO_USER_MAJOR_VER ||
> +        msgp->minor > VFIO_USER_MINOR_VER) {
> +        error_setg(errp, "incompatible server version");
> +        return -1;
> +    }
> +    if (caps_check(msgp->minor, (char *)msgp + sizeof(*msgp), errp) != 0) {

The reply is untrusted so we cannot treat it as a NUL-terminated string
yet. The final byte msgp->capabilities[] needs to be checked first.

Please be careful about input validation, I might miss something so it's
best if you audit the patches too. QEMU must not trust the device
emulation process and vice versa.

> +        return -1;
> +    }
> +
> +    return 0;
> +}
> -- 
> 2.25.1
>

signature.asc
Description: PGP signature