lkml.org 
[lkml]   [2008]   [Nov]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
Subject[RFC][PATCH] drm: fix leak of uninitialized data to userspace #2
From
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. The buffer was also too small if snprintf() returns
== 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 f1035aa2e85a3908b311beea44c1c32f1135cfd1 Mon Sep 17 00:00:00 2001
From: Vegard Nossum <vegard.nossum@gmail.com>
Date: Tue, 11 Nov 2008 22:38:06 +0100
Subject: [PATCH] drm: fix leak of uninitialized data to userspace

On Fri, Oct 10, 2008 at 10:54 AM, Sitsofe Wheeler <sitsofe@yahoo.com> wrote:
>
> [ 175.375036] WARNING: kmemcheck: Caught 32-bit read from uninitialized memory (f65d2294)
> [ 175.375049] 7063693a303030303a30303a30322e3000a76c080800000000a76c080e000000
> [ 175.375096] i i i i i i i i i i i i i i i i i u u u u u u u u u u u u u u u
> [ 175.375137] ^
> [ 175.375142]
> [ 175.375148] Pid: 2288, comm: Xorg Not tainted (2.6.27-tipskw-00069-g37cb0b7-dirty #81) 900
> [ 175.375155] EIP: 0060:[<c020d283>] EFLAGS: 00003246 CPU: 0
> [ 175.375169] EIP is at __copy_to_user_ll+0x43/0x60
> [ 175.375174] EAX: 00000000 EBX: 00000028 ECX: 00000005 EDX: f65d2280
> [ 175.375180] ESI: f65d2294 EDI: 086cb6b4 EBP: f631bef4 ESP: c055bd68
> [ 175.375186] DS: 007b ES: 007b FS: 0000 GS: 0033 SS: 0068
> [ 175.375191] CR0: 8005003b CR2: f67c1c44 CR3: 36368000 CR4: 000006c0
> [ 175.375197] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
> [ 175.375202] DR6: ffff4ff0 DR7: 00000400
> [ 175.375206] [<c020d6f3>] copy_to_user+0x43/0x60
> [ 175.375214] [<c028df28>] drm_getunique+0x38/0x50
> [ 175.375224] [<c028d329>] drm_ioctl+0x1b9/0x2f0
> [ 175.375231] [<c0188a07>] vfs_ioctl+0x67/0x70
> [ 175.375239] [<c0188a6c>] do_vfs_ioctl+0x5c/0x270
> [ 175.375246] [<c0188cbe>] sys_ioctl+0x3e/0x60
> [ 175.375253] [<c010336d>] sysenter_do_call+0x12/0x35
> [ 175.375261] [<ffffffff>] 0xffffffff

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().

Reported-by: Sitsofe Wheeler <sitsofe@yahoo.com>
Signed-off-by: Vegard Nossum <vegardno@thuin.ifi.uio.no>
---
drivers/gpu/drm/drm_ioctl.c | 6 ++++--
1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 16829fb..7212c38 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -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


\
 
 \ /
  Last update: 2008-11-11 23:19    [W:0.024 / U:0.536 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site