> On Aug 24, 2021, at 8:59 AM, Stefan Hajnoczi <stefa...@redhat.com> wrote:
>
> 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.
>
The return ended up in a later patch.
>> + 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/
>
OK
>>
>> /*
>> * 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/
OK
>
>> +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.
>
This message is consumed by vfio-user, so I can check for valid
replies, but for most messages this checking will have to be done at in
the VFIO common or bus-specific code, as vfio-user doens’t know valid
data from invalid.
JJ
>> + return -1;
>> + }
>> +
>> + return 0;
>> +}
>> --
>> 2.25.1
