Virtualization - Feb 2023 - [PATCH v3] drm/virtio: Add option to disable KMS support

On Mon, Feb 27, 2023 at 9:57 AM Dmitry Osipenko
<dmitry.osipenko at 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

Hi

Am 27.02.23 um 19:15 schrieb Rob Clark:
> On Mon, Feb 27, 2023 at 9:57 AM Dmitry Osipenko
> <dmitry.osipenko at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL:
<http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20230228/652bd4ce/attachment-0001.sig>