| Message ID | 20230228155406.2881252-1-robdclark@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4] drm/virtio: Add option to disable KMS support | expand |
Rob Clark <robdclark@gmail.com> writes: Hello Rob, > From: Rob Clark <robdclark@chromium.org> > > 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 <robdclark@chromium.org> > Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > --- The patch looks good to me. Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
On 2/28/23 18:54, Rob Clark wrote: > From: Rob Clark <robdclark@chromium.org> > > 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 <robdclark@chromium.org> > Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > --- 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.
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 <robdclark@chromium.org> > > > > 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 <robdclark@chromium.org> > > Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > > --- > > 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
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: