Skip to contentSkip to conversation

PATCH: 2.6.7-rc3 drivers/char/drm/gamma_dma.c: several user/kernel pointer bugs

PATCH Robert T. Johnson 4 participants 5 messages Jun 9, 2004 – Jun 10, 2004 Char and misc drivers

lore
Share & tools
lore
#0 lore raw

gamma_dma_priority and gamma_dma_send_buffers both deref d->send_indices and/or d->send_sizes. When these functions are called from gamma_dma, these pointers are user pointers and are thus not safe to deref. This patch copies over the pointers inside gamma_dma_priority and gamma_dma_send_buffers. Let me know if you have any questions or if I've made a mistake.

Best,
Rob

drivers/char/drm/gamma_dma.c renamed from drivers/char/drm/gamma_dma.c.orig +39 -6
@@ -346,6 +346,9 @@ static int gamma_dma_priority(struct fil
 	drm_buf_t	  *buf;
 	drm_buf_t	  *last_buf = NULL;
 	drm_device_dma_t  *dma	    = dev->dma;
+	int               *send_indices = NULL;
+	int               *send_sizes = NULL;
+
 	DECLARE_WAITQUEUE(entry, current);
 
 				/* Turn off interrupt handling */
@@ -365,11 +368,31 @@ static int gamma_dma_priority(struct fil
 		++must_free;
 	}
 
+	send_indices = DRM(alloc)(d->send_count * sizeof(*send_indices),
+		                  DRM_MEM_DRIVER);
+        if (send_indices == NULL)
+                return -ENOMEM;
+	if (copy_from_user(send_indices, d->send_indices, 
+			   d->send_count * sizeof(*send_indices))) {
+		retcode = -EFAULT;
+                goto cleanup;
+	}
+	
+	send_sizes = DRM(alloc)(d->send_count * sizeof(*send_sizes),
+		                  DRM_MEM_DRIVER);
+        if (send_sizes == NULL)
+                return -ENOMEM;
+	if (copy_from_user(send_sizes, d->send_sizes, 
+			   d->send_count * sizeof(*send_sizes))) {
+		retcode = -EFAULT;
+                goto cleanup;
+	}
+
 	for (i = 0; i < d->send_count; i++) {
-		idx = d->send_indices[i];
+		idx = send_indices[i];
 		if (idx < 0 || idx >= dma->buf_count) {
 			DRM_ERROR("Index %d (of %d max)\n",
-				  d->send_indices[i], dma->buf_count - 1);
+				  send_indices[i], dma->buf_count - 1);
 			continue;
 		}
 		buf = dma->buflist[ idx ];
@@ -391,7 +414,7 @@ static int gamma_dma_priority(struct fil
 				   process closes the /dev/drm? handle, so
 				   it can't also be doing DMA. */
 		buf->list	  = DRM_LIST_PRIO;
-		buf->used	  = d->send_sizes[i];
+		buf->used	  = send_sizes[i];
 		buf->context	  = d->context;
 		buf->while_locked = d->flags & _DRM_DMA_WHILE_LOCKED;
 		address		  = (unsigned long)buf->address;
@@ -402,14 +425,14 @@ static int gamma_dma_priority(struct fil
 		if (buf->pending) {
 			DRM_ERROR("Sending pending buffer:"
 				  " buffer %d, offset %d\n",
-				  d->send_indices[i], i);
+				  send_indices[i], i);
 			retcode = -EINVAL;
 			goto cleanup;
 		}
 		if (buf->waiting) {
 			DRM_ERROR("Sending waiting buffer:"
 				  " buffer %d, offset %d\n",
-				  d->send_indices[i], i);
+				  send_indices[i], i);
 			retcode = -EINVAL;
 			goto cleanup;
 		}
@@ -458,6 +481,12 @@ cleanup:
 		gamma_dma_ready(dev);
 		gamma_free_buffer(dev, last_buf);
 	}
