linux-kernel - Re: [RFC] drm: Allow DRM_IOCTL_MODE_MAP

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 1 Aug 2018 16:24:55 +0200
From:   Martin Fuzzey <martin.fuzzey@...wbird.group>
To:     robert.foss@...labora.com
Cc:     airlied@...ux.ie, brianp@...are.com,
        dri-devel@...ts.freedesktop.org, emil.l.velikov@...il.com,
        eric.engestrom@...el.com, gustavo@...ovan.org,
        linux-kernel@...r.kernel.org, maarten.lankhorst@...ux.intel.com,
        norvez@...omium.org, robh@...nel.org, seanpaul@...omium.org,
        tfiga@...omium.org, tomeu.vizoso@...labora.com
Subject: Re: [RFC] drm: Allow DRM_IOCTL_MODE_MAP_DUMB for render nodes

Hi,

I am running into the same problem using etnaviv on i.MX6 under Android 8.1

The mesa etnaviv code uses CREATE_DUMB / MAP_DUMB for the scanout buffers
and is called from the surfaceflinger process.

But, under Android 8.1 drm_hwcomposer runs in a seperate process to surfaceflinger.
Since drm_hwcomposer needs to use the KMS API it must be the DRM master,
that means that surface flinger cannot be DRM master too.

Adding a render node to the imx drm device and configuring mesa to user fixes
the problem but requires the render node to have access rights to use CREATE_DUMB
/ MAP_DUMB.


Here is my full patch:

Make imx-drm export a render node so that mesa can use it to allocate

From: Martin Fuzzey <martin.fuzzey@...wbird.group>
Date: 2018-07-26 15:37:28 +0200

dumb memory rather than requiring the master node.

The problem with using the master node is that, on android 8.1, drm_hwcomposer
is a seperate process and must be the master node (to use the KMS API),
since only a single process may be master surfaceflinger cannot be master too.

With this change surfaceflinger can use just a rendernode.

Note that we also have to modify the permissions table to allow render nodes
to use the dumb allocation functions. This is a hack and unlikely to be accepted
upstream but it's better than the huge security hole of allowing everything we were
using before.

Need to discuss an upstream acceptable way for this...
---
  drivers/gpu/drm/drm_ioctl.c        |    6 +++---
  drivers/gpu/drm/imx/imx-drm-core.c |    2 +-
  2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index a9ae6dd..31c4c86 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -639,9 +639,9 @@ int drm_ioctl_permit(u32 flags, struct drm_file *file_priv)
  	DRM_IOCTL_DEF(DRM_IOCTL_MODE_RMFB, drm_mode_rmfb, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
  	DRM_IOCTL_DEF(DRM_IOCTL_MODE_PAGE_FLIP, drm_mode_page_flip_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
  	DRM_IOCTL_DEF(DRM_IOCTL_MODE_DIRTYFB, drm_mode_dirtyfb_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
-	DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATE_DUMB, drm_mode_create_dumb_ioctl, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
-	DRM_IOCTL_DEF(DRM_IOCTL_MODE_MAP_DUMB, drm_mode_mmap_dumb_ioctl, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
-	DRM_IOCTL_DEF(DRM_IOCTL_MODE_DESTROY_DUMB, drm_mode_destroy_dumb_ioctl, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
+	DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATE_DUMB, drm_mode_create_dumb_ioctl, DRM_CONTROL_ALLOW|DRM_UNLOCKED|DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF(DRM_IOCTL_MODE_MAP_DUMB, drm_mode_mmap_dumb_ioctl, DRM_CONTROL_ALLOW|DRM_UNLOCKED|DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF(DRM_IOCTL_MODE_DESTROY_DUMB, drm_mode_destroy_dumb_ioctl, DRM_CONTROL_ALLOW|DRM_UNLOCKED|DRM_RENDER_ALLOW),
  	DRM_IOCTL_DEF(DRM_IOCTL_MODE_OBJ_GETPROPERTIES, drm_mode_obj_get_properties_ioctl, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
  	DRM_IOCTL_DEF(DRM_IOCTL_MODE_OBJ_SETPROPERTY, drm_mode_obj_set_property_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
  	DRM_IOCTL_DEF(DRM_IOCTL_MODE_CURSOR2, drm_mode_cursor2_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
index f91cb72..0c34306 100644
--- a/drivers/gpu/drm/imx/imx-drm-core.c
+++ b/drivers/gpu/drm/imx/imx-drm-core.c
@@ -177,7 +177,7 @@ int imx_drm_encoder_parse_of(struct drm_device *drm,
  
  static struct drm_driver imx_drm_driver = {
  	.driver_features	= DRIVER_MODESET | DRIVER_GEM | DRIVER_PRIME |
-				  DRIVER_ATOMIC,
+				  DRIVER_ATOMIC | DRIVER_RENDER,
  	.lastclose		= imx_drm_driver_lastclose,
  	.gem_free_object_unlocked = drm_gem_cma_free_object,
  	.gem_vm_ops		= &drm_gem_cma_vm_ops,

If it is not acceptable to modify the access rights for the DUMB allocation functions how should this be done?

Best regards,

Martin