diff mbox series

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

Message ID 20230227173800.2809727-1-robdclark@gmail.com (mailing list archive)
State New, archived
Headers show
Series [v3] drm/virtio: Add option to disable KMS support | expand

Commit Message

Rob Clark Feb. 27, 2023, 5:38 p.m. UTC 
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.

v2: Use more if (IS_ENABLED(...))
v3: Also permit the host to advertise no scanouts

Signed-off-by: Rob Clark <robdclark@chromium.org>
---
 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(-)

Comments

Dmitry Osipenko Feb. 27, 2023, 5:57 p.m. UTC | #1 
On 2/27/23 20:38, Rob Clark wrote:
...
> +	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);

If it's now configurable by host, why do we need the
CONFIG_DRM_VIRTIO_GPU_KMS?
Rob Clark Feb. 27, 2023, 6:15 p.m. UTC | #2 
On Mon, Feb 27, 2023 at 9:57 AM Dmitry Osipenko
<dmitry.osipenko@collabora.com> wrote:
>
> On 2/27/23 20:38, Rob Clark wrote:
> ...
> > +     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);
>
> If it's now configurable by host, why do we need the
> CONFIG_DRM_VIRTIO_GPU_KMS?

Because a kernel config option makes it more obvious that
modeset/atomic ioctls are blocked.  Which makes it more obvious about
where any potential security issues apply and where fixes need to get
backported to.  The config option is the only thing _I_ want,
everything else is just a bonus to help other people's use-cases.

BR,
-R
Dmitry Osipenko Feb. 27, 2023, 6:44 p.m. UTC | #3 
On 2/27/23 20:38, Rob Clark wrote:
>  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)

I'd also replace the `#if defined()` with `#if IS_ENABLED()`, for
consistency.

Maybe won't hurt to expand the commit message a tad, emphasizing the
security aspect and telling about the new num_scanouts=0 behaviour.

I can change it all while applying, if Gerd is okay with this patch.

Othwerise, good to me:

Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Thomas Zimmermann Feb. 28, 2023, 12:34 p.m. UTC | #4 
Hi

Am 27.02.23 um 19:15 schrieb Rob Clark:
> On Mon, Feb 27, 2023 at 9:57 AM Dmitry Osipenko
> <dmitry.osipenko@collabora.com> wrote:
>>
>> On 2/27/23 20:38, Rob Clark wrote:
>> ...
>>> +     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);
>>
>> If it's now configurable by host, why do we need the
>> CONFIG_DRM_VIRTIO_GPU_KMS?
> 
> Because a kernel config option makes it more obvious that
> modeset/atomic ioctls are blocked.  Which makes it more obvious about
> where any potential security issues apply and where fixes need to get
> backported to.  The config option is the only thing _I_ want,
> everything else is just a bonus to help other people's use-cases.

I find this very vague. What's the security thread?

And if the config option is useful, shouldn't it be DRM-wide? The 
modesetting ioctl calls are shared among all drivers.

Best regards
Thomas

> 
> BR,
> -R
Gerd Hoffmann Feb. 28, 2023, 12:46 p.m. UTC | #5 
Hi,

> +		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");

That error message isn't very clear.

Also I'd suggest to just drop the edid check.  It's about commands
being supported by the device, not about the actual presence of an EDID
blob, so the check doesn't look very useful to me.

take care,
  Gerd
Thomas Zimmermann Feb. 28, 2023, 12:47 p.m. UTC | #6 
Am 28.02.23 um 13:34 schrieb Thomas Zimmermann:
> Hi
> 
> Am 27.02.23 um 19:15 schrieb Rob Clark:
>> On Mon, Feb 27, 2023 at 9:57 AM Dmitry Osipenko
>> <dmitry.osipenko@collabora.com> wrote:
>>>
>>> On 2/27/23 20:38, Rob Clark wrote:
>>> ...
>>>> +     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);
>>>
>>> If it's now configurable by host, why do we need the
>>> CONFIG_DRM_VIRTIO_GPU_KMS?
>>
>> Because a kernel config option makes it more obvious that
>> modeset/atomic ioctls are blocked.  Which makes it more obvious about
>> where any potential security issues apply and where fixes need to get
>> backported to.  The config option is the only thing _I_ want,
>> everything else is just a bonus to help other people's use-cases.
> 
> I find this very vague. What's the security thread?
> 
> And if the config option is useful, shouldn't it be DRM-wide? The 
> modesetting ioctl calls are shared among all drivers.

For reference, here's an older discussion about render-only devices.

https://lore.kernel.org/dri-devel/20221011110437.15258-1-christian.koenig@amd.com/

> 
> Best regards
> Thomas
> 
>>
>> BR,
>> -R
>
Rob Clark Feb. 28, 2023, 3:43 p.m. UTC | #7 
On Tue, Feb 28, 2023 at 4:34 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi
>
> Am 27.02.23 um 19:15 schrieb Rob Clark:
> > On Mon, Feb 27, 2023 at 9:57 AM Dmitry Osipenko
> > <dmitry.osipenko@collabora.com> wrote:
> >>
> >> On 2/27/23 20:38, Rob Clark wrote:
> >> ...
> >>> +     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);
> >>
> >> If it's now configurable by host, why do we need the
> >> CONFIG_DRM_VIRTIO_GPU_KMS?
> >
> > Because a kernel config option makes it more obvious that
> > modeset/atomic ioctls are blocked.  Which makes it more obvious about
> > where any potential security issues apply and where fixes need to get
> > backported to.  The config option is the only thing _I_ want,
> > everything else is just a bonus to help other people's use-cases.
>
> I find this very vague. What's the security thread?

The modeset ioctls are a big potential attack surface area.  Which in
the case of CrOS VM guests serves no legitimate purpose.  (kms is
unused in the guest, instead guest window surfaces are proxied to host
for composition alongside host window surfaces.)

There have been in the past potential security bugs (use-after-free,
etc) found in the kms ioctls.  We should assume that there will be
more in the future.  So it seems like simple common sense to want to
block unused ioctls.

> And if the config option is useful, shouldn't it be DRM-wide? The
> modesetting ioctl calls are shared among all drivers.

Maybe, if there is a use?  The situation of compositing guest windows
in the host seems a bit unique to virtgpu, which is why I went with a
config option specific to virtgpu.

BR,
-R

> Best regards
> Thomas
>
> >
> > BR,
> > -R
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Ivo Totev
diff mbox series

Patch

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: