From: Rob Clark <[email protected]>
Add a build option to disable modesetting support. This is useful in
cases where the guest only needs to use the GPU in a headless mode, or
(such as in the CrOS usage) window surfaces are proxied to a host
compositor.
As the modesetting ioctls are a big surface area for potential security
bugs to be found (it's happened in the past, we should assume it will
again in the future), it makes sense to have a build option to disable
those ioctls in cases where they serve no legitimate purpose.
v2: Use more if (IS_ENABLED(...))
v3: Also permit the host to advertise no scanouts
v4: Spiff out commit msg
Signed-off-by: Rob Clark <[email protected]>
Reviewed-by: Dmitry Osipenko <[email protected]>
---
drivers/gpu/drm/virtio/Kconfig | 11 +++++++
drivers/gpu/drm/virtio/Makefile | 5 +++-
drivers/gpu/drm/virtio/virtgpu_drv.c | 6 +++-
drivers/gpu/drm/virtio/virtgpu_drv.h | 10 +++++++
drivers/gpu/drm/virtio/virtgpu_kms.c | 44 ++++++++++++++++++----------
5 files changed, 59 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/virtio/Kconfig b/drivers/gpu/drm/virtio/Kconfig
index 51ec7c3240c9..ea06ff2aa4b4 100644
--- a/drivers/gpu/drm/virtio/Kconfig
+++ b/drivers/gpu/drm/virtio/Kconfig
@@ -11,3 +11,14 @@ config DRM_VIRTIO_GPU
QEMU based VMMs (like KVM or Xen).
If unsure say M.
+
+config DRM_VIRTIO_GPU_KMS
+ bool "Virtio GPU driver modesetting support"
+ depends on DRM_VIRTIO_GPU
+ default y
+ help
+ Enable modesetting support for virtio GPU driver. This can be
+ disabled in cases where only "headless" usage of the GPU is
+ required.
+
+ If unsure, say Y.
diff --git a/drivers/gpu/drm/virtio/Makefile b/drivers/gpu/drm/virtio/Makefile
index b99fa4a73b68..24c7ebe87032 100644
--- a/drivers/gpu/drm/virtio/Makefile
+++ b/drivers/gpu/drm/virtio/Makefile
@@ -4,8 +4,11 @@
# Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.
virtio-gpu-y := virtgpu_drv.o virtgpu_kms.o virtgpu_gem.o virtgpu_vram.o \
- virtgpu_display.o virtgpu_vq.o \
+ virtgpu_vq.o \
virtgpu_fence.o virtgpu_object.o virtgpu_debugfs.o virtgpu_plane.o \
virtgpu_ioctl.o virtgpu_prime.o virtgpu_trace_points.o
+virtio-gpu-$(CONFIG_DRM_VIRTIO_GPU_KMS) += \
+ virtgpu_display.o
+
obj-$(CONFIG_DRM_VIRTIO_GPU) += virtio-gpu.o
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c
index ae97b98750b6..9cb7d6dd3da6 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.c
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
@@ -172,7 +172,11 @@ MODULE_AUTHOR("Alon Levy");
DEFINE_DRM_GEM_FOPS(virtio_gpu_driver_fops);
static const struct drm_driver driver = {
- .driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_RENDER | DRIVER_ATOMIC,
+ .driver_features =
+#if defined(CONFIG_DRM_VIRTIO_GPU_KMS)
+ DRIVER_MODESET | DRIVER_ATOMIC |
+#endif
+ DRIVER_GEM | DRIVER_RENDER,
.open = virtio_gpu_driver_open,
.postclose = virtio_gpu_driver_postclose,
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
index af6ffb696086..ffe8faf67247 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -426,8 +426,18 @@ virtio_gpu_cmd_set_scanout_blob(struct virtio_gpu_device *vgdev,
uint32_t x, uint32_t y);
/* virtgpu_display.c */
+#if defined(CONFIG_DRM_VIRTIO_GPU_KMS)
int virtio_gpu_modeset_init(struct virtio_gpu_device *vgdev);
void virtio_gpu_modeset_fini(struct virtio_gpu_device *vgdev);
+#else
+static inline int virtio_gpu_modeset_init(struct virtio_gpu_device *vgdev)
+{
+ return 0;
+}
+static inline void virtio_gpu_modeset_fini(struct virtio_gpu_device *vgdev)
+{
+}
+#endif
/* virtgpu_plane.c */
uint32_t virtio_gpu_translate_format(uint32_t drm_fourcc);
diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c b/drivers/gpu/drm/virtio/virtgpu_kms.c
index 27b7f14dae89..1d888e309d6b 100644
--- a/drivers/gpu/drm/virtio/virtgpu_kms.c
+++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
@@ -161,7 +161,8 @@ int virtio_gpu_init(struct virtio_device *vdev, struct drm_device *dev)
if (virtio_has_feature(vgdev->vdev, VIRTIO_GPU_F_VIRGL))
vgdev->has_virgl_3d = true;
#endif
- if (virtio_has_feature(vgdev->vdev, VIRTIO_GPU_F_EDID)) {
+ if (IS_ENABLED(CONFIG_DRM_VIRTIO_GPU_KMS) &&
+ virtio_has_feature(vgdev->vdev, VIRTIO_GPU_F_EDID)) {
vgdev->has_edid = true;
}
if (virtio_has_feature(vgdev->vdev, VIRTIO_RING_F_INDIRECT_DESC)) {
@@ -218,17 +219,28 @@ int virtio_gpu_init(struct virtio_device *vdev, struct drm_device *dev)
goto err_vbufs;
}
- /* get display info */
- virtio_cread_le(vgdev->vdev, struct virtio_gpu_config,
- num_scanouts, &num_scanouts);
- vgdev->num_scanouts = min_t(uint32_t, num_scanouts,
- VIRTIO_GPU_MAX_SCANOUTS);
- if (!vgdev->num_scanouts) {
- DRM_ERROR("num_scanouts is zero\n");
- ret = -EINVAL;
- goto err_scanouts;
+ if (IS_ENABLED(CONFIG_DRM_VIRTIO_GPU_KMS)) {
+ /* get display info */
+ virtio_cread_le(vgdev->vdev, struct virtio_gpu_config,
+ num_scanouts, &num_scanouts);
+ vgdev->num_scanouts = min_t(uint32_t, num_scanouts,
+ VIRTIO_GPU_MAX_SCANOUTS);
+ if (!vgdev->num_scanouts) {
+ /*
+ * Having an EDID but no scanouts is non-sensical,
+ * but it is permitted to have no scanouts and no
+ * EDID (in which case DRIVER_MODESET and
+ * DRIVER_ATOMIC are not advertised)
+ */
+ if (vgdev->has_edid) {
+ DRM_ERROR("num_scanouts is zero\n");
+ ret = -EINVAL;
+ goto err_scanouts;
+ }
+ dev->driver_features &= ~(DRIVER_MODESET | DRIVER_ATOMIC);
+ }
+ DRM_INFO("number of scanouts: %d\n", num_scanouts);
}
- DRM_INFO("number of scanouts: %d\n", num_scanouts);
virtio_cread_le(vgdev->vdev, struct virtio_gpu_config,
num_capsets, &num_capsets);
@@ -246,10 +258,12 @@ int virtio_gpu_init(struct virtio_device *vdev, struct drm_device *dev)
virtio_gpu_get_capsets(vgdev, num_capsets);
if (vgdev->has_edid)
virtio_gpu_cmd_get_edids(vgdev);
- virtio_gpu_cmd_get_display_info(vgdev);
- virtio_gpu_notify(vgdev);
- wait_event_timeout(vgdev->resp_wq, !vgdev->display_info_pending,
- 5 * HZ);
+ if (IS_ENABLED(CONFIG_DRM_VIRTIO_GPU_KMS) && vgdev->num_scanouts) {
+ virtio_gpu_cmd_get_display_info(vgdev);
+ virtio_gpu_notify(vgdev);
+ wait_event_timeout(vgdev->resp_wq, !vgdev->display_info_pending,
+ 5 * HZ);
+ }
return 0;
err_scanouts:
--
2.39.1
Rob Clark <[email protected]> writes:
Hello Rob,
> From: Rob Clark <[email protected]>
>
> Add a build option to disable modesetting support. This is useful in
> cases where the guest only needs to use the GPU in a headless mode, or
> (such as in the CrOS usage) window surfaces are proxied to a host
> compositor.
>
> As the modesetting ioctls are a big surface area for potential security
> bugs to be found (it's happened in the past, we should assume it will
> again in the future), it makes sense to have a build option to disable
> those ioctls in cases where they serve no legitimate purpose.
>
> v2: Use more if (IS_ENABLED(...))
> v3: Also permit the host to advertise no scanouts
> v4: Spiff out commit msg
>
> Signed-off-by: Rob Clark <[email protected]>
> Reviewed-by: Dmitry Osipenko <[email protected]>
> ---
The patch looks good to me.
Reviewed-by: Javier Martinez Canillas <[email protected]>
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
On 2/28/23 18:54, Rob Clark wrote:
> From: Rob Clark <[email protected]>
>
> Add a build option to disable modesetting support. This is useful in
> cases where the guest only needs to use the GPU in a headless mode, or
> (such as in the CrOS usage) window surfaces are proxied to a host
> compositor.
>
> As the modesetting ioctls are a big surface area for potential security
> bugs to be found (it's happened in the past, we should assume it will
> again in the future), it makes sense to have a build option to disable
> those ioctls in cases where they serve no legitimate purpose.
>
> v2: Use more if (IS_ENABLED(...))
> v3: Also permit the host to advertise no scanouts
> v4: Spiff out commit msg
>
> Signed-off-by: Rob Clark <[email protected]>
> Reviewed-by: Dmitry Osipenko <[email protected]>
> ---
Gerd, to give you some context on the v4.. we've chatted a bit more on
the #dri-devel and concluded that config option is the most robust way
of having KMS disabled from a security stand point. We would also want
to have a per-driver option (and not global) because there are scenarios
of using passthrough GPU + virtio-gpu in a guest, hence we would only
want to toggle KMS for a particular driver.
--
Best regards,
Dmitry
On Wed, Mar 01, 2023 at 03:37:24AM +0300, Dmitry Osipenko wrote:
> On 2/28/23 18:54, Rob Clark wrote:
> > From: Rob Clark <[email protected]>
> >
> > Add a build option to disable modesetting support. This is useful in
> > cases where the guest only needs to use the GPU in a headless mode, or
> > (such as in the CrOS usage) window surfaces are proxied to a host
> > compositor.
> >
> > As the modesetting ioctls are a big surface area for potential security
> > bugs to be found (it's happened in the past, we should assume it will
> > again in the future), it makes sense to have a build option to disable
> > those ioctls in cases where they serve no legitimate purpose.
> >
> > v2: Use more if (IS_ENABLED(...))
> > v3: Also permit the host to advertise no scanouts
> > v4: Spiff out commit msg
> >
> > Signed-off-by: Rob Clark <[email protected]>
> > Reviewed-by: Dmitry Osipenko <[email protected]>
> > ---
>
> Gerd, to give you some context on the v4.. we've chatted a bit more on
> the #dri-devel and concluded that config option is the most robust way
> of having KMS disabled from a security stand point. We would also want
> to have a per-driver option (and not global) because there are scenarios
> of using passthrough GPU + virtio-gpu in a guest, hence we would only
> want to toggle KMS for a particular driver.
IMHO both ways options to disable the KMS bits should work the same way.
With the current patch modeset_init() runs with num_scanouts == 0 but
doesn't with CONFIG_KMS=n. There are also two different ways to tweak
driver_features. Can we get rid of that please, for robustness reasons?
I'd suggest to have a is_kms_enabled() helper function (probably best as
inline so gcc can figure it is constant false for CONFIG_KMS=n and throw
away unreachable code). Add "if (!is_kms_enabled()) return;" to
modeset_init() and modeset_fini() instead of stubbing them out. Use the
drm_device->driver_features override in both cases.
Also the edid check can go away. As already mentioned this is about a
device feature not a edid being present.
take care,
Gerd