diff mbox series

[RFC,v2,07/16] vfio-user: get device info

Message ID d2c6a72f9b0b207bcb2c7fe49abe45854d4e017b.1629131628.git.elena.ufimtseva@oracle.com
State New
Headers show
Series vfio-user implementation | expand

Commit Message

Elena Ufimtseva Aug. 16, 2021, 4:42 p.m. UTC 
From: John Johnson <john.g.johnson@oracle.com>

Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
---
 hw/vfio/user-protocol.h | 13 +++++++++++++
 hw/vfio/user.h          |  1 +
 hw/vfio/pci.c           | 13 +++++++++++++
 hw/vfio/user.c          | 20 ++++++++++++++++++++
 4 files changed, 47 insertions(+)

Comments

Stefan Hajnoczi Aug. 24, 2021, 4:04 p.m. UTC | #1 
On Mon, Aug 16, 2021 at 09:42:40AM -0700, Elena Ufimtseva wrote:
> +int vfio_user_get_info(VFIODevice *vbasedev)
> +{
> +    VFIOUserDeviceInfo msg;
> +
> +    memset(&msg, 0, sizeof(msg));
> +    vfio_user_request_msg(&msg.hdr, VFIO_USER_DEVICE_GET_INFO, sizeof(msg), 0);
> +    msg.argsz = sizeof(struct vfio_device_info);
> +
> +    vfio_user_send_recv(vbasedev->proxy, &msg.hdr, NULL, 0, 0);
> +    if (msg.hdr.flags & VFIO_USER_ERROR) {
> +        return -msg.hdr.error_reply;
> +    }
> +
> +    vbasedev->num_irqs = msg.num_irqs;
> +    vbasedev->num_regions = msg.num_regions;
> +    vbasedev->flags = msg.flags;
> +    vbasedev->reset_works = !!(msg.flags & VFIO_DEVICE_FLAGS_RESET);

No input validation. I haven't checked what happens when num_irqs,
num_regions, or flags are bogus but it's a little concerning. Unlike
kernel VFIO, we do not trust these values.

Stefan
John Johnson Aug. 30, 2021, 3:11 a.m. UTC | #2 
> On Aug 24, 2021, at 9:04 AM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> 
> On Mon, Aug 16, 2021 at 09:42:40AM -0700, Elena Ufimtseva wrote:
>> +int vfio_user_get_info(VFIODevice *vbasedev)
>> +{
>> +    VFIOUserDeviceInfo msg;
>> +
>> +    memset(&msg, 0, sizeof(msg));
>> +    vfio_user_request_msg(&msg.hdr, VFIO_USER_DEVICE_GET_INFO, sizeof(msg), 0);
>> +    msg.argsz = sizeof(struct vfio_device_info);
>> +
>> +    vfio_user_send_recv(vbasedev->proxy, &msg.hdr, NULL, 0, 0);
>> +    if (msg.hdr.flags & VFIO_USER_ERROR) {
>> +        return -msg.hdr.error_reply;
>> +    }
>> +
>> +    vbasedev->num_irqs = msg.num_irqs;
>> +    vbasedev->num_regions = msg.num_regions;
>> +    vbasedev->flags = msg.flags;
>> +    vbasedev->reset_works = !!(msg.flags & VFIO_DEVICE_FLAGS_RESET);
> 
> No input validation. I haven't checked what happens when num_irqs,
> num_regions, or flags are bogus but it's a little concerning. Unlike
> kernel VFIO, we do not trust these values.
> 

	As in the last reply, vfio-user doesn’t know valid values
from invalid, so I need to re-work this so the PCI-specific code that
calls vfio-user_get_info() can test for invalid values.

							JJ
Stefan Hajnoczi Sept. 7, 2021, 1:54 p.m. UTC | #3 
On Mon, Aug 30, 2021 at 03:11:39AM +0000, John Johnson wrote:
> 
> 
> > On Aug 24, 2021, at 9:04 AM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > 
> > On Mon, Aug 16, 2021 at 09:42:40AM -0700, Elena Ufimtseva wrote:
> >> +int vfio_user_get_info(VFIODevice *vbasedev)
> >> +{
> >> +    VFIOUserDeviceInfo msg;
> >> +
> >> +    memset(&msg, 0, sizeof(msg));
> >> +    vfio_user_request_msg(&msg.hdr, VFIO_USER_DEVICE_GET_INFO, sizeof(msg), 0);
> >> +    msg.argsz = sizeof(struct vfio_device_info);
> >> +
> >> +    vfio_user_send_recv(vbasedev->proxy, &msg.hdr, NULL, 0, 0);
> >> +    if (msg.hdr.flags & VFIO_USER_ERROR) {
> >> +        return -msg.hdr.error_reply;
> >> +    }
> >> +
> >> +    vbasedev->num_irqs = msg.num_irqs;
> >> +    vbasedev->num_regions = msg.num_regions;
> >> +    vbasedev->flags = msg.flags;
> >> +    vbasedev->reset_works = !!(msg.flags & VFIO_DEVICE_FLAGS_RESET);
> > 
> > No input validation. I haven't checked what happens when num_irqs,
> > num_regions, or flags are bogus but it's a little concerning. Unlike
> > kernel VFIO, we do not trust these values.
> > 
> 
> 	As in the last reply, vfio-user doesn’t know valid values
> from invalid, so I need to re-work this so the PCI-specific code that
> calls vfio-user_get_info() can test for invalid values.

Sounds good. I won't look further for missing input validation in the
VFIO message contents in this revision of the patch series. Once you're
happy with input validation I'll look at the code from this angle again.

Stefan
diff mbox series

Patch

diff --git a/hw/vfio/user-protocol.h b/hw/vfio/user-protocol.h
index 14b762d1ad..13e44ebf1c 100644
--- a/hw/vfio/user-protocol.h
+++ b/hw/vfio/user-protocol.h
@@ -82,4 +82,17 @@  typedef struct {
 #define VFIO_USER_MAX_MAX_XFER  (64 * 1024 * 1024)
 
 
+/*
+ * VFIO_USER_DEVICE_GET_INFO
+ * imported from struct_device_info
+ */
+typedef struct {
+    VFIOUserHdr hdr;
+    uint32_t argsz;
+    uint32_t flags;
+    uint32_t num_regions;
+    uint32_t num_irqs;
+    uint32_t cap_offset;
+} VFIOUserDeviceInfo;
+
 #endif /* VFIO_USER_PROTOCOL_H */
diff --git a/hw/vfio/user.h b/hw/vfio/user.h
index cab957ba7a..82044e7e78 100644
--- a/hw/vfio/user.h
+++ b/hw/vfio/user.h
@@ -71,5 +71,6 @@  void vfio_user_set_reqhandler(VFIODevice *vbasdev,
                                              void *reqarg);
 void vfio_user_send_reply(VFIOProxy *proxy, char *buf, int ret);
 int vfio_user_validate_version(VFIODevice *vbasedev, Error **errp);
+int vfio_user_get_info(VFIODevice *vbasedev);
 
 #endif /* VFIO_USER_H */
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index eae33e746f..63aa2441f0 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3369,6 +3369,7 @@  static void vfio_user_pci_realize(PCIDevice *pdev, Error **errp)
     VFIODevice *vbasedev = &vdev->vbasedev;
     SocketAddress addr;
     VFIOProxy *proxy;
+    int ret;
     Error *err = NULL;
 
     /*
@@ -3410,6 +3411,18 @@  static void vfio_user_pci_realize(PCIDevice *pdev, Error **errp)
     vbasedev->no_mmap = false;
     vbasedev->ops = &vfio_user_pci_ops;
 
+    ret = vfio_user_get_info(&vdev->vbasedev);
+    if (ret) {
+        error_setg_errno(errp, -ret, "get info failure");
+        goto error;
+    }
+
+    vfio_populate_device(vdev, &err);
+    if (err) {
+        error_propagate(errp, err);
+        goto error;
+    }
+
 error:
     vfio_user_disconnect(proxy);
     error_prepend(errp, VFIO_MSG_PREFIX, vdev->vbasedev.name);
diff --git a/hw/vfio/user.c b/hw/vfio/user.c
index e89464a571..b584b8e0f2 100644
--- a/hw/vfio/user.c
+++ b/hw/vfio/user.c
@@ -714,3 +714,23 @@  int vfio_user_validate_version(VFIODevice *vbasedev, Error **errp)
 
     return 0;
 }
+
+int vfio_user_get_info(VFIODevice *vbasedev)
+{
+    VFIOUserDeviceInfo msg;
+
+    memset(&msg, 0, sizeof(msg));
+    vfio_user_request_msg(&msg.hdr, VFIO_USER_DEVICE_GET_INFO, sizeof(msg), 0);
+    msg.argsz = sizeof(struct vfio_device_info);
+
+    vfio_user_send_recv(vbasedev->proxy, &msg.hdr, NULL, 0, 0);
+    if (msg.hdr.flags & VFIO_USER_ERROR) {
+        return -msg.hdr.error_reply;
+    }
+
+    vbasedev->num_irqs = msg.num_irqs;
+    vbasedev->num_regions = msg.num_regions;
+    vbasedev->flags = msg.flags;
+    vbasedev->reset_works = !!(msg.flags & VFIO_DEVICE_FLAGS_RESET);
+    return 0;
+}