+        if (send_indices)
+                DRM(free)(send_indices, d->send_count * sizeof(*send_indices), 
+			  DRM_MEM_DRIVER);
+        if (send_sizes)
+                DRM(free)(send_sizes, d->send_count * sizeof(*send_sizes), 
+			  DRM_MEM_DRIVER);
 
 	if (must_free && !dev->context_flag) {
 		if (gamma_lock_free(dev, &dev->lock.hw_lock->lock,
@@ -476,9 +505,13 @@ static int gamma_dma_send_buffers(struct
 	drm_buf_t	  *last_buf = NULL;
 	int		  retcode   = 0;
 	drm_device_dma_t  *dma	    = dev->dma;
+	int               send_index;
+
+	if (get_user(send_index, &d->send_indices[d->send_count-1]))
+		return -EFAULT;
 
 	if (d->flags & _DRM_DMA_BLOCK) {
-		last_buf = dma->buflist[d->send_indices[d->send_count-1]];
+		last_buf = dma->buflist[send_index];
 		add_wait_queue(&last_buf->dma_wait, &entry);
 	}
 
#1 lore raw
Quoted 6 lines · Robert T. Johnson

gamma_dma_priority and gamma_dma_send_buffers both deref d->send_indices and/or d->send_sizes. When these functions are called from gamma_dma, these pointers are user pointers and are thus not safe to deref. This patch copies over the pointers inside gamma_dma_priority and gamma_dma_send_buffers. Let me know if you have any questions or if I've made a mistake.

ACK.

#2 lore raw

I'll fix this in the DRM bk tree and push to Linus...

Dave.

Quoted 22 lines · On Thu, 10 Jun 2004 [email protected] wrote:
Quoted 6 lines · Robert T. Johnson

gamma_dma_priority and gamma_dma_send_buffers both deref d->send_indices and/or d->send_sizes. When these functions are called from gamma_dma, these pointers are user pointers and are thus not safe to deref. This patch copies over the pointers inside gamma_dma_priority and gamma_dma_send_buffers. Let me know if you have any questions or if I've made a mistake.

ACK.

-------------------------------------------------------
This SF.Net email is sponsored by: GNOME Foundation
Hackers Unite! GUADEC: The world's #1 Open Source Desktop Event. GNOME Users and Developers European Conference, 28-30th June in Norway
http://2004/guadec.org
--
_______________________________________________
Dri-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/dri-devel

Signature · 3 lines
David Airlie, Software Engineer
http://www.skynet.ie/~airlied / airlied at skynet.ie
pam_smb / Linux DECstation / Linux VAX / ILUG person
#3 lore raw
In reply to Robert T. Johnson
Quoted 7 lines

gamma_dma_priority and gamma_dma_send_buffers both deref d->send_indices and/or d->send_sizes. When these functions are called from gamma_dma, these pointers are user pointers and are thus not safe to deref. This patch copies over the pointers inside gamma_dma_priority and gamma_dma_send_buffers. Let me know if you have any questions or if I've made a mistake.

okay I've checked this into the drm bk tree and DRM CVS, I've no way to test it apart from visual inspection and it compiles, I've asked Linus to sync the drm tree again, I probably need to add some __user annotations in a few places..

Dave.

Signature · 3 lines
David Airlie, Software Engineer
http://www.skynet.ie/~airlied / airlied at skynet.ie
pam_smb / Linux DECstation / Linux VAX / ILUG person
#4 lore raw
Quoted 4 lines · Dave Airlie

okay I've checked this into the drm bk tree and DRM CVS, I've no way to test it apart from visual inspection and it compiles, I've asked Linus to sync the drm tree again, I probably need to add some __user annotations in a few places..

I've got an Oxygen GMX somewhere, but last time I investigated the drivers didn't work anyway. Not that fixing the security bits isnt a good idea regardless.

I'll try and test it at some point