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

From: Thomas Zimmermann <tzimmermann@suse.de>
To: Rob Clark <robdclark@gmail.com>,
	Dmitry Osipenko <dmitry.osipenko@collabora.com>
Cc: Rob Clark <robdclark@chromium.org>,
	open list <linux-kernel@vger.kernel.org>,
	dri-devel@lists.freedesktop.org,
	Gurchetan Singh <gurchetansingh@chromium.org>,
	Gerd Hoffmann <kraxel@redhat.com>,
	Ryan Neph <ryanneph@chromium.org>,
	David Airlie <airlied@redhat.com>,
	"open list:VIRTIO GPU DRIVER"
	<virtualization@lists.linux-foundation.org>
Subject: Re: [PATCH v3] drm/virtio: Add option to disable KMS support
Date: Tue, 28 Feb 2023 13:34:52 +0100	[thread overview]
Message-ID: <fb70356e-4e13-1858-9e1a-e886f5918030@suse.de> (raw)
In-Reply-To: <CAF6AEGsT8_o+v0vzGu1nyh6Z82pj8FnGUdMFc0Lq+4OWoSjRBQ@mail.gmail.com>

[-- Attachment #1.1: Type: text/plain, Size: 2158 bytes --]

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

-- 
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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]