drm: fix DRM_IOCTL_MODE_GETFB handle-leak

DRM_IOCTL_MODE_GETFB is used to retrieve information about a given framebuffer ID. It is a read-only helper and was thus declassified for unprivileged access in:

commit a14b1b42477c5ef089fcda88cbaae50d979eb8f9
Author: Mandeep Singh Baines <[email protected]>
Date: Fri Jan 20 12:11:16 2012 -0800

drm: remove master fd restriction on mode setting getters

However, beside width, height and stride information, DRM_IOCTL_MODE_GETFB also passes back a handle to the underlying gem-bo of the framebuffer. This handle allows users to mmap() it and read or write into it. Obviously, this should be restricted to DRM-Master.

With the current setup, *any* process with access to /dev/dri/card0 (which means any process with access to hardware-accelerated rendering) can access the current screen framebuffer and modify it ad libitum.

For backwards-compatibility reasons we want to keep the DRM_IOCTL_MODE_GETFB call unprivileged. Besides, it provides quite useful information regarding screen setup. So we simply test whether the caller is the current DRM-Master and if not, we return 0 as handle, which is always invalid. A following DRM_IOCTL_GEM_CLOSE on this handle will fail with EINVAL, but we accept this. Users shouldn't test for errors during GEM_CLOSE, anyway. And it is still better than a failing MODE_GETFB call.

---
 drivers/gpu/drm/drm_crtc.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

index 54b4169..096e170 100644
@@ -2598,10 +2598,22 @@ int drm_mode_getfb(struct drm_device *dev,
 	r->depth = fb->depth;
 	r->bpp = fb->bits_per_pixel;
 	r->pitch = fb->pitches[0];
-	if (fb->funcs->create_handle)
-		ret = fb->funcs->create_handle(fb, file_priv, &r->handle);
-	else
+	if (fb->funcs->create_handle) {
+		if (file_priv->is_master) {
+			ret = fb->funcs->create_handle(fb, file_priv,
+						       &r->handle);
+		} else {
+			/* GET_FB() is an unprivileged ioctl so we must not
+			 * return a buffer-handle to non-master processes! For
+			 * backwards-compatibility reasons, we cannot make
+			 * GET_FB() privileged, so just return an invalid handle
+			 * for non-masters. */
+			r->handle = 0;
+			ret = 0;
+		}
+	} else {
 		ret = -ENODEV;
+	}
 
 	drm_framebuffer_unreference(fb);
 

1.8.4

Hmm, I do have userspace that takes advantage of this already for reads. So if you can make it a DRM_MASTER || cappable(SYS_ADMID) privileged operation instead, I can live with that.
-Chris

Chris Wilson, Intel Open Source Technology Centre

DRM_IOCTL_MODE_GETFB is used to retrieve information about a given framebuffer ID. It is a read-only helper and was thus declassified for unprivileged access in:

commit a14b1b42477c5ef089fcda88cbaae50d979eb8f9
Author: Mandeep Singh Baines <[email protected]>
Date: Fri Jan 20 12:11:16 2012 -0800

drm: remove master fd restriction on mode setting getters

However, alongside width, height and stride information, DRM_IOCTL_MODE_GETFB also passes back a handle to the underlying buffer of the framebuffer. This handle allows users to mmap() it and read or write into it. Obviously, this should be restricted to DRM-Master.

With the current setup, *any* process with access to /dev/dri/card0 (which means any process with access to hardware-accelerated rendering) can access the current screen framebuffer and modify it ad libitum.

For backwards-compatibility reasons we want to keep the DRM_IOCTL_MODE_GETFB call unprivileged. Besides, it provides quite useful information regarding screen setup. So we simply test whether the caller is the current DRM-Master and if not, we return 0 as handle, which is always invalid. A following DRM_IOCTL_GEM_CLOSE on this handle will fail with EINVAL, but we accept this. Users shouldn't test for errors during GEM_CLOSE, anyway. And it is still better as a failing MODE_GETFB call.

v2: add capable(CAP_SYS_ADMIN) check for compatibility with i-g-t

---
 drivers/gpu/drm/drm_crtc.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

index 54b4169..a05e878 100644
@@ -2598,10 +2598,22 @@ int drm_mode_getfb(struct drm_device *dev,
 	r->depth = fb->depth;
 	r->bpp = fb->bits_per_pixel;
 	r->pitch = fb->pitches[0];
-	if (fb->funcs->create_handle)
-		ret = fb->funcs->create_handle(fb, file_priv, &r->handle);
-	else
+	if (fb->funcs->create_handle) {
+		if (file_priv->is_master || capable(CAP_SYS_ADMIN)) {
+			ret = fb->funcs->create_handle(fb, file_priv,
+						       &r->handle);
+		} else {
+			/* GET_FB() is an unprivileged ioctl so we must not
+			 * return a buffer-handle to non-master processes! For
+			 * backwards-compatibility reasons, we cannot make
+			 * GET_FB() privileged, so just return an invalid handle
+			 * for non-masters. */
+			r->handle = 0;
+			ret = 0;
+		}
+	} else {
 		ret = -ENODEV;
+	}
 
 	drm_framebuffer_unreference(fb);
 

1.8.4

-Chris

Chris Wilson, Intel Open Source Technology Centre