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

[John Johnson]

> On Aug 24, 2021, at 8:59 AM, Stefan Hajnoczi <stefanha@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