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
