[PATCH xf86-video-ati 0/6] Harden against other DRM masters

[PATCH xf86-video-ati 0/6] Harden against other DRM masters

[Michel Dänzer]

From: Michel Dänzer <michel.daenzer@amd.com>

While our VT is inactive, so we aren't DRM master, other processes can
become DRM master. A DRM master can access any KMS framebuffer (FB) by
guessing its handle (in practice, it should be easy to find all existing
FBs by brute-forcing through a relatively small number of handles).

This series makes us destroy all FBs created by this driver before
leaving our VT, except for an all-black one created especially for this
purpose. This closes a long-standing potential information leak, which
was made worse by reference-counting the FBs we create.

Patches 1-4 are preparatory. The meat is in patch 5. Patch 6 removes a
function which is no longer used with patch 5.

Michel Dänzer (6):
  Create radeon_pixmap_clear helper
  Create drmmode_set_mode helper
  Create radeon_pixmap_get_fb_ptr helper
  Create radeon_master_screen helper
  Make all active CRTCs scan out an all-black framebuffer in LeaveVT
  Remove drmmode_scanout_free

 src/drmmode_display.c  | 112 +++++++++++++++++++++++--------------------------
 src/drmmode_display.h  |   8 +++-
 src/radeon.h           |  67 ++++++++++++++++-------------
 src/radeon_bo_helper.c |  21 ++++++++++
 src/radeon_bo_helper.h |   3 ++
 src/radeon_kms.c       |  98 ++++++++++++++++++++++++++++++++++++++++---
 6 files changed, 214 insertions(+), 95 deletions(-)

-- 
2.14.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH xf86-video-ati 1/6] Create radeon_pixmap_clear helper

[Michel Dänzer]

From: Michel Dänzer <michel.daenzer@amd.com>

Preparatory, no functional change intended yet.

Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
---
 src/drmmode_display.c  | 16 +---------------
 src/radeon_bo_helper.c | 21 +++++++++++++++++++++
 src/radeon_bo_helper.h |  3 +++
 3 files changed, 25 insertions(+), 15 deletions(-)

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index f926bc018..387d9e094 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -2211,9 +2211,6 @@ drmmode_xf86crtc_resize (ScrnInfoPtr scrn, int width, int height)
 	uint32_t tiling_flags = 0, base_align;
 	PixmapPtr ppix = screen->GetScreenPixmap(screen);
 	void *fb_shadow;
-	xRectangle rect;
-	Bool force;
-	GCPtr gc;
 
 	if (scrn->virtualX == width && scrn->virtualY == height)
 		return TRUE;
@@ -2356,18 +2353,7 @@ drmmode_xf86crtc_resize (ScrnInfoPtr scrn, int width, int height)
 			goto fail;
 	}
 
-	/* Clear new buffer */
-	gc = GetScratchGC(ppix->drawable.depth, scrn->pScreen);
-	force = info->accel_state->force;
-	info->accel_state->force = TRUE;
-	ValidateGC(&ppix->drawable, gc);
-	rect.x = 0;
-	rect.y = 0;
-	rect.width = width;
-	rect.height = height;
-	(*gc->ops->PolyFillRect)(&ppix->drawable, gc, 1, &rect);
-	FreeScratchGC(gc);
-	info->accel_state->force = force;
+	radeon_pixmap_clear(ppix);
 	radeon_cs_flush_indirect(scrn);
 	radeon_bo_wait(info->front_bo);
 
diff --git a/src/radeon_bo_helper.c b/src/radeon_bo_helper.c
index a8ba76185..01b9e3df5 100644
--- a/src/radeon_bo_helper.c
+++ b/src/radeon_bo_helper.c
@@ -195,6 +195,27 @@ radeon_alloc_pixmap_bo(ScrnInfoPtr pScrn, int width, int height, int depth,
     return bo;
 }
 
