Re: [PATCH v4] drm/virtio: Add option to disable KMS support

2023-02-28 Thread Gerd Hoffmann
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 
> > 
> > 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 
> > Reviewed-by: Dmitry Osipenko 
> > ---
> 
> 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



Re: [PATCH v4] drm/virtio: Add option to disable KMS support

2023-02-28 Thread Dmitry Osipenko
On 2/28/23 18:54, Rob Clark wrote:
> From: Rob Clark 
> 
> 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 
> Reviewed-by: Dmitry Osipenko 
> ---

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



Re: [PATCH v4] drm/virtio: Add option to disable KMS support

2023-02-28 Thread Javier Martinez Canillas
Rob Clark  writes:

Hello Rob,

> From: Rob Clark 
>
> 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 
> Reviewed-by: Dmitry Osipenko 
> ---

The patch looks good to me.

Reviewed-by: Javier Martinez Canillas 

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat



[PATCH v4] drm/virtio: Add option to disable KMS support

2023-02-28 Thread Rob Clark
From: Rob Clark 

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 
Reviewed-by: Dmitry Osipenko 
---
 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, _scanouts);
-   vgdev->num_scanouts = min_t(uint32_t, num_scanouts,
-