drm: fix leak of uninitialized data to userspace #2

Hi,

First patch had an additional problem in case snprintf() wanted to write a string longer than the buffer and returned a large value. Most likely this would never ever hit, but we can handle it by only setting the actual length when we _didn't_ try to overflow the buffer.

Secondly, the snprintf() won't ever write more than the given number of bytes given, so allocating + 1 bytes isn't ever going to do anything useful.

Thirdly, we also need to check whether snprintf() returned

== unique_len. snprintf() will always ensure that the result is properly null-terminated, except in the case where the buffer is NULL or the buffer size is zero (can never happen here).

It is also a bit suspicious that this function's return value is never checked, and that we don't free the first allocation if the second one fails. But maybe it's ok, didn't really check.

I encourage careful review of this patch, as my first attempt was invalid without me noticing or anybody else saying a thing (or was that why it was never pushed to mainline?) :-(

(And the original code was obviously in error to begin with.)

Vegard

PS: Feel free to move any/all of this text into commit message if patch is deemed worthy of inclusion. I just wanted to make the new info stand out.

From: Vegard Nossum <[email protected]>
Date: Tue, 11 Nov 2008 22:38:06 +0100
Subject: [PATCH] drm: fix leak of uninitialized data to userspace

The hexdump decodes to:

vegardno@thuin ~ $ ./a.out 7063693a303030303a30303a30322e3000a76c080800000000a76c080e000000 pci:0000:00:02.0<0><167>l<8><8><0><0><0><0><167>l<8><14><0><0><0>

...so drm_getunique() is trying to copy some uninitialized data to userspace. The ECX register contains the number of words that are left to copy -- so there are 5 * 4 = 20 bytes left. The offset of the first uninitialized byte (counting from the start of the string) is also 20 (i.e. 0xf65d2294&((1 << 5)-1) == 20). So somebody tried to copy 40 bytes when the string was only 19 long.

In drm_set_busid() we have this code:

        dev->unique_len = 40;
        dev->unique = drm_alloc(dev->unique_len + 1, DRM_MEM_DRIVER);
	...
        len = snprintf(dev->unique, dev->unique_len, "pci:%04x:%02x:%02x.%d",

...so it seems that dev->unique is never updated to reflect the actual length of the string. The remaining bytes (20 in this case) are random uninitialized bytes that are copied into userspace.

This patch fixes the problem by setting dev->unique_len after the snprintf().

---
 drivers/gpu/drm/drm_ioctl.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

index 16829fb..7212c38 100644
@@ -133,7 +133,7 @@ static int drm_set_busid(struct drm_device * dev)
 		return 0;
 
 	dev->unique_len = 40;
-	dev->unique = drm_alloc(dev->unique_len + 1, DRM_MEM_DRIVER);
+	dev->unique = drm_alloc(dev->unique_len, DRM_MEM_DRIVER);
 	if (dev->unique == NULL)
 		return -ENOMEM;
 

@@ -142,8 +142,10 @@ static int drm_set_busid(struct drm_device * dev)
 		       PCI_SLOT(dev->pdev->devfn),
 		       PCI_FUNC(dev->pdev->devfn));
 
-	if (len > dev->unique_len)
+	if (len >= dev->unique_len)
 		DRM_ERROR("Unique buffer overflowed\n");
+	else
+		dev->unique_len = len;
 
 	dev->devname =
 	    drm_alloc(strlen(dev->driver->pci_driver.name) + dev->unique_len +

1.5.6.5