On Mon, Aug 30, 2021 at 03:08:50AM +0000, John Johnson wrote:
> > 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:
> >> +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.

Good point. Today's VFIO code trusts the replies because they come from
the kernel. Now that they can come from an untrusted source they must be
validated and common VFIO code may need to change its trust model.

Stefan