+/* Clear the pixmap contents to black */
+void
+radeon_pixmap_clear(PixmapPtr pixmap)
+{
+    ScreenPtr screen = pixmap->drawable.pScreen;
+    RADEONInfoPtr info = RADEONPTR(xf86ScreenToScrn(screen));
+    GCPtr gc = GetScratchGC(pixmap->drawable.depth, screen);
+    Bool force = info->accel_state->force;
+    xRectangle rect;
+
+    info->accel_state->force = TRUE;
+    ValidateGC(&pixmap->drawable, gc);
+    rect.x = 0;
+    rect.y = 0;
+    rect.width = pixmap->drawable.width;
+    rect.height = pixmap->drawable.height;
+    gc->ops->PolyFillRect(&pixmap->drawable, gc, 1, &rect);
+    FreeScratchGC(gc);
+    info->accel_state->force = force;
+}
+
 /* Get GEM handle for the pixmap */
 Bool radeon_get_pixmap_handle(PixmapPtr pixmap, uint32_t *handle)
 {
diff --git a/src/radeon_bo_helper.h b/src/radeon_bo_helper.h
index 771342502..e1856adb1 100644
--- a/src/radeon_bo_helper.h
+++ b/src/radeon_bo_helper.h
@@ -28,6 +28,9 @@ radeon_alloc_pixmap_bo(ScrnInfoPtr pScrn, int width, int height, int depth,
 		       int usage_hint, int bitsPerPixel, int *new_pitch,
 		       struct radeon_surface *new_surface, uint32_t *new_tiling);
 
+extern void
+radeon_pixmap_clear(PixmapPtr pixmap);
+
 extern uint32_t
 radeon_get_pixmap_tiling_flags(PixmapPtr pPix);
 
-- 
2.14.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH xf86-video-ati 2/6] Create drmmode_set_mode helper

[Michel Dänzer]

From: Michel Dänzer <michel.daenzer@amd.com>

Preparatory, no functional change intended yet.

Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
---
 src/drmmode_display.c | 82 ++++++++++++++++++++++++++++++---------------------
 src/drmmode_display.h |  3 ++
 2 files changed, 52 insertions(+), 33 deletions(-)

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index 387d9e094..791d59986 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -860,6 +860,52 @@ drmmode_crtc_gamma_do_set(xf86CrtcPtr crtc, uint16_t *red, uint16_t *green,
 			    blue);
 }
 
+Bool
+drmmode_set_mode(xf86CrtcPtr crtc, struct drmmode_fb *fb, DisplayModePtr mode,
+		 int x, int y)
+{
+	ScrnInfoPtr scrn = crtc->scrn;
+	RADEONEntPtr pRADEONEnt = RADEONEntPriv(scrn);
+	xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(scrn);
+	drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
+	uint32_t *output_ids = calloc(sizeof(uint32_t), xf86_config->num_output);
+	int output_count = 0;
+	drmModeModeInfo kmode;
+	Bool ret;
+	int i;
+
+	if (!output_ids)
+		return FALSE;
+
+	for (i = 0; i < xf86_config->num_output; i++) {
+		xf86OutputPtr output = xf86_config->output[i];
+		drmmode_output_private_ptr drmmode_output = output->driver_private;
+
+		if (output->crtc != crtc)
+			continue;
+
+		output_ids[output_count] = drmmode_output->mode_output->connector_id;
+		output_count++;
+	}
+
+	drmmode_ConvertToKMode(scrn, &kmode, mode);
+
+	ret = drmModeSetCrtc(pRADEONEnt->fd,
+			     drmmode_crtc->mode_crtc->crtc_id,
+			     fb->handle, x, y, output_ids,
+			     output_count, &kmode) == 0;
+
+	if (ret) {
+		drmmode_fb_reference(pRADEONEnt->fd, &drmmode_crtc->fb, fb);
+	} else {
+		xf86DrvMsg(scrn->scrnIndex, X_ERROR,
+			   "failed to set mode: %s\n", strerror(errno));
+	}
+
+	free(output_ids);
+	return ret;
+}
+
 static Bool
 drmmode_set_mode_major(xf86CrtcPtr crtc, DisplayModePtr mode,
 		     Rotation rotation, int x, int y)
