* Re: [PATCH] drm: fix leak of uninitialized data to userspace (acpi_system_read_event)
@ 2008-10-11 7:55 Sitsofe Wheeler
0 siblings, 0 replies; 5+ messages in thread
From: Sitsofe Wheeler @ 2008-10-11 7:55 UTC (permalink / raw)
To: Vegard Nossum, Ingo Molnar
Cc: Vegard Nossum, Dave Airlie, Pekka Enberg, linux-kernel
> From: Vegard Nossum <vegard.nossum@gmail.com>
>
> Does that also mean that the DRM patch fixed the first one? :-)
Yes, that's "the original kmemcheck drm warning is no longer present with your patch".
Tested-by: Sitsofe Wheeler <sitsofe@yahoo.com>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] drm: fix leak of uninitialized data to userspace (acpi_system_read_event)
@ 2008-10-10 13:26 Sitsofe Wheeler
2008-10-10 14:37 ` Ingo Molnar
0 siblings, 1 reply; 5+ messages in thread
From: Sitsofe Wheeler @ 2008-10-10 13:26 UTC (permalink / raw)
To: Ingo Molnar, Vegard Nossum; +Cc: Dave Airlie, Pekka Enberg, linux-kernel
> From: Ingo Molnar <mingo@elte.hu>
>
> * Vegard Nossum wrote:
>
> > ...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
> > Signed-off-by: Vegard Nossum
>
> 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?
No other uninitialized memory access so far (although having kmemcheck on does seem to provoke rcu stall warnings)...
...I take it back. This just turned up:
[ 992.417019] WARNING: kmemcheck: Caught 32-bit read from uninitialized memory (f2363d14)
[ 992.417033] 000110000002200061635f61646170746572000000000000cc2c030041433000
[ 992.417077] i i 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 i i i i
[ 992.417117] ^
[ 992.417121]
[ 992.417127] Pid: 1893, comm: acpid Not tainted (2.6.27-tipskw-00088-g9f41241-dirty #84) 900
[ 992.417134] EIP: 0060:[<c025fbdd>] EFLAGS: 00000286 CPU: 0
[ 992.417147] EIP is at acpi_bus_receive_event+0xd6/0x109
[ 992.417153] EAX: 00054489 EBX: f2363d00 ECX: 00000006 EDX: ffffffed
[ 992.417158] ESI: f2363d14 EDI: f6057f28 EBP: f6057f08 ESP: c0566d68
[ 992.417164] DS: 007b ES: 007b FS: 0000 GS: 0033 SS: 0068
[ 992.417169] CR0: 8005003b CR2: f6671034 CR3: 360ea000 CR4: 000006c0
[ 992.417175] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
[ 992.417180] DR6: ffff4ff0 DR7: 00000400
[ 992.417184] [<c026b86f>] acpi_system_read_event+0x49/0xc5
[ 992.417195] [<c01b2381>] proc_reg_read+0x61/0x90
[ 992.417206] [<c017efb5>] vfs_read+0x95/0x120
[ 992.417215] [<c017f5f2>] sys_read+0x42/0x70
[ 992.417222] [<c010336d>] sysenter_do_call+0x12/0x35
[ 992.417230] [<ffffffff>] 0xffffffff
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] drm: fix leak of uninitialized data to userspace (acpi_system_read_event)
2008-10-10 13:26 Sitsofe Wheeler
@ 2008-10-10 14:37 ` Ingo Molnar
2008-10-10 15:26 ` Vegard Nossum
0 siblings, 1 reply; 5+ messages in thread
From: Ingo Molnar @ 2008-10-10 14:37 UTC (permalink / raw)
To: Sitsofe Wheeler; +Cc: Vegard Nossum, Dave Airlie, Pekka Enberg, linux-kernel
* Sitsofe Wheeler <sitsofe@yahoo.com> wrote:
> > From: Ingo Molnar <mingo@elte.hu>
>
> >
> > * Vegard Nossum wrote:
> >
> > > ...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
> > > Signed-off-by: Vegard Nossum
> >
> > 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?
>
> No other uninitialized memory access so far (although having kmemcheck on does seem to provoke rcu stall warnings)...
>
> ...I take it back. This just turned up:
> [ 992.417019] WARNING: kmemcheck: Caught 32-bit read from uninitialized memory (f2363d14)
> [ 992.417033] 000110000002200061635f61646170746572000000000000cc2c030041433000
> [ 992.417077] i i 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 i i i i
> [ 992.417117] ^
> [ 992.417121]
> [ 992.417127] Pid: 1893, comm: acpid Not tainted (2.6.27-tipskw-00088-g9f41241-dirty #84) 900
> [ 992.417134] EIP: 0060:[<c025fbdd>] EFLAGS: 00000286 CPU: 0
> [ 992.417147] EIP is at acpi_bus_receive_event+0xd6/0x109
> [ 992.417153] EAX: 00054489 EBX: f2363d00 ECX: 00000006 EDX: ffffffed
> [ 992.417158] ESI: f2363d14 EDI: f6057f28 EBP: f6057f08 ESP: c0566d68
> [ 992.417164] DS: 007b ES: 007b FS: 0000 GS: 0033 SS: 0068
> [ 992.417169] CR0: 8005003b CR2: f6671034 CR3: 360ea000 CR4: 000006c0
> [ 992.417175] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
> [ 992.417180] DR6: ffff4ff0 DR7: 00000400
> [ 992.417184] [<c026b86f>] acpi_system_read_event+0x49/0xc5
> [ 992.417195] [<c01b2381>] proc_reg_read+0x61/0x90
> [ 992.417206] [<c017efb5>] vfs_read+0x95/0x120
> [ 992.417215] [<c017f5f2>] sys_read+0x42/0x70
> [ 992.417222] [<c010336d>] sysenter_do_call+0x12/0x35
> [ 992.417230] [<ffffffff>] 0xffffffff
this too could be a real bug i think, uncovered by kmemcheck. Vegard?
Ingo
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm: fix leak of uninitialized data to userspace (acpi_system_read_event)
2008-10-10 14:37 ` Ingo Molnar
@ 2008-10-10 15:26 ` Vegard Nossum
2008-10-10 15:32 ` Ingo Molnar
0 siblings, 1 reply; 5+ messages in thread
From: Vegard Nossum @ 2008-10-10 15:26 UTC (permalink / raw)
To: Ingo Molnar
Cc: Sitsofe Wheeler, Vegard Nossum, Dave Airlie, Pekka Enberg,
linux-kernel
On Fri, Oct 10, 2008 at 4:37 PM, Ingo Molnar <mingo@elte.hu> wrote:
> * Sitsofe Wheeler <sitsofe@yahoo.com> wrote:
>> > Sitsofe, could you please check very latest tip/master with
>> > CONFIG_KMEMCHECK=y, does it find any other uninitialized memory access?
>>
>> No other uninitialized memory access so far (although having kmemcheck on does seem to provoke rcu stall warnings)...
Does that also mean that the DRM patch fixed the first one? :-)
>>
>> ...I take it back. This just turned up:
>> [ 992.417019] WARNING: kmemcheck: Caught 32-bit read from uninitialized memory (f2363d14)
>> [ 992.417033] 000110000002200061635f61646170746572000000000000cc2c030041433000
>> [ 992.417077] i i 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 i i i i
>> [ 992.417117] ^
>> [ 992.417121]
>> [ 992.417127] Pid: 1893, comm: acpid Not tainted (2.6.27-tipskw-00088-g9f41241-dirty #84) 900
>> [ 992.417134] EIP: 0060:[<c025fbdd>] EFLAGS: 00000286 CPU: 0
>> [ 992.417147] EIP is at acpi_bus_receive_event+0xd6/0x109
>> [ 992.417153] EAX: 00054489 EBX: f2363d00 ECX: 00000006 EDX: ffffffed
>> [ 992.417158] ESI: f2363d14 EDI: f6057f28 EBP: f6057f08 ESP: c0566d68
>> [ 992.417164] DS: 007b ES: 007b FS: 0000 GS: 0033 SS: 0068
>> [ 992.417169] CR0: 8005003b CR2: f6671034 CR3: 360ea000 CR4: 000006c0
>> [ 992.417175] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
>> [ 992.417180] DR6: ffff4ff0 DR7: 00000400
>> [ 992.417184] [<c026b86f>] acpi_system_read_event+0x49/0xc5
>> [ 992.417195] [<c01b2381>] proc_reg_read+0x61/0x90
>> [ 992.417206] [<c017efb5>] vfs_read+0x95/0x120
>> [ 992.417215] [<c017f5f2>] sys_read+0x42/0x70
>> [ 992.417222] [<c010336d>] sysenter_do_call+0x12/0x35
>> [ 992.417230] [<ffffffff>] 0xffffffff
>
> this too could be a real bug i think, uncovered by kmemcheck. Vegard?
No, it looks OK.
acpi_bus_receive_event() gets an entry off the acpi_bus_event_list and
copies it to the "struct acpi_bus_event event;" found in
acpi_system_read_event. So it's a dynamic-memory-to-stack copy.
It is added to the list in acpi_bus_generate_proc_event4(), which also
allocates the event and copies some strings into it:
strcpy(event->device_class, device_class);
strcpy(event->bus_id, bus_id);
And these are defined as character arrays:
typedef char acpi_device_class[20];
typedef char acpi_bus_id[5];
...
struct acpi_bus_event {
struct list_head node;
acpi_device_class device_class;
acpi_bus_id bus_id;
It would be cool to be track the stack as well (can we tell #PF to
switch stacks?). Or maybe allow memcpy() of anything to stack, that
shouldn't be too hard. Again, it's a balance. Allowing too much in
general will throw the child out with the bathwater. Maybe the easiest
solution for now is to annotate them. We can do it with the bitfields
API in two lines of code extra.
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] drm: fix leak of uninitialized data to userspace (acpi_system_read_event)
2008-10-10 15:26 ` Vegard Nossum
@ 2008-10-10 15:32 ` Ingo Molnar
0 siblings, 0 replies; 5+ messages in thread
From: Ingo Molnar @ 2008-10-10 15:32 UTC (permalink / raw)
To: Vegard Nossum
Cc: Sitsofe Wheeler, Vegard Nossum, Dave Airlie, Pekka Enberg,
linux-kernel
* Vegard Nossum <vegard.nossum@gmail.com> wrote:
> On Fri, Oct 10, 2008 at 4:37 PM, Ingo Molnar <mingo@elte.hu> wrote:
> > * Sitsofe Wheeler <sitsofe@yahoo.com> wrote:
> >> > Sitsofe, could you please check very latest tip/master with
> >> > CONFIG_KMEMCHECK=y, does it find any other uninitialized memory access?
> >>
> >> No other uninitialized memory access so far (although having kmemcheck on does seem to provoke rcu stall warnings)...
>
> Does that also mean that the DRM patch fixed the first one? :-)
>
> >>
> >> ...I take it back. This just turned up:
> >> [ 992.417019] WARNING: kmemcheck: Caught 32-bit read from uninitialized memory (f2363d14)
> >> [ 992.417033] 000110000002200061635f61646170746572000000000000cc2c030041433000
> >> [ 992.417077] i i 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 i i i i
> >> [ 992.417117] ^
> >> [ 992.417121]
> >> [ 992.417127] Pid: 1893, comm: acpid Not tainted (2.6.27-tipskw-00088-g9f41241-dirty #84) 900
> >> [ 992.417134] EIP: 0060:[<c025fbdd>] EFLAGS: 00000286 CPU: 0
> >> [ 992.417147] EIP is at acpi_bus_receive_event+0xd6/0x109
> >> [ 992.417153] EAX: 00054489 EBX: f2363d00 ECX: 00000006 EDX: ffffffed
> >> [ 992.417158] ESI: f2363d14 EDI: f6057f28 EBP: f6057f08 ESP: c0566d68
> >> [ 992.417164] DS: 007b ES: 007b FS: 0000 GS: 0033 SS: 0068
> >> [ 992.417169] CR0: 8005003b CR2: f6671034 CR3: 360ea000 CR4: 000006c0
> >> [ 992.417175] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
> >> [ 992.417180] DR6: ffff4ff0 DR7: 00000400
> >> [ 992.417184] [<c026b86f>] acpi_system_read_event+0x49/0xc5
> >> [ 992.417195] [<c01b2381>] proc_reg_read+0x61/0x90
> >> [ 992.417206] [<c017efb5>] vfs_read+0x95/0x120
> >> [ 992.417215] [<c017f5f2>] sys_read+0x42/0x70
> >> [ 992.417222] [<c010336d>] sysenter_do_call+0x12/0x35
> >> [ 992.417230] [<ffffffff>] 0xffffffff
> >
> > this too could be a real bug i think, uncovered by kmemcheck. Vegard?
>
> No, it looks OK.
>
> acpi_bus_receive_event() gets an entry off the acpi_bus_event_list and
> copies it to the "struct acpi_bus_event event;" found in
> acpi_system_read_event. So it's a dynamic-memory-to-stack copy.
>
> It is added to the list in acpi_bus_generate_proc_event4(), which also
> allocates the event and copies some strings into it:
>
> strcpy(event->device_class, device_class);
> strcpy(event->bus_id, bus_id);
>
> And these are defined as character arrays:
>
> typedef char acpi_device_class[20];
> typedef char acpi_bus_id[5];
> ...
> struct acpi_bus_event {
> struct list_head node;
> acpi_device_class device_class;
> acpi_bus_id bus_id;
>
> It would be cool to be track the stack as well (can we tell #PF to
> switch stacks?). [...]
yeah, the stack would be very interesting to track. Wont be too fast but
that's part of the deal ...
> [...] Or maybe allow memcpy() of anything to stack, that shouldn't be
> too hard. Again, it's a balance. Allowing too much in general will
> throw the child out with the bathwater. Maybe the easiest solution for
> now is to annotate them. We can do it with the bitfields API in two
> lines of code extra.
i'd suggest to go for maximum detection power, with smart annotations to
avoid all false positives. Perhaps with tricks to improve the code in
other ways too so that we dont have to a change 'only' for annotation
purposes.
For example many lockdep annotations document the locking rules of a
given piece of code - that's very useful independently of the
correctness checking aspect.
Ingo
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2008-10-11 7:55 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-10-11 7:55 [PATCH] drm: fix leak of uninitialized data to userspace (acpi_system_read_event) Sitsofe Wheeler
-- strict thread matches above, loose matches on Subject: below --
2008-10-10 13:26 Sitsofe Wheeler
2008-10-10 14:37 ` Ingo Molnar
2008-10-10 15:26 ` Vegard Nossum
2008-10-10 15:32 ` Ingo Molnar
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox