>From f2e1569413900fa3ce5dad571e1a24d31307ab75 Mon Sep 17 00:00:00 2001
From: Vegard Nossum <[email protected]>
Date: Fri, 10 Oct 2008 11:50:57 +0200
Subject: [PATCH] drm: fix leak of uninitialized data to userspace
On Fri, Oct 10, 2008 at 10:54 AM, Sitsofe Wheeler <[email protected]> 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().
Completely untested.
Reported-by: Sitsofe Wheeler <[email protected]>
Signed-off-by: Vegard Nossum <[email protected]>
---
drivers/gpu/drm/drm_ioctl.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 16829fb..480225b 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -144,6 +144,7 @@ static int drm_set_busid(struct drm_device * dev)
if (len > dev->unique_len)
DRM_ERROR("Unique buffer overflowed\n");
+ dev->unique_len = len + 1;
dev->devname =
drm_alloc(strlen(dev->driver->pci_driver.name) + dev->unique_len +
--
1.5.6.4
* Vegard Nossum <[email protected]> wrote:
> >From f2e1569413900fa3ce5dad571e1a24d31307ab75 Mon Sep 17 00:00:00 2001
> From: Vegard Nossum <[email protected]>
> Date: Fri, 10 Oct 2008 11:50:57 +0200
> Subject: [PATCH] drm: fix leak of uninitialized data to userspace
>
> On Fri, Oct 10, 2008 at 10:54 AM, Sitsofe Wheeler <[email protected]> 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().
>
> Completely untested.
>
> Reported-by: Sitsofe Wheeler <[email protected]>
> Signed-off-by: Vegard Nossum <[email protected]>
i've stuck it into the tip/out-of-tree quick-fixes branch.
Sitsofe, could you please check very latest tip/master with
CONFIG_KMEMCHECK=y, does it find any other uninitialized memory access?
Ingo
> From: Ingo Molnar <[email protected]>
>
> i've stuck it into the tip/out-of-tree quick-fixes branch.
I've just got to say that testing Ingo's stuff is always an absolute pleasure. Ingo has pulled in test patches into his git tree which makes testing potential fixes many times easier for me (I've had yahoo mail mangle patches which would force me to rebuild the patch by hand rather than just apply it). The sheer speed of turn around is scarily impressive too. If you ever want to know how to get people to test your stuff Ingo's lead is a good one to follow.
> Sitsofe, could you please check very latest tip/master with
> CONFIG_KMEMCHECK=y, does it find any other uninitialized memory access?
Give me a few minutes and I'll try and get back to you...
On Fri, Oct 10, 2008 at 12:17:53PM +0200, Ingo Molnar wrote:
>
> * Vegard Nossum <[email protected]> wrote:
>
> > >From f2e1569413900fa3ce5dad571e1a24d31307ab75 Mon Sep 17 00:00:00 2001
> > From: Vegard Nossum <[email protected]>
> > Date: Fri, 10 Oct 2008 11:50:57 +0200
> > Subject: [PATCH] drm: fix leak of uninitialized data to userspace
> >
> > On Fri, Oct 10, 2008 at 10:54 AM, Sitsofe Wheeler <[email protected]> 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().
> >
> > Completely untested.
> >
> > Reported-by: Sitsofe Wheeler <[email protected]>
> > Signed-off-by: Vegard Nossum <[email protected]>
>
> i've stuck it into the tip/out-of-tree quick-fixes branch.
>
> Sitsofe, could you please check very latest tip/master with
> CONFIG_KMEMCHECK=y, does it find any other uninitialized memory access?
What ever happened to this patch? Did it go into Linus's tree? If so,
I can't seem to find it :(
thanks,
greg k-h
* Greg KH <[email protected]> wrote:
> On Fri, Oct 10, 2008 at 12:17:53PM +0200, Ingo Molnar wrote:
> >
> > * Vegard Nossum <[email protected]> wrote:
> >
> > > >From f2e1569413900fa3ce5dad571e1a24d31307ab75 Mon Sep 17 00:00:00 2001
> > > From: Vegard Nossum <[email protected]>
> > > Date: Fri, 10 Oct 2008 11:50:57 +0200
> > > Subject: [PATCH] drm: fix leak of uninitialized data to userspace
> > >
> > > On Fri, Oct 10, 2008 at 10:54 AM, Sitsofe Wheeler <[email protected]> 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().
> > >
> > > Completely untested.
> > >
> > > Reported-by: Sitsofe Wheeler <[email protected]>
> > > Signed-off-by: Vegard Nossum <[email protected]>
> >
> > i've stuck it into the tip/out-of-tree quick-fixes branch.
> >
> > Sitsofe, could you please check very latest tip/master with
> > CONFIG_KMEMCHECK=y, does it find any other uninitialized memory
> > access?
>
> What ever happened to this patch? Did it go into Linus's tree? If
> so, I can't seem to find it :(
i'm not doing DRM patches - tip/out-of-tree is for pure out-of-tree
hotfixes.
Ingo
On Tue, Nov 04, 2008 at 11:17:30PM +0100, Ingo Molnar wrote:
>
> * Greg KH <[email protected]> wrote:
>
> > On Fri, Oct 10, 2008 at 12:17:53PM +0200, Ingo Molnar wrote:
> > >
> > > * Vegard Nossum <[email protected]> wrote:
> > >
> > > > >From f2e1569413900fa3ce5dad571e1a24d31307ab75 Mon Sep 17 00:00:00 2001
> > > > From: Vegard Nossum <[email protected]>
> > > > Date: Fri, 10 Oct 2008 11:50:57 +0200
> > > > Subject: [PATCH] drm: fix leak of uninitialized data to userspace
> > > >
> > > > On Fri, Oct 10, 2008 at 10:54 AM, Sitsofe Wheeler <[email protected]> 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().
> > > >
> > > > Completely untested.
> > > >
> > > > Reported-by: Sitsofe Wheeler <[email protected]>
> > > > Signed-off-by: Vegard Nossum <[email protected]>
> > >
> > > i've stuck it into the tip/out-of-tree quick-fixes branch.
> > >
> > > Sitsofe, could you please check very latest tip/master with
> > > CONFIG_KMEMCHECK=y, does it find any other uninitialized memory
> > > access?
> >
> > What ever happened to this patch? Did it go into Linus's tree? If
> > so, I can't seem to find it :(
>
> i'm not doing DRM patches - tip/out-of-tree is for pure out-of-tree
> hotfixes.
So it goes no where?
Dave, any plan to pick this up and get it to Linus?
thanks,
greg k-h
On Tue, Nov 4, 2008 at 11:23 PM, Greg KH <[email protected]> wrote:
> On Tue, Nov 04, 2008 at 11:17:30PM +0100, Ingo Molnar wrote:
>>
>> * Greg KH <[email protected]> wrote:
>> > What ever happened to this patch? Did it go into Linus's tree? If
>> > so, I can't seem to find it :(
>>
>> i'm not doing DRM patches - tip/out-of-tree is for pure out-of-tree
>> hotfixes.
>
> So it goes no where?
>
> Dave, any plan to pick this up and get it to Linus?
For Dave: You may also remove the "completely untested" line of the
original submission and add Sitsofe's Tested-by line:
http://lkml.org/lkml/2008/10/11/108
Original patch can be found here:
http://lkml.org/lkml/2008/10/10/115
:-)
Thanks,
Vegard
--
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
-- E. W. Dijkstra, EWD1036
On Tue, 2008-11-04 at 14:23 -0800, Greg KH wrote:
> On Tue, Nov 04, 2008 at 11:17:30PM +0100, Ingo Molnar wrote:
> >
> > * Greg KH <[email protected]> wrote:
> >
> > > On Fri, Oct 10, 2008 at 12:17:53PM +0200, Ingo Molnar wrote:
> > > >
> > > > * Vegard Nossum <[email protected]> wrote:
> > > >
> > > > > >From f2e1569413900fa3ce5dad571e1a24d31307ab75 Mon Sep 17 00:00:00 2001
> > > > > From: Vegard Nossum <[email protected]>
> > > > > Date: Fri, 10 Oct 2008 11:50:57 +0200
> > > > > Subject: [PATCH] drm: fix leak of uninitialized data to userspace
> > > > >
> > > > > On Fri, Oct 10, 2008 at 10:54 AM, Sitsofe Wheeler <[email protected]> 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().
> > > > >
> > > > > Completely untested.
> > > > >
> > > > > Reported-by: Sitsofe Wheeler <[email protected]>
> > > > > Signed-off-by: Vegard Nossum <[email protected]>
> > > >
> > > > i've stuck it into the tip/out-of-tree quick-fixes branch.
> > > >
> > > > Sitsofe, could you please check very latest tip/master with
> > > > CONFIG_KMEMCHECK=y, does it find any other uninitialized memory
> > > > access?
> > >
> > > What ever happened to this patch? Did it go into Linus's tree? If
> > > so, I can't seem to find it :(
> >
> > i'm not doing DRM patches - tip/out-of-tree is for pure out-of-tree
> > hotfixes.
>
> So it goes no where?
>
> Dave, any plan to pick this up and get it to Linus?
>
Oops just fell off my radar, will stick it the next set of drm-fixes.
Dave.