@@ -875,12 +921,9 @@ drmmode_set_mode_major(xf86CrtcPtr crtc, DisplayModePtr mode,
 	int saved_x, saved_y;
 	Rotation saved_rotation;
 	DisplayModeRec saved_mode;
-	uint32_t *output_ids = NULL;
-	int output_count = 0;
 	Bool ret = FALSE;
 	int i;
 	struct drmmode_fb *fb = NULL;
-	drmModeModeInfo kmode;
 
 	/* The root window contents may be undefined before the WindowExposures
 	 * hook is called for it, so bail if we get here before that
@@ -899,22 +942,6 @@ drmmode_set_mode_major(xf86CrtcPtr crtc, DisplayModePtr mode,
 		crtc->y = y;
 		crtc->rotation = rotation;
 
-		output_ids = calloc(sizeof(uint32_t), xf86_config->num_output);
-		if (!output_ids)
-			goto done;
-
-		for (i = 0; i < xf86_config->num_output; i++) {
-			xf86OutputPtr output = xf86_config->output[i];
-			drmmode_output_private_ptr drmmode_output;
-
-			if (output->crtc != crtc)
-				continue;
-
-			drmmode_output = output->driver_private;
-			output_ids[output_count] = drmmode_output->mode_output->connector_id;
-			output_count++;
-		}
-
 		if (!drmmode_handle_transform(crtc))
 			goto done;
 
@@ -925,8 +952,6 @@ drmmode_set_mode_major(xf86CrtcPtr crtc, DisplayModePtr mode,
 		drmmode_crtc_gamma_do_set(crtc, crtc->gamma_red, crtc->gamma_green,
 					  crtc->gamma_blue, crtc->gamma_size);
 
-		drmmode_ConvertToKMode(crtc->scrn, &kmode, mode);
-
 #ifdef RADEON_PIXMAP_SHARING
 		if (drmmode_crtc->prime_scanout_pixmap) {
 			drmmode_crtc_prime_scanout_update(crtc, mode, scanout_id,
@@ -967,17 +992,10 @@ drmmode_set_mode_major(xf86CrtcPtr crtc, DisplayModePtr mode,
 		drmmode_crtc_wait_pending_event(drmmode_crtc, pRADEONEnt->fd,
 						drmmode_crtc->flip_pending);
 
-		if (drmModeSetCrtc(pRADEONEnt->fd,
-				   drmmode_crtc->mode_crtc->crtc_id,
-				   fb->handle, x, y, output_ids,
-				   output_count, &kmode) != 0) {
-			xf86DrvMsg(crtc->scrn->scrnIndex, X_ERROR,
-				   "failed to set mode: %s\n", strerror(errno));
+		if (!drmmode_set_mode(crtc, fb, mode, x, y))
 			goto done;
-		} else {
-			ret = TRUE;
-			drmmode_fb_reference(pRADEONEnt->fd, &drmmode_crtc->fb, fb);
-		}
+
+		ret = TRUE;
 
 		if (pScreen)
 			xf86CrtcSetScreenSubpixelOrder(pScreen);
@@ -1032,8 +1050,6 @@ done:
 		}
 	}
 
-	free(output_ids);
-
 	return ret;
 }
 
diff --git a/src/drmmode_display.h b/src/drmmode_display.h
index a6db87f8e..00b5e8119 100644
--- a/src/drmmode_display.h
+++ b/src/drmmode_display.h
@@ -220,6 +220,9 @@ extern void drmmode_scanout_free(ScrnInfoPtr scrn);
 extern void drmmode_uevent_init(ScrnInfoPtr scrn, drmmode_ptr drmmode);
 extern void drmmode_uevent_fini(ScrnInfoPtr scrn, drmmode_ptr drmmode);
 
+Bool drmmode_set_mode(xf86CrtcPtr crtc, struct drmmode_fb *fb,
+		      DisplayModePtr mode, int x, int y);
+
 extern int drmmode_get_crtc_id(xf86CrtcPtr crtc);
 extern int drmmode_get_height_align(ScrnInfoPtr scrn, uint32_t tiling);
 extern int drmmode_get_pitch_align(ScrnInfoPtr scrn, int bpe, uint32_t tiling);
-- 
2.14.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH xf86-video-ati 3/6] Create radeon_pixmap_get_fb_ptr helper

[Michel Dänzer]

From: Michel Dänzer <michel.daenzer@amd.com>

Preparatory, no functional change intended yet.

Also inline radeon_pixmap_create_fb into radeon_pixmap_get_fb, since
there's only one call-site anymore.

Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
---
 src/radeon.h | 53 +++++++++++++++++++++++++++++------------------------
 1 file changed, 29 insertions(+), 24 deletions(-)

diff --git a/src/radeon.h b/src/radeon.h
index 71123c7c7..5ce9999ab 100644
--- a/src/radeon.h
+++ b/src/radeon.h
@@ -842,24 +842,10 @@ radeon_fb_create(int drm_fd, uint32_t width, uint32_t height, uint8_t depth,
     return NULL;
 }
 
-static inline struct drmmode_fb*
-radeon_pixmap_create_fb(int drm_fd, PixmapPtr pix)
-{
-    uint32_t handle;
-
-    if (!radeon_get_pixmap_handle(pix, &handle))
-	return NULL;
-
-    return radeon_fb_create(drm_fd, pix->drawable.width, pix->drawable.height,
-			    pix->drawable.depth, pix->drawable.bitsPerPixel,
-			    pix->devKind, handle);
-}
-
-static inline struct drmmode_fb*
-radeon_pixmap_get_fb(PixmapPtr pix)
+static inline struct drmmode_fb**
+radeon_pixmap_get_fb_ptr(PixmapPtr pix)
 {
     ScrnInfoPtr scrn = xf86ScreenToScrn(pix->drawable.pScreen);
-    RADEONEntPtr pRADEONEnt = RADEONEntPriv(scrn);
     RADEONInfoPtr info = RADEONPTR(scrn);
 
 #ifdef USE_GLAMOR
@@ -869,10 +855,7 @@ radeon_pixmap_get_fb(PixmapPtr pix)
 	if (!priv)
 	    return NULL;
 
-	if (!priv->fb)
-	    priv->fb = radeon_pixmap_create_fb(pRADEONEnt->fd, pix);
-
-	return priv->fb;
+	return &priv->fb;
     } else
 #endif
     if (info->accelOn)
@@ -883,15 +866,37 @@ radeon_pixmap_get_fb(PixmapPtr pix)
 	if (!driver_priv)
 	    return NULL;
 
-	if (!driver_priv->fb)
-	    driver_priv->fb = radeon_pixmap_create_fb(pRADEONEnt->fd, pix);
-
-	return driver_priv->fb;
+	return &driver_priv->fb;
     }
 
     return NULL;
 }
 
+static inline struct drmmode_fb*
+radeon_pixmap_get_fb(PixmapPtr pix)
+{
+    struct drmmode_fb **fb_ptr = radeon_pixmap_get_fb_ptr(pix);
+
+    if (!fb_ptr)
+	return NULL;
+
+    if (!*fb_ptr) {
+	uint32_t handle;
+
+	if (radeon_get_pixmap_handle(pix, &handle)) {
+	    ScrnInfoPtr scrn = xf86ScreenToScrn(pix->drawable.pScreen);
+	    RADEONEntPtr pRADEONEnt = RADEONEntPriv(scrn);
+
+	    *fb_ptr = radeon_fb_create(pRADEONEnt->fd, pix->drawable.width,
+				       pix->drawable.height, pix->drawable.depth,
+				       pix->drawable.bitsPerPixel, pix->devKind,
+				       handle);
+	}
+    }
+
+    return *fb_ptr;
+}
+
 #define CP_PACKET0(reg, n)						\
 	(RADEON_CP_PACKET0 | ((n) << 16) | ((reg) >> 2))
 #define CP_PACKET1(reg0, reg1)						\
-- 
2.14.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH xf86-video-ati 4/6] Create radeon_master_screen helper

[Michel Dänzer]

From: Michel Dänzer <michel.daenzer@amd.com>

Preparatory, no functional change intended yet.

Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
---
 src/radeon.h | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/src/radeon.h b/src/radeon.h
index 5ce9999ab..319565a1c 100644
--- a/src/radeon.h
+++ b/src/radeon.h
@@ -185,6 +185,15 @@ typedef enum {
 #define radeon_is_gpu_screen(screen) (screen)->isGPU
 #define radeon_is_gpu_scrn(scrn) (scrn)->is_gpu
 
+static inline ScreenPtr
+radeon_master_screen(ScreenPtr screen)
+{
+    if (screen->current_master)
+	return screen->current_master;
+
+    return screen;
+}
+
 static inline ScreenPtr
 radeon_dirty_master(PixmapDirtyUpdatePtr dirty)
 {
@@ -194,10 +203,7 @@ radeon_dirty_master(PixmapDirtyUpdatePtr dirty)
     ScreenPtr screen = dirty->src->drawable.pScreen;
 #endif
 
-    if (screen->current_master)
-	return screen->current_master;
-
-    return screen;
+    return radeon_master_screen(screen);
 }
 
 static inline Bool
-- 
2.14.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH xf86-video-ati 5/6] Make all active CRTCs scan out an all-black framebuffer in LeaveVT

[Michel Dänzer]

From: Michel Dänzer <michel.daenzer@amd.com>

And destroy all other FBs. This is so that other DRM masters can only
get access to this all-black FB, not to any other FB we created, while
we're switched away and not DRM master.

Fixes: 55e513b978b2 ("Use reference counting for tracking KMS
                      framebuffer lifetimes")
Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
---
 src/drmmode_display.c |  4 +--
 src/drmmode_display.h |  4 +++
 src/radeon_kms.c      | 98 +++++++++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 98 insertions(+), 8 deletions(-)

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index 791d59986..ba170ee64 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -536,7 +536,7 @@ drmmode_crtc_scanout_destroy(drmmode_ptr drmmode,
 	}
 }
 
-static void
+void
 drmmode_crtc_scanout_free(drmmode_crtc_private_ptr drmmode_crtc)
 {
 	drmmode_crtc_scanout_destroy(drmmode_crtc->drmmode,
@@ -558,7 +558,7 @@ drmmode_scanout_free(ScrnInfoPtr scrn)
 		drmmode_crtc_scanout_free(xf86_config->crtc[c]->driver_private);
 }
 
-static PixmapPtr
+PixmapPtr
 drmmode_crtc_scanout_create(xf86CrtcPtr crtc, struct drmmode_scanout *scanout,
 			    int width, int height)
 {
diff --git a/src/drmmode_display.h b/src/drmmode_display.h
index 00b5e8119..ace059dcc 100644
--- a/src/drmmode_display.h
+++ b/src/drmmode_display.h
@@ -216,6 +216,10 @@ extern Bool drmmode_setup_colormap(ScreenPtr pScreen, ScrnInfoPtr pScrn);
 extern void drmmode_crtc_scanout_destroy(drmmode_ptr drmmode,
 					 struct drmmode_scanout *scanout);
 extern void drmmode_scanout_free(ScrnInfoPtr scrn);
+void drmmode_crtc_scanout_free(drmmode_crtc_private_ptr drmmode_crtc);
+PixmapPtr drmmode_crtc_scanout_create(xf86CrtcPtr crtc,
+				      struct drmmode_scanout *scanout,
+				      int width, int height);
 
 extern void drmmode_uevent_init(ScrnInfoPtr scrn, drmmode_ptr drmmode);
 extern void drmmode_uevent_fini(ScrnInfoPtr scrn, drmmode_ptr drmmode);
diff --git a/src/radeon_kms.c b/src/radeon_kms.c
index ca2d36d04..5410c4208 100644
--- a/src/radeon_kms.c
+++ b/src/radeon_kms.c
@@ -32,6 +32,7 @@
 #include <sys/ioctl.h>
 /* Driver data structures */
 #include "radeon.h"
+#include "radeon_bo_helper.h"
 #include "radeon_drm_queue.h"
 #include "radeon_glamor.h"
 #include "radeon_reg.h"
@@ -1158,9 +1159,10 @@ static void RADEONBlockHandler_KMS(BLOCKHANDLER_ARGS_DECL)
     (*pScreen->BlockHandler) (BLOCKHANDLER_ARGS);
     pScreen->BlockHandler = RADEONBlockHandler_KMS;
 
-    if (!pScrn->vtSema) {
-	radeon_cs_flush_indirect(pScrn);
-
+    if (!xf86ScreenToScrn(radeon_master_screen(pScreen))->vtSema) {
+	/* Unreference the all-black FB created by RADEONLeaveVT_KMS. After
+	 * this, there should be no FB left created by this driver.
+	 */
 	for (c = 0; c < xf86_config->num_crtc; c++) {
 	    drmmode_crtc_private_ptr drmmode_crtc =
 		xf86_config->crtc[c]->driver_private;
@@ -2472,21 +2474,105 @@ Bool RADEONEnterVT_KMS(VT_FUNC_ARGS_DECL)
 }
 
 
+static void
+pixmap_unref_fb(void *value, XID id, void *cdata)
+{
+    PixmapPtr pixmap = value;
+    RADEONEntPtr pRADEONEnt = cdata;
+    struct drmmode_fb **fb_ptr = radeon_pixmap_get_fb_ptr(pixmap);
+
+    if (fb_ptr)
+	drmmode_fb_reference(pRADEONEnt->fd, fb_ptr, NULL);
+}
+
 void RADEONLeaveVT_KMS(VT_FUNC_ARGS_DECL)
 {
     SCRN_INFO_PTR(arg);
     RADEONInfoPtr  info  = RADEONPTR(pScrn);
+    RADEONEntPtr pRADEONEnt = RADEONEntPriv(pScrn);
+    ScreenPtr pScreen = pScrn->pScreen;
+    xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(pScrn);
+    struct drmmode_scanout black_scanout = { .pixmap = NULL, .bo = NULL };
+    xf86CrtcPtr crtc;
+    drmmode_crtc_private_ptr drmmode_crtc;
+    unsigned w = 0, h = 0;
+    int i;
 
     xf86DrvMsgVerb(pScrn->scrnIndex, X_INFO, RADEON_LOGLEVEL_DEBUG,
 		   "RADEONLeaveVT_KMS\n");
 
-    radeon_drop_drm_master(pScrn);
+    /* Compute maximum scanout dimensions of active CRTCs */
+    for (i = 0; i < xf86_config->num_crtc; i++) {
+	crtc = xf86_config->crtc[i];
+	drmmode_crtc = crtc->driver_private;
+
+	if (!drmmode_crtc->fb)
+	    continue;
+
+	w = max(w, crtc->mode.HDisplay);
+	h = max(h, crtc->mode.VDisplay);
+    }
+
+    /* Make all active CRTCs scan out from an all-black framebuffer */
+    if (w > 0 && h > 0) {
+	if (drmmode_crtc_scanout_create(crtc, &black_scanout, w, h)) {
+	    struct drmmode_fb *black_fb =
+		radeon_pixmap_get_fb(black_scanout.pixmap);
+
+	    radeon_pixmap_clear(black_scanout.pixmap);
+	    radeon_cs_flush_indirect(pScrn);
+	    radeon_bo_wait(black_scanout.bo);
+
+	    for (i = 0; i < xf86_config->num_crtc; i++) {
+		crtc = xf86_config->crtc[i];
+		drmmode_crtc = crtc->driver_private;
+
+		if (drmmode_crtc->fb) {
+		    if (black_fb) {
+			drmmode_set_mode(crtc, black_fb, &crtc->mode, 0, 0);
+		    } else {
+			drmModeSetCrtc(pRADEONEnt->fd,
+				       drmmode_crtc->mode_crtc->crtc_id, 0, 0,
+				       0, NULL, 0, NULL);
+			drmmode_fb_reference(pRADEONEnt->fd, &drmmode_crtc->fb,
+					     NULL);
+		    }
+
+		    if (pScrn->is_gpu) {
+			if (drmmode_crtc->scanout[0].pixmap)
+			    pixmap_unref_fb(drmmode_crtc->scanout[0].pixmap,
+					    None, pRADEONEnt);
+			if (drmmode_crtc->scanout[1].pixmap)
+			    pixmap_unref_fb(drmmode_crtc->scanout[1].pixmap,
+					    None, pRADEONEnt);
+		    } else {
+			drmmode_crtc_scanout_free(drmmode_crtc);
+		    }
+		}
+	    }
+	}
+    }
 
     xf86RotateFreeShadow(pScrn);
-    if (!pScrn->is_gpu)
-	drmmode_scanout_free(pScrn);
+    drmmode_crtc_scanout_destroy(&info->drmmode, &black_scanout);
+
+    /* Unreference FBs of all pixmaps. After this, the only FB remaining
+     * should be the all-black one being scanned out by active CRTCs
+     */
+    for (i = 0; i < currentMaxClients; i++) {
+	if (i > 0 &&
+	    (!clients[i] || clients[i]->clientState != ClientStateRunning))
+            continue;
+
+	FindClientResourcesByType(clients[i], RT_PIXMAP, pixmap_unref_fb,
+				  pRADEONEnt);
+    }
+    pixmap_unref_fb(pScreen->GetScreenPixmap(pScreen), None, pRADEONEnt);
 
     xf86_hide_cursors (pScrn);
+
+    radeon_drop_drm_master(pScrn);
+
     info->accel_state->XInited3D = FALSE;
     info->accel_state->engineMode = EXA_ENGINEMODE_UNKNOWN;
 
-- 
2.14.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-ati 5/6] Make all active CRTCs scan out an all-black framebuffer in LeaveVT

[Emil Velikov]

Hi Michel,

On 28 August 2017 at 10:23, Michel Dänzer <michel@daenzer.net> wrote:
> From: Michel Dänzer <michel.daenzer@amd.com>
>
> And destroy all other FBs. This is so that other DRM masters can only
> get access to this all-black FB, not to any other FB we created, while
> we're switched away and not DRM master.
>
Isn't the issue applicable overall - be that in X, wayland compositors, other?

IIRC the vmwgfx's kernel driver, which has extra locking [1] in order
to address that.
Would a similar approach like that be applicable for radeon/amdgpu?

I'm not saying that this is bad/not needed. Just wondering on a
"perfect" long term solution.
That is, unless I've lost the plot and the two are completely unrelated.

Thanks
Emil

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c?h=v4.13-rc7#n1054
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-ati 5/6] Make all active CRTCs scan out an all-black framebuffer in LeaveVT

[Michel Dänzer]

On 29/08/17 07:56 PM, Emil Velikov wrote:
> On 28 August 2017 at 10:23, Michel Dänzer <michel@daenzer.net> wrote:
>> From: Michel Dänzer <michel.daenzer@amd.com>
>>
>> And destroy all other FBs. This is so that other DRM masters can only
>> get access to this all-black FB, not to any other FB we created, while
>> we're switched away and not DRM master.
>>
> Isn't the issue applicable overall - be that in X, wayland compositors, other?

Potentially.


> IIRC the vmwgfx's kernel driver, which has extra locking [1] in order
> to address that.
> Would a similar approach like that be applicable for radeon/amdgpu?

I don't see how that's related.

The issue is that if the caller of DRM_IOCTL_MODE_GETFB is DRM master
(or root, or using a DRM control device node), the ioctl returns a valid
GEM handle for the framebuffer, so the caller can use the underlying
buffer arbitrarily. This is handled by core DRM code, there's nothing
kernel drivers can do about it.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-ati 5/6] Make all active CRTCs scan out an all-black framebuffer in LeaveVT

[Emil Velikov]

On 30 August 2017 at 09:04, Michel Dänzer <michel@daenzer.net> wrote:
> On 29/08/17 07:56 PM, Emil Velikov wrote:
>> On 28 August 2017 at 10:23, Michel Dänzer <michel@daenzer.net> wrote:
>>> From: Michel Dänzer <michel.daenzer@amd.com>
>>>
>>> And destroy all other FBs. This is so that other DRM masters can only
>>> get access to this all-black FB, not to any other FB we created, while
>>> we're switched away and not DRM master.
>>>
>> Isn't the issue applicable overall - be that in X, wayland compositors, other?
>
> Potentially.
>
Ack, ty.

>
>> IIRC the vmwgfx's kernel driver, which has extra locking [1] in order
>> to address that.
>> Would a similar approach like that be applicable for radeon/amdgpu?
>
> I don't see how that's related.
>
> The issue is that if the caller of DRM_IOCTL_MODE_GETFB is DRM master
> (or root, or using a DRM control device node), the ioctl returns a valid
> GEM handle for the framebuffer, so the caller can use the underlying
> buffer arbitrarily. This is handled by core DRM code, there's nothing
> kernel drivers can do about it.
>
Disclaimer: I'm might be a mile off.

vmgfx's vmw_master_check() which does the usual DRM master/control or
root checking.
There is a "Check if we were previously master, but now dropped" which
seems, perhaps too vaguely, related to what's happening here.

Admittedly I've not looked too closely at the locked_master and
associated ttm code (ttm_read_lock, etc).
But was under the impression that it is used to restrict access to the
[contents behind the] GEM handle, when DRM master is dropped.

That said, I'm silly - vmwgfx specifics in a radeon/ati thread is not
the right thing.

Thanks
Emil
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-ati 5/6] Make all active CRTCs scan out an all-black framebuffer in LeaveVT

