| Message ID | 20230302233506.3146290-1-robdclark@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v6] drm/virtio: Add option to disable KMS support | expand |
On Thu, Mar 02, 2023 at 03:35:06PM -0800, 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 > v5: Make num_scanouts==0 and DRM_VIRTIO_GPU_KMS=n behave the same > v6: Drop conditionally building virtgpu_display.c and early-out of > it's init/fini fxns instead Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>
On 3/3/23 02:35, 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 > v5: Make num_scanouts==0 and DRM_VIRTIO_GPU_KMS=n behave the same > v6: Drop conditionally building virtgpu_display.c and early-out of > it's init/fini fxns instead > > Signed-off-by: Rob Clark <robdclark@chromium.org> > Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> > --- Applied to misc-next
On 3/3/23 02:35, Rob Clark wrote: > @@ -223,12 +223,15 @@ int virtio_gpu_init(struct virtio_device *vdev, struct drm_device *dev) > 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) || !vgdev->num_scanouts) { Unfortunately the IS_ENABLED check needs to be inverted here. Secondly, with the IS_ENABLED check fixed and CONFIG_DRM_VIRTIO_GPU_KMS disabled, kernel crashes with a NULL deref on boot after getting the VIRTIO_GPU_EVENT_DISPLAY from host. ================================================================== [ 0.870144] BUG: KASAN: null-ptr-deref in drm_kms_helper_hotplug_event+0x2b/0x50 [ 0.870588] Read of size 8 at addr 0000000000000010 by task kworker/0:1/14 [ 0.870588] [ 0.870588] CPU: 0 PID: 14 Comm: kworker/0:1 Not tainted 6.2.0-next-20230303+ #387 [ 0.871441] scsi host0: ahci [ 0.870588] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.16.0-0-gd239552ce722-prebuilt.qemu.org 04/01/2014 [ 0.870588] Workqueue: events virtio_gpu_dequeue_ctrl_func [ 0.870588] Call Trace: [ 0.870588] <TASK> [ 0.872841] scsi host1: ahci [ 0.870588] dump_stack_lvl+0x46/0x70 [ 0.870588] kasan_report+0xbb/0xf0 [ 0.870588] ? drm_kms_helper_hotplug_event+0x2b/0x50 [ 0.870588] drm_kms_helper_hotplug_event+0x2b/0x50 [ 0.870588] virtio_gpu_dequeue_ctrl_func+0x143/0x500 [ 0.870588] ? lock_is_held_type+0xd8/0x130 [ 0.870588] ? virtio_gpu_free_vbufs+0x40/0x40 [ 0.875105] scsi host2: ahci [ 0.870588] process_one_work+0x4f5/0x9a0 [ 0.870588] ? pwq_dec_nr_in_flight+0x100/0x100 [ 0.870588] ? spin_bug+0xe0/0xe0 [ 0.870588] worker_thread+0x8c/0x610 [ 0.870588] ? process_one_work+0x9a0/0x9a0 [ 0.870588] kthread+0x15a/0x190 [ 0.870588] ? kthread_complete_and_exit+0x20/0x20 [ 0.870588] ret_from_fork+0x1f/0x30 [ 0.877201] scsi host3: ahci [ 0.870588] </TASK> [ 0.870588] ================================================================== I'll send a follow up fix.
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/virtgpu_display.c b/drivers/gpu/drm/virtio/virtgpu_display.c index 9ea7611a9e0f..ad924a8502e9 100644 --- a/drivers/gpu/drm/virtio/virtgpu_display.c +++ b/drivers/gpu/drm/virtio/virtgpu_display.c @@ -336,6 +336,9 @@ int virtio_gpu_modeset_init(struct virtio_gpu_device *vgdev) { int i, ret; + if (!vgdev->num_scanouts) + return 0; + ret = drmm_mode_config_init(vgdev->ddev); if (ret) return ret; @@ -362,6 +365,9 @@ void virtio_gpu_modeset_fini(struct virtio_gpu_device *vgdev) { int i; + if (!vgdev->num_scanouts) + return; + for (i = 0 ; i < vgdev->num_scanouts; ++i) kfree(vgdev->outputs[i].edid); } diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c index ae97b98750b6..add075681e18 100644 --- a/drivers/gpu/drm/virtio/virtgpu_drv.c +++ b/drivers/gpu/drm/virtio/virtgpu_drv.c @@ -172,6 +172,10 @@ MODULE_AUTHOR("Alon Levy"); DEFINE_DRM_GEM_FOPS(virtio_gpu_driver_fops); static const struct drm_driver driver = { + /* + * If KMS is disabled DRIVER_MODESET and DRIVER_ATOMIC are masked + * out via drm_device::driver_features: + */ .driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_RENDER | DRIVER_ATOMIC, .open = virtio_gpu_driver_open, .postclose = virtio_gpu_driver_postclose, diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c b/drivers/gpu/drm/virtio/virtgpu_kms.c index 27b7f14dae89..02e5c18c2c75 100644 --- a/drivers/gpu/drm/virtio/virtgpu_kms.c +++ b/drivers/gpu/drm/virtio/virtgpu_kms.c @@ -223,12 +223,15 @@ int virtio_gpu_init(struct virtio_device *vdev, struct drm_device *dev) 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) || !vgdev->num_scanouts) { + DRM_INFO("KMS disabled\n"); + vgdev->num_scanouts = 0; + vgdev->has_edid = false; + dev->driver_features &= ~(DRIVER_MODESET | DRIVER_ATOMIC); + } else { + 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 +249,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 (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: