drm: fix a LOR issue on FreeBSD for savage driver

Correct a LOR issue on FreeBSD by allocating temporary space and doing a single
DRM_COPY_FROM_USER rather than DRM_VERIFYAREA_READ followed by tons of
DRM_COPY_FROM_USER_UNCHECKED.  I don't like the look of the temporary space
allocation, but I like the simplification in the rest of the file.  Tested
with glxgears, tuxracer, and q3 on a savage4.

From: Eric Anholt <anholt@freebsd.org>
Signed-off-by: Dave Airlie <airlied@linux.ie>

1 files changed, 164 insertions, 160 deletions

diff --git a/drivers/char/drm/savage_state.c b/drivers/char/drm/savage_state.c
index e87a5d59b99..ef2581d1614 100644
--- a/drivers/char/drm/savage_state.c
+++ b/drivers/char/drm/savage_state.c
@@ -27,7 +27,7 @@
 #include "savage_drv.h"
 
 void savage_emit_clip_rect_s3d(drm_savage_private_t * dev_priv,
-			       drm_clip_rect_t * pbox)
+			       const drm_clip_rect_t * pbox)
 {
 	uint32_t scstart = dev_priv->state.s3d.new_scstart;
 	uint32_t scend = dev_priv->state.s3d.new_scend;
@@ -53,7 +53,7 @@ void savage_emit_clip_rect_s3d(drm_savage_private_t * dev_priv,
 }
 
 void savage_emit_clip_rect_s4(drm_savage_private_t * dev_priv,
-			      drm_clip_rect_t * pbox)
+			      const drm_clip_rect_t * pbox)
 {
 	uint32_t drawctrl0 = dev_priv->state.s4.new_drawctrl0;
 	uint32_t drawctrl1 = dev_priv->state.s4.new_drawctrl1;
@@ -115,18 +115,19 @@ static int savage_verify_texaddr(drm_savage_private_t * dev_priv, int unit,
 
 #define SAVE_STATE(reg,where)			\
 	if(start <= reg && start+count > reg)	\
-		DRM_GET_USER_UNCHECKED(dev_priv->state.where, &regs[reg-start])
+		dev_priv->state.where = regs[reg - start]
 #define SAVE_STATE_MASK(reg,where,mask) do {			\
 	if(start <= reg && start+count > reg) {			\
 		uint32_t tmp;					\
-		DRM_GET_USER_UNCHECKED(tmp, &regs[reg-start]);	\
+		tmp = regs[reg - start];			\
 		dev_priv->state.where = (tmp & (mask)) |	\
 			(dev_priv->state.where & ~(mask));	\
 	}							\
 } while (0)
+
 static int savage_verify_state_s3d(drm_savage_private_t * dev_priv,
 				   unsigned int start, unsigned int count,
-				   const uint32_t __user * regs)
+				   const uint32_t *regs)
 {
 	if (start < SAVAGE_TEXPALADDR_S3D ||
 	    start + count - 1 > SAVAGE_DESTTEXRWWATERMARK_S3D) {
@@ -148,8 +149,7 @@ static int savage_verify_state_s3d(drm_savage_private_t * dev_priv,
 		SAVE_STATE(SAVAGE_TEXADDR_S3D, s3d.texaddr);
 		if (dev_priv->state.s3d.texctrl & SAVAGE_TEXCTRL_TEXEN_MASK)
 			return savage_verify_texaddr(dev_priv, 0,
-						     dev_priv->state.s3d.
-						     texaddr);
+						dev_priv->state.s3d.texaddr);
 	}
 
 	return 0;
@@ -157,7 +157,7 @@ static int savage_verify_state_s3d(drm_savage_private_t * dev_priv,
 
 static int savage_verify_state_s4(drm_savage_private_t * dev_priv,
 				  unsigned int start, unsigned int count,
-				  const uint32_t __user * regs)
+				  const uint32_t *regs)
 {
 	int ret = 0;
 
@@ -174,19 +174,18 @@ static int savage_verify_state_s4(drm_savage_private_t * dev_priv,
 			~SAVAGE_SCISSOR_MASK_S4);
 
 	/* if any texture regs were changed ... */
-	if (start <= SAVAGE_TEXDESCR_S4 && start + count > SAVAGE_TEXPALADDR_S4) {
+	if (start <= SAVAGE_TEXDESCR_S4 &&
+	    start + count > SAVAGE_TEXPALADDR_S4) {
 		/* ... check texture state */
 		SAVE_STATE(SAVAGE_TEXDESCR_S4, s4.texdescr);
 		SAVE_STATE(SAVAGE_TEXADDR0_S4, s4.texaddr0);
 		SAVE_STATE(SAVAGE_TEXADDR1_S4, s4.texaddr1);
 		if (dev_priv->state.s4.texdescr & SAVAGE_TEXDESCR_TEX0EN_MASK)
-			ret |=
-			    savage_verify_texaddr(dev_priv, 0,
-						  dev_priv->state.s4.texaddr0);
+			ret |= savage_verify_texaddr(dev_priv, 0,
+						dev_priv->state.s4.texaddr0);
 		if (dev_priv->state.s4.texdescr & SAVAGE_TEXDESCR_TEX1EN_MASK)
-			ret |=
-			    savage_verify_texaddr(dev_priv, 1,
-						  dev_priv->state.s4.texaddr1);
+			ret |= savage_verify_texaddr(dev_priv, 1,
+						dev_priv->state.s4.texaddr1);
 	}
 
 	return ret;
@@ -197,7 +196,7 @@ static int savage_verify_state_s4(drm_savage_private_t * dev_priv,
 
 static int savage_dispatch_state(drm_savage_private_t * dev_priv,
 				 const drm_savage_cmd_header_t * cmd_header,
-				 const uint32_t __user * regs)
+				 const uint32_t *regs)
 {
 	unsigned int count = cmd_header->state.count;
 	unsigned int start = cmd_header->state.start;
@@ -209,9 +208,6 @@ static int savage_dispatch_state(drm_savage_private_t * dev_priv,
 	if (!count)
 		return 0;
 
-	if (DRM_VERIFYAREA_READ(regs, count * 4))
-		return DRM_ERR(EFAULT);
-
 	if (S3_SAVAGE3D_SERIES(dev_priv->chipset)) {
 		ret = savage_verify_state_s3d(dev_priv, start, count, regs);
 		if (ret != 0)
@@ -236,8 +232,8 @@ static int savage_dispatch_state(drm_savage_private_t * dev_priv,
 		/* scissor regs are emitted in savage_dispatch_draw */
 		if (start < SAVAGE_DRAWCTRL0_S4) {
 			if (start + count > SAVAGE_DRAWCTRL1_S4 + 1)
-				count2 =
-				    count - (SAVAGE_DRAWCTRL1_S4 + 1 - start);
+				count2 = count -
+					 (SAVAGE_DRAWCTRL1_S4 + 1 - start);
 			if (start + count > SAVAGE_DRAWCTRL0_S4)
 				count = SAVAGE_DRAWCTRL0_S4 - start;
 		} else if (start <= SAVAGE_DRAWCTRL1_S4) {
@@ -263,7 +259,7 @@ static int savage_dispatch_state(drm_savage_private_t * dev_priv,
 		while (count > 0) {
 			unsigned int n = count < 255 ? count : 255;
 			DMA_SET_REGISTERS(start, n);
-			DMA_COPY_FROM_USER(regs, n);
+			DMA_COPY(regs, n);
 			count -= n;
 			start += n;
 			regs += n;
@@ -421,8 +417,8 @@ static int savage_dispatch_dma_prim(drm_savage_private_t * dev_priv,
 
 static int savage_dispatch_vb_prim(drm_savage_private_t * dev_priv,
 				   const drm_savage_cmd_header_t * cmd_header,
-				   const uint32_t __user * vtxbuf,
-				   unsigned int vb_size, unsigned int vb_stride)
+				   const uint32_t *vtxbuf, unsigned int vb_size,
+				   unsigned int vb_stride)
 {
 	unsigned char reorder = 0;
 	unsigned int prim = cmd_header->prim.prim;
@@ -507,8 +503,7 @@ static int savage_dispatch_vb_prim(drm_savage_private_t * dev_priv,
 
 			for (i = start; i < start + count; ++i) {
 				unsigned int j = i + reorder[i % 3];
-				DMA_COPY_FROM_USER(&vtxbuf[vb_stride * j],
-						   vtx_size);
+				DMA_COPY(&vtxbuf[vb_stride * j], vtx_size);
 			}
 
 			DMA_COMMIT();
@@ -517,13 +512,12 @@ static int savage_dispatch_vb_prim(drm_savage_private_t * dev_priv,
 			DMA_DRAW_PRIMITIVE(count, prim, skip);
 
 			if (vb_stride == vtx_size) {
-				DMA_COPY_FROM_USER(&vtxbuf[vb_stride * start],
-						   vtx_size * count);
+				DMA_COPY(&vtxbuf[vb_stride * start], 
+					 vtx_size * count);
 			} else {
 				for (i = start; i < start + count; ++i) {
-					DMA_COPY_FROM_USER(&vtxbuf
-							   [vb_stride * i],
-							   vtx_size);
+					DMA_COPY(&vtxbuf [vb_stride * i],
+						 vtx_size);
 				}
 			}
 
@@ -541,7 +535,7 @@ static int savage_dispatch_vb_prim(drm_savage_private_t * dev_priv,
 
 static int savage_dispatch_dma_idx(drm_savage_private_t * dev_priv,
 				   const drm_savage_cmd_header_t * cmd_header,
-				   const uint16_t __user * usr_idx,
+				   const uint16_t *idx,
 				   const drm_buf_t * dmabuf)
 {
 	unsigned char reorder = 0;
@@ -628,11 +622,8 @@ static int savage_dispatch_dma_idx(drm_savage_private_t * dev_priv,
 	while (n != 0) {
 		/* Can emit up to 255 indices (85 triangles) at once. */
 		unsigned int count = n > 255 ? 255 : n;
-		/* Is it ok to allocate 510 bytes on the stack in an ioctl? */
-		uint16_t idx[255];
 
-		/* Copy and check indices */
-		DRM_COPY_FROM_USER_UNCHECKED(idx, usr_idx, count * 2);
+		/* check indices */
 		for (i = 0; i < count; ++i) {
 			if (idx[i] > dmabuf->total / 32) {
 				DRM_ERROR("idx[%u]=%u out of range (0-%u)\n",
@@ -652,8 +643,8 @@ static int savage_dispatch_dma_idx(drm_savage_private_t * dev_priv,
 
 			for (i = 1; i + 1 < count; i += 2)
 				BCI_WRITE(idx[i + reorder[i % 3]] |
-					  (idx[i + 1 + reorder[(i + 1) % 3]] <<
-					   16));
+					  (idx[i + 1 +
+					   reorder[(i + 1) % 3]] << 16));
 			if (i < count)
 				BCI_WRITE(idx[i + reorder[i % 3]]);
 		} else if (S3_SAVAGE3D_SERIES(dev_priv->chipset)) {
@@ -674,7 +665,7 @@ static int savage_dispatch_dma_idx(drm_savage_private_t * dev_priv,
 				BCI_WRITE(idx[i]);
 		}
 
-		usr_idx += count;
+		idx += count;
 		n -= count;
 
 		prim |= BCI_CMD_DRAW_CONT;
@@ -685,8 +676,8 @@ static int savage_dispatch_dma_idx(drm_savage_private_t * dev_priv,
 
 static int savage_dispatch_vb_idx(drm_savage_private_t * dev_priv,
 				  const drm_savage_cmd_header_t * cmd_header,
-				  const uint16_t __user * usr_idx,
-				  const uint32_t __user * vtxbuf,
+				  const uint16_t *idx,
+				  const uint32_t *vtxbuf,
 				  unsigned int vb_size, unsigned int vb_stride)
 {
 	unsigned char reorder = 0;
@@ -751,11 +742,8 @@ static int savage_dispatch_vb_idx(drm_savage_private_t * dev_priv,
 	while (n != 0) {
 		/* Can emit up to 255 vertices (85 triangles) at once. */
 		unsigned int count = n > 255 ? 255 : n;
-		/* Is it ok to allocate 510 bytes on the stack in an ioctl? */
-		uint16_t idx[255];
-
-		/* Copy and check indices */
-		DRM_COPY_FROM_USER_UNCHECKED(idx, usr_idx, count * 2);
+		
+		/* Check indices */
 		for (i = 0; i < count; ++i) {
 			if (idx[i] > vb_size / (vb_stride * 4)) {
 				DRM_ERROR("idx[%u]=%u out of range (0-%u)\n",
@@ -775,8 +763,7 @@ static int savage_dispatch_vb_idx(drm_savage_private_t * dev_priv,
 
 			for (i = 0; i < count; ++i) {
 				unsigned int j = idx[i + reorder[i % 3]];
-				DMA_COPY_FROM_USER(&vtxbuf[vb_stride * j],
-						   vtx_size);
+				DMA_COPY(&vtxbuf[vb_stride * j], vtx_size);
 			}
 
 			DMA_COMMIT();
@@ -786,14 +773,13 @@ static int savage_dispatch_vb_idx(drm_savage_private_t * dev_priv,
 
 			for (i = 0; i < count; ++i) {
 				unsigned int j = idx[i];
-				DMA_COPY_FROM_USER(&vtxbuf[vb_stride * j],
-						   vtx_size);
+				DMA_COPY(&vtxbuf[vb_stride * j], vtx_size);
 			}
 
 			DMA_COMMIT();
 		}
 
-		usr_idx += count;
+		idx += count;
 		n -= count;
 
 		prim |= BCI_CMD_DRAW_CONT;
@@ -804,11 +790,11 @@ static int savage_dispatch_vb_idx(drm_savage_private_t * dev_priv,
 
 static int savage_dispatch_clear(drm_savage_private_t * dev_priv,
 				 const drm_savage_cmd_header_t * cmd_header,
-				 const drm_savage_cmd_header_t __user * data,
+				 const drm_savage_cmd_header_t *data,
 				 unsigned int nbox,
-				 const drm_clip_rect_t __user * usr_boxes)
+				 const drm_clip_rect_t *boxes)
 {
-	unsigned int flags = cmd_header->clear0.flags, mask, value;
+	unsigned int flags = cmd_header->clear0.flags;
 	unsigned int clear_cmd;
 	unsigned int i, nbufs;
 	DMA_LOCALS;
@@ -816,9 +802,6 @@ static int savage_dispatch_clear(drm_savage_private_t * dev_priv,
 	if (nbox == 0)
 		return 0;
 
-	DRM_GET_USER_UNCHECKED(mask, &data->clear1.mask);
-	DRM_GET_USER_UNCHECKED(value, &data->clear1.value);
-
 	clear_cmd = BCI_CMD_RECT | BCI_CMD_RECT_XP | BCI_CMD_RECT_YP |
 	    BCI_CMD_SEND_COLOR | BCI_CMD_DEST_PBD_NEW;
 	BCI_CMD_SET_ROP(clear_cmd, 0xCC);
@@ -828,21 +811,19 @@ static int savage_dispatch_clear(drm_savage_private_t * dev_priv,
 	if (nbufs == 0)
 		return 0;
 
-	if (mask != 0xffffffff) {
+	if (data->clear1.mask != 0xffffffff) {
 		/* set mask */
 		BEGIN_DMA(2);
 		DMA_SET_REGISTERS(SAVAGE_BITPLANEWTMASK, 1);
-		DMA_WRITE(mask);
+		DMA_WRITE(data->clear1.mask);
 		DMA_COMMIT();
 	}
 	for (i = 0; i < nbox; ++i) {
-		drm_clip_rect_t box;
 		unsigned int x, y, w, h;
 		unsigned int buf;
-		DRM_COPY_FROM_USER_UNCHECKED(&box, &usr_boxes[i], sizeof(box));
-		x = box.x1, y = box.y1;
-		w = box.x2 - box.x1;
-		h = box.y2 - box.y1;
+		x = boxes[i].x1, y = boxes[i].y1;
+		w = boxes[i].x2 - boxes[i].x1;
+		h = boxes[i].y2 - boxes[i].y1;
 		BEGIN_DMA(nbufs * 6);
 		for (buf = SAVAGE_FRONT; buf <= SAVAGE_DEPTH; buf <<= 1) {
 			if (!(flags & buf))
@@ -862,13 +843,13 @@ static int savage_dispatch_clear(drm_savage_private_t * dev_priv,
 				DMA_WRITE(dev_priv->depth_bd);
 				break;
 			}
-			DMA_WRITE(value);
+			DMA_WRITE(data->clear1.value);
 			DMA_WRITE(BCI_X_Y(x, y));
 			DMA_WRITE(BCI_W_H(w, h));
 		}
 		DMA_COMMIT();
 	}
-	if (mask != 0xffffffff) {
+	if (data->clear1.mask != 0xffffffff) {
 		/* reset mask */
 		BEGIN_DMA(2);
 		DMA_SET_REGISTERS(SAVAGE_BITPLANEWTMASK, 1);
@@ -880,8 +861,7 @@ static int savage_dispatch_clear(drm_savage_private_t * dev_priv,
 }
 
 static int savage_dispatch_swap(drm_savage_private_t * dev_priv,
-				unsigned int nbox,
-				const drm_clip_rect_t __user * usr_boxes)
+				unsigned int nbox, const drm_clip_rect_t *boxes)
 {
 	unsigned int swap_cmd;
 	unsigned int i;
@@ -895,16 +875,14 @@ static int savage_dispatch_swap(drm_savage_private_t * dev_priv,
 	BCI_CMD_SET_ROP(swap_cmd, 0xCC);
 
 	for (i = 0; i < nbox; ++i) {
-		drm_clip_rect_t box;
-		DRM_COPY_FROM_USER_UNCHECKED(&box, &usr_boxes[i], sizeof(box));
-
 		BEGIN_DMA(6);
 		DMA_WRITE(swap_cmd);
 		DMA_WRITE(dev_priv->back_offset);
 		DMA_WRITE(dev_priv->back_bd);
-		DMA_WRITE(BCI_X_Y(box.x1, box.y1));
-		DMA_WRITE(BCI_X_Y(box.x1, box.y1));
-		DMA_WRITE(BCI_W_H(box.x2 - box.x1, box.y2 - box.y1));
+		DMA_WRITE(BCI_X_Y(boxes[i].x1, boxes[i].y1));
+		DMA_WRITE(BCI_X_Y(boxes[i].x1, boxes[i].y1));
+		DMA_WRITE(BCI_W_H(boxes[i].x2 - boxes[i].x1,
+				  boxes[i].y2 - boxes[i].y1));
 		DMA_COMMIT();
 	}
 
@@ -912,68 +890,52 @@ static int savage_dispatch_swap(drm_savage_private_t * dev_priv,
 }
 
 static int savage_dispatch_draw(drm_savage_private_t * dev_priv,
-				const drm_savage_cmd_header_t __user * start,
-				const drm_savage_cmd_header_t __user * end,
+				const drm_savage_cmd_header_t *start,
+				const drm_savage_cmd_header_t *end,
 				const drm_buf_t * dmabuf,
-				const unsigned int __user * usr_vtxbuf,
+				const unsigned int *vtxbuf,
 				unsigned int vb_size, unsigned int vb_stride,
 				unsigned int nbox,
-				const drm_clip_rect_t __user * usr_boxes)
+				const drm_clip_rect_t *boxes)
 {
 	unsigned int i, j;
 	int ret;
 
 	for (i = 0; i < nbox; ++i) {
-		drm_clip_rect_t box;
-		const drm_savage_cmd_header_t __user *usr_cmdbuf;
-		DRM_COPY_FROM_USER_UNCHECKED(&box, &usr_boxes[i], sizeof(box));
-		dev_priv->emit_clip_rect(dev_priv, &box);
+		const drm_savage_cmd_header_t *cmdbuf;
+		dev_priv->emit_clip_rect(dev_priv, &boxes[i]);
 
-		usr_cmdbuf = start;
-		while (usr_cmdbuf < end) {
+		cmdbuf = start;
+		while (cmdbuf < end) {
 			drm_savage_cmd_header_t cmd_header;
-			DRM_COPY_FROM_USER_UNCHECKED(&cmd_header, usr_cmdbuf,
-						     sizeof(cmd_header));
-			usr_cmdbuf++;
+			cmd_header = *cmdbuf;
+			cmdbuf++;
 			switch (cmd_header.cmd.cmd) {
 			case SAVAGE_CMD_DMA_PRIM:
-				ret =
-				    savage_dispatch_dma_prim(dev_priv,
-							     &cmd_header,
-							     dmabuf);
+				ret = savage_dispatch_dma_prim(
+					dev_priv, &cmd_header, dmabuf);
 				break;
 			case SAVAGE_CMD_VB_PRIM:
-				ret =
-				    savage_dispatch_vb_prim(dev_priv,
-							    &cmd_header,
-							    (const uint32_t
-							     __user *)
-							    usr_vtxbuf, vb_size,
-							    vb_stride);
+				ret = savage_dispatch_vb_prim(
+					dev_priv, &cmd_header,
+					vtxbuf, vb_size, vb_stride);
 				break;
 			case SAVAGE_CMD_DMA_IDX:
 				j = (cmd_header.idx.count + 3) / 4;
 				/* j was check in savage_bci_cmdbuf */
-				ret =
-				    savage_dispatch_dma_idx(dev_priv,
-							    &cmd_header,
-							    (const uint16_t
-							     __user *)
-							    usr_cmdbuf, dmabuf);
-				usr_cmdbuf += j;
+				ret = savage_dispatch_dma_idx(dev_priv,
+					&cmd_header, (const uint16_t *)cmdbuf,
+					dmabuf);
+				cmdbuf += j;
 				break;
 			case SAVAGE_CMD_VB_IDX:
 				j = (cmd_header.idx.count + 3) / 4;
 				/* j was check in savage_bci_cmdbuf */
-				ret =
-				    savage_dispatch_vb_idx(dev_priv,
-							   &cmd_header,
-							   (const uint16_t
-							    __user *)usr_cmdbuf,
-							   (const uint32_t
-							    __user *)usr_vtxbuf,
-							   vb_size, vb_stride);
-				usr_cmdbuf += j;
+				ret = savage_dispatch_vb_idx(dev_priv,
+					&cmd_header, (const uint16_t *)cmdbuf,
+					(const uint32_t *)vtxbuf, vb_size, 
+					vb_stride);
+				cmdbuf += j;
 				break;
 			default:
 				/* What's the best return code? EFAULT? */
@@ -998,10 +960,10 @@ int savage_bci_cmdbuf(DRM_IOCTL_ARGS)
 	drm_device_dma_t *dma = dev->dma;
 	drm_buf_t *dmabuf;
 	drm_savage_cmdbuf_t cmdbuf;
-	drm_savage_cmd_header_t __user *usr_cmdbuf;
-	drm_savage_cmd_header_t __user *first_draw_cmd;
-	unsigned int __user *usr_vtxbuf;
-	drm_clip_rect_t __user *usr_boxes;
+	drm_savage_cmd_header_t *kcmd_addr = NULL;
+	drm_savage_cmd_header_t *first_draw_cmd;
+	unsigned int *kvb_addr = NULL;
+	drm_clip_rect_t *kbox_addr = NULL;
 	unsigned int i, j;
 	int ret = 0;
 
@@ -1024,15 +986,53 @@ int savage_bci_cmdbuf(DRM_IOCTL_ARGS)
 		dmabuf = NULL;
 	}
 
-	usr_cmdbuf = (drm_savage_cmd_header_t __user *) cmdbuf.cmd_addr;
-	usr_vtxbuf = (unsigned int __user *)cmdbuf.vb_addr;
-	usr_boxes = (drm_clip_rect_t __user *) cmdbuf.box_addr;
-	if ((cmdbuf.size && DRM_VERIFYAREA_READ(usr_cmdbuf, cmdbuf.size * 8)) ||
-	    (cmdbuf.vb_size && DRM_VERIFYAREA_READ(usr_vtxbuf, cmdbuf.vb_size))
-	    || (cmdbuf.nbox
-		&& DRM_VERIFYAREA_READ(usr_boxes,
-				       cmdbuf.nbox * sizeof(drm_clip_rect_t))))
-		return DRM_ERR(EFAULT);
+	/* Copy the user buffers into kernel temporary areas.  This hasn't been
+	 * a performance loss compared to VERIFYAREA_READ/
+	 * COPY_FROM_USER_UNCHECKED when done in other drivers, and is correct
+	 * for locking on FreeBSD.
+	 */
+	if (cmdbuf.size) {
+		kcmd_addr = drm_alloc(cmdbuf.size * 8, DRM_MEM_DRIVER);
+		if (kcmd_addr == NULL)
+			return ENOMEM;
+
+		if (DRM_COPY_FROM_USER(kcmd_addr, cmdbuf.cmd_addr,
+				       cmdbuf.size * 8))
+		{
+			drm_free(kcmd_addr, cmdbuf.size * 8, DRM_MEM_DRIVER);
+			return DRM_ERR(EFAULT);
+		}
+		cmdbuf.cmd_addr = kcmd_addr;
+	}
+	if (cmdbuf.vb_size) {
+		kvb_addr = drm_alloc(cmdbuf.vb_size, DRM_MEM_DRIVER);
+		if (kvb_addr == NULL) {
+			ret = DRM_ERR(ENOMEM);
+			goto done;
+		}
+
+		if (DRM_COPY_FROM_USER(kvb_addr, cmdbuf.vb_addr,
+				       cmdbuf.vb_size)) {
+			ret = DRM_ERR(EFAULT);
+			goto done;
+		}
+		cmdbuf.vb_addr = kvb_addr;
+	}
+	if (cmdbuf.nbox) {
+		kbox_addr = drm_alloc(cmdbuf.nbox * sizeof(drm_clip_rect_t),
+				       DRM_MEM_DRIVER);
+		if (kbox_addr == NULL) {
+			ret = DRM_ERR(ENOMEM);
+			goto done;
+		}
+
+		if (DRM_COPY_FROM_USER(kbox_addr, cmdbuf.box_addr,
+				       cmdbuf.nbox * sizeof(drm_clip_rect_t))) {
+			ret = DRM_ERR(EFAULT);
+			goto done;
+		}
+	cmdbuf.box_addr = kbox_addr;
+	}
 
 	/* Make sure writes to DMA buffers are finished before sending
 	 * DMA commands to the graphics hardware. */
@@ -1046,9 +1046,8 @@ int savage_bci_cmdbuf(DRM_IOCTL_ARGS)
 	first_draw_cmd = NULL;
 	while (i < cmdbuf.size) {
 		drm_savage_cmd_header_t cmd_header;
-		DRM_COPY_FROM_USER_UNCHECKED(&cmd_header, usr_cmdbuf,
-					     sizeof(cmd_header));
-		usr_cmdbuf++;
+		cmd_header = *(drm_savage_cmd_header_t *)cmdbuf.cmd_addr;
+		cmdbuf.cmd_addr++;
 		i++;
 
 		/* Group drawing commands with same state to minimize
@@ -1068,21 +1067,18 @@ int savage_bci_cmdbuf(DRM_IOCTL_ARGS)
 		case SAVAGE_CMD_DMA_PRIM:
 		case SAVAGE_CMD_VB_PRIM:
 			if (!first_draw_cmd)
-				first_draw_cmd = usr_cmdbuf - 1;
-			usr_cmdbuf += j;
+				first_draw_cmd = cmdbuf.cmd_addr - 1;
+			cmdbuf.cmd_addr += j;
 			i += j;
 			break;
 		default:
 			if (first_draw_cmd) {
-				ret =
-				    savage_dispatch_draw(dev_priv,
-							 first_draw_cmd,
-							 usr_cmdbuf - 1, dmabuf,
-							 usr_vtxbuf,
-							 cmdbuf.vb_size,
-							 cmdbuf.vb_stride,
-							 cmdbuf.nbox,
-							 usr_boxes);
+				ret = savage_dispatch_draw(
+				      dev_priv, first_draw_cmd,
+				      cmdbuf.cmd_addr - 1,
+				      dmabuf, cmdbuf.vb_addr, cmdbuf.vb_size,
+				      cmdbuf.vb_stride,
+				      cmdbuf.nbox, cmdbuf.box_addr);
 				if (ret != 0)
 					return ret;
 				first_draw_cmd = NULL;
@@ -1098,12 +1094,12 @@ int savage_bci_cmdbuf(DRM_IOCTL_ARGS)
 				DRM_ERROR("command SAVAGE_CMD_STATE extends "
 					  "beyond end of command buffer\n");
 				DMA_FLUSH();
-				return DRM_ERR(EINVAL);
+				ret = DRM_ERR(EINVAL);
+				goto done;
 			}
 			ret = savage_dispatch_state(dev_priv, &cmd_header,
-						    (uint32_t __user *)
-						    usr_cmdbuf);
-			usr_cmdbuf += j;
+				(const uint32_t *)cmdbuf.cmd_addr);
+			cmdbuf.cmd_addr += j;
 			i += j;
 			break;
 		case SAVAGE_CMD_CLEAR:
@@ -1111,39 +1107,40 @@ int savage_bci_cmdbuf(DRM_IOCTL_ARGS)
 				DRM_ERROR("command SAVAGE_CMD_CLEAR extends "
 					  "beyond end of command buffer\n");
 				DMA_FLUSH();
-				return DRM_ERR(EINVAL);
+				ret = DRM_ERR(EINVAL);
+				goto done;
 			}
 			ret = savage_dispatch_clear(dev_priv, &cmd_header,
-						    usr_cmdbuf,
-						    cmdbuf.nbox, usr_boxes);
-			usr_cmdbuf++;
+						    cmdbuf.cmd_addr,
+						    cmdbuf.nbox, cmdbuf.box_addr);
+			cmdbuf.cmd_addr++;
 			i++;
 			break;
 		case SAVAGE_CMD_SWAP:
-			ret = savage_dispatch_swap(dev_priv,
-						   cmdbuf.nbox, usr_boxes);
+			ret = savage_dispatch_swap(dev_priv, cmdbuf.nbox,
+						   cmdbuf.box_addr);
 			break;
 		default:
 			DRM_ERROR("invalid command 0x%x\n", cmd_header.cmd.cmd);
 			DMA_FLUSH();
-			return DRM_ERR(EINVAL);
+			ret = DRM_ERR(EINVAL);
+			goto done;
 		}
 
 		if (ret != 0) {
 			DMA_FLUSH();
-			return ret;
+			goto done;
 		}
 	}
 
 	if (first_draw_cmd) {
-		ret =
-		    savage_dispatch_draw(dev_priv, first_draw_cmd, usr_cmdbuf,
-					 dmabuf, usr_vtxbuf, cmdbuf.vb_size,
-					 cmdbuf.vb_stride, cmdbuf.nbox,
-					 usr_boxes);
+		ret = savage_dispatch_draw (
+			dev_priv, first_draw_cmd, cmdbuf.cmd_addr, dmabuf,
+			cmdbuf.vb_addr, cmdbuf.vb_size, cmdbuf.vb_stride,
+			cmdbuf.nbox, cmdbuf.box_addr);
 		if (ret != 0) {
 			DMA_FLUSH();
-			return ret;
+			goto done;
 		}
 	}
 
@@ -1157,5 +1154,12 @@ int savage_bci_cmdbuf(DRM_IOCTL_ARGS)
 		savage_freelist_put(dev, dmabuf);
 	}
 
-	return 0;
+done:
+	/* If we didn't need to allocate them, these'll be NULL */
+	drm_free(kcmd_addr, cmdbuf.size * 8, DRM_MEM_DRIVER);
+	drm_free(kvb_addr, cmdbuf.vb_size, DRM_MEM_DRIVER);
+	drm_free(kbox_addr, cmdbuf.nbox * sizeof(drm_clip_rect_t),
+		 DRM_MEM_DRIVER);
+
+	return ret;
 }