[Michel Dänzer]

On 30/08/17 07:27 PM, Emil Velikov wrote:
> On 30 August 2017 at 09:04, Michel Dänzer <michel@daenzer.net> wrote:
>> On 29/08/17 07:56 PM, Emil Velikov wrote:
>>
>>> IIRC the vmwgfx's kernel driver, which has extra locking [1] in order
>>> to address that.
>>> Would a similar approach like that be applicable for radeon/amdgpu?
>>
>> I don't see how that's related.
>>
>> The issue is that if the caller of DRM_IOCTL_MODE_GETFB is DRM master
>> (or root, or using a DRM control device node), the ioctl returns a valid
>> GEM handle for the framebuffer, so the caller can use the underlying
>> buffer arbitrarily. This is handled by core DRM code, there's nothing
>> kernel drivers can do about it.
>>
> Disclaimer: I'm might be a mile off.
> 
> vmgfx's vmw_master_check() which does the usual DRM master/control or
> root checking.
> There is a "Check if we were previously master, but now dropped" which
> seems, perhaps too vaguely, related to what's happening here.
> 
> Admittedly I've not looked too closely at the locked_master and
> associated ttm code (ttm_read_lock, etc).
> But was under the impression that it is used to restrict access to the
> [contents behind the] GEM handle, when DRM master is dropped.

I'm not familiar with that code either, so I may be wrong, but AFAICT
it's kind of the opposite, allowing userspace which was previously DRM
master to do some things it otherwise wouldn't be allowed to do.

Whereas this change is about something userspace can do while it's DRM
master, and which cannot be disallowed per se without breaking userspace
(e.g. smooth bootsplash -> login screen -> user session transitions).


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH xf86-video-ati 6/6] Remove drmmode_scanout_free

[Michel Dänzer]

From: Michel Dänzer <michel.daenzer@amd.com>

Not used anymore.

Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
---
 src/drmmode_display.c | 10 ----------
 src/drmmode_display.h |  1 -
 2 files changed, 11 deletions(-)

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index ba170ee64..afb442414 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -548,16 +548,6 @@ drmmode_crtc_scanout_free(drmmode_crtc_private_ptr drmmode_crtc)
 		DamageDestroy(drmmode_crtc->scanout_damage);
 }
 
-void
-drmmode_scanout_free(ScrnInfoPtr scrn)
-{
-	xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(scrn);
-	int c;
-
-	for (c = 0; c < xf86_config->num_crtc; c++)
-		drmmode_crtc_scanout_free(xf86_config->crtc[c]->driver_private);
-}
-
 PixmapPtr
 drmmode_crtc_scanout_create(xf86CrtcPtr crtc, struct drmmode_scanout *scanout,
 			    int width, int height)
diff --git a/src/drmmode_display.h b/src/drmmode_display.h
index ace059dcc..8387279f6 100644
--- a/src/drmmode_display.h
+++ b/src/drmmode_display.h
@@ -215,7 +215,6 @@ extern Bool drmmode_setup_colormap(ScreenPtr pScreen, ScrnInfoPtr pScrn);
 
 extern void drmmode_crtc_scanout_destroy(drmmode_ptr drmmode,
 					 struct drmmode_scanout *scanout);
-extern void drmmode_scanout_free(ScrnInfoPtr scrn);
 void drmmode_crtc_scanout_free(drmmode_crtc_private_ptr drmmode_crtc);
 PixmapPtr drmmode_crtc_scanout_create(xf86CrtcPtr crtc,
 				      struct drmmode_scanout *scanout,
-- 
2.14.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH xf86-video-ati 0/6] Harden against other DRM masters

[Deucher, Alexander]

> -----Original Message-----
> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf
> Of Michel Dänzer
> Sent: Monday, August 28, 2017 5:24 AM
> To: amd-gfx@lists.freedesktop.org
> Subject: [PATCH xf86-video-ati 0/6] Harden against other DRM masters
> 
> From: Michel Dänzer <michel.daenzer@amd.com>
> 
> While our VT is inactive, so we aren't DRM master, other processes can
> become DRM master. A DRM master can access any KMS framebuffer (FB)
> by
> guessing its handle (in practice, it should be easy to find all existing
> FBs by brute-forcing through a relatively small number of handles).
> 
> This series makes us destroy all FBs created by this driver before
> leaving our VT, except for an all-black one created especially for this
> purpose. This closes a long-standing potential information leak, which
> was made worse by reference-counting the FBs we create.
> 
> Patches 1-4 are preparatory. The meat is in patch 5. Patch 6 removes a
> function which is no longer used with patch 5.

Series is:
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

> 
> Michel Dänzer (6):
>   Create radeon_pixmap_clear helper
>   Create drmmode_set_mode helper
>   Create radeon_pixmap_get_fb_ptr helper
>   Create radeon_master_screen helper
>   Make all active CRTCs scan out an all-black framebuffer in LeaveVT
>   Remove drmmode_scanout_free
> 
>  src/drmmode_display.c  | 112 +++++++++++++++++++++++-------------------
> -------
>  src/drmmode_display.h  |   8 +++-
>  src/radeon.h           |  67 ++++++++++++++++-------------
>  src/radeon_bo_helper.c |  21 ++++++++++
>  src/radeon_bo_helper.h |   3 ++
>  src/radeon_kms.c       |  98
> ++++++++++++++++++++++++++++++++++++++++---
>  6 files changed, 214 insertions(+), 95 deletions(-)
> 
> --
> 2.14.1
> 
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx