v2.6.28-rc1: readlink /proc/*/exe returns uninitialized data to userspace

v2.6.28-rc1: readlink /proc/*/exe returns uninitialized data to userspace

[Vegard Nossum]

Hi,

When I run readlink on the /proc/*/exe-file for udevd, the kernel
returns some unitialized data to userspace:

# strace -e trace=readlink readlink /proc/4762/exe
readlink("/proc/4762/exe", "/sbin/udevd", 1025) = 30

You can see it because the kernel thinks that the string is 30 bytes
long, but in fact it is only 12 (including the '\0').

If we explicitly clear the buffer before calling readlink, we can also
see that some garbage has been filled in there, after the string:

# ./readlink /proc/4762/exe
readlink(/proc/4762/exe) = 30
2f7362696e2f7564657664000000ffffffad4effffffadffffffdeffffffffffffffff202864656c657465642900000000000000000000000000000

(Output is from following simple program:)

#include <stdio.h>
#include <string.h>
#include <unistd.h>

int main(int argc, char *argv[])
{
        char buf[1024];
        int i;
        ssize_t n;

        memset(buf, 0, sizeof(buf));
        n = readlink(argv[1], buf, sizeof(buf));

        printf("readlink(%s) = %d\n", argv[1], n);

        for (i = 0; i < sizeof(buf); ++i)
                printf("%02x", buf[i]);
        printf("\n");

        return 0;
}

It was discovered by kmemcheck:

WARNING: kmemcheck: Caught 32-bit read from uninitialized memory (f6a109e4)
64000000ad4eaddeffffffffffffffff000000000200000000000000c0838ff8
 i i u u u u u u u u u u u u u u u u u u u u u u u u u u u u u u
         ^

Pid: 21511, comm: readlink Not tainted (2.6.28-rc1 #58) 945P-A
EIP: 0060:[<c04f988d>] EFLAGS: 00000296 CPU: 0
EIP is at __d_path+0x8d/0x1c0
EAX: 0000000e EBX: d7ba0fe7 ECX: 00000001 EDX: f68b0b40
ESI: f6a109e4 EDI: d7ba0fef EBP: e58c3f28 ESP: c2569c08
 DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
CR0: 8005003b CR2: f6c1d704 CR3: 31fc7000 CR4: 00000650
DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
DR6: ffff4ff0 DR7: 00000400
 [<c04fa4b0>] d_path+0xb0/0xd0
 [<c052c37c>] proc_pid_readlink+0x6c/0xc0
 [<c04eda34>] sys_readlinkat+0x94/0xa0
 [<c04eda67>] sys_readlink+0x27/0x30
 [<c0422f83>] sysenter_do_call+0x12/0x3f
 [<ffffffff>] 0xffffffff

Line numbers are these (as of commit
e013e13bf605b9e6b702adffbe2853cfc60e7806 in Linus's tree):

$ addr2line -e vmlinux -i c04f988d c04fa4b0 c052c37c c04eda34 c04eda67
fs/dcache.c:1895
fs/dcache.c:1901
fs/dcache.c:1957
fs/dcache.c:2016
fs/proc/base.c:1347
fs/proc/base.c:1374
fs/stat.c:312
fs/stat.c:325

I couldn't immediately figure out who/what to blame, please Cc in
right direction if you think you know it :-)

(For the record: This didn't show up in 2.6.27-rc with the same
version of LTP, so it seems to be a recent regression.)


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

Re: v2.6.28-rc1: readlink /proc/*/exe returns uninitialized data to userspace

[Rafael J. Wysocki]

On Saturday, 25 of October 2008, Vegard Nossum wrote:
> Hi,
> 
> When I run readlink on the /proc/*/exe-file for udevd, the kernel
> returns some unitialized data to userspace:
> 
> # strace -e trace=readlink readlink /proc/4762/exe
> readlink("/proc/4762/exe", "/sbin/udevd", 1025) = 30
> 
> You can see it because the kernel thinks that the string is 30 bytes
> long, but in fact it is only 12 (including the '\0').
> 
> If we explicitly clear the buffer before calling readlink, we can also
> see that some garbage has been filled in there, after the string:
> 
> # ./readlink /proc/4762/exe
> readlink(/proc/4762/exe) = 30
> 2f7362696e2f7564657664000000ffffffad4effffffadffffffdeffffffffffffffff202864656c657465642900000000000000000000000000000
> 
> (Output is from following simple program:)
> 
> #include <stdio.h>
> #include <string.h>
> #include <unistd.h>
> 
> int main(int argc, char *argv[])
> {
>         char buf[1024];
>         int i;
>         ssize_t n;
> 
>         memset(buf, 0, sizeof(buf));
>         n = readlink(argv[1], buf, sizeof(buf));
> 
>         printf("readlink(%s) = %d\n", argv[1], n);
> 
>         for (i = 0; i < sizeof(buf); ++i)
>                 printf("%02x", buf[i]);
>         printf("\n");
> 
>         return 0;
> }
> 
> It was discovered by kmemcheck:
> 
> WARNING: kmemcheck: Caught 32-bit read from uninitialized memory (f6a109e4)
> 64000000ad4eaddeffffffffffffffff000000000200000000000000c0838ff8
>  i i u u u u u u u u u u u u u u u u u u u u u u u u u u u u u u
>          ^
> 
> Pid: 21511, comm: readlink Not tainted (2.6.28-rc1 #58) 945P-A
> EIP: 0060:[<c04f988d>] EFLAGS: 00000296 CPU: 0
> EIP is at __d_path+0x8d/0x1c0
> EAX: 0000000e EBX: d7ba0fe7 ECX: 00000001 EDX: f68b0b40
> ESI: f6a109e4 EDI: d7ba0fef EBP: e58c3f28 ESP: c2569c08
>  DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
> CR0: 8005003b CR2: f6c1d704 CR3: 31fc7000 CR4: 00000650
> DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
> DR6: ffff4ff0 DR7: 00000400
>  [<c04fa4b0>] d_path+0xb0/0xd0
>  [<c052c37c>] proc_pid_readlink+0x6c/0xc0
>  [<c04eda34>] sys_readlinkat+0x94/0xa0
>  [<c04eda67>] sys_readlink+0x27/0x30
>  [<c0422f83>] sysenter_do_call+0x12/0x3f
>  [<ffffffff>] 0xffffffff
> 
> Line numbers are these (as of commit
> e013e13bf605b9e6b702adffbe2853cfc60e7806 in Linus's tree):
> 
> $ addr2line -e vmlinux -i c04f988d c04fa4b0 c052c37c c04eda34 c04eda67
> fs/dcache.c:1895
> fs/dcache.c:1901
> fs/dcache.c:1957
> fs/dcache.c:2016
> fs/proc/base.c:1347
> fs/proc/base.c:1374
> fs/stat.c:312
> fs/stat.c:325
> 
> I couldn't immediately figure out who/what to blame, please Cc in
> right direction if you think you know it :-)

Well, I only can say who may be interested (CCs added).

Thanks,
Rafael

Re: v2.6.28-rc1: readlink /proc/*/exe returns uninitialized data to userspace

[Eric W. Biederman]

"Rafael J. Wysocki" <rjw@sisk.pl> writes:

> On Saturday, 25 of October 2008, Vegard Nossum wrote:
>> Hi,
>> 
>> When I run readlink on the /proc/*/exe-file for udevd, the kernel
>> returns some unitialized data to userspace:
>> 
>> # strace -e trace=readlink readlink /proc/4762/exe
>> readlink("/proc/4762/exe", "/sbin/udevd", 1025) = 30
>> 
>> You can see it because the kernel thinks that the string is 30 bytes
>> long, but in fact it is only 12 (including the '\0').
>> 
>> If we explicitly clear the buffer before calling readlink, we can also
>> see that some garbage has been filled in there, after the string:
>> 
>> # ./readlink /proc/4762/exe
>> readlink(/proc/4762/exe) = 30
>>
> 2f7362696e2f7564657664000000ffffffad4effffffadffffffdeffffffffffffffff202864656c657465642900000000000000000000000000000
>> 
>> (Output is from following simple program:)
>> 
>> #include <stdio.h>
>> #include <string.h>
>> #include <unistd.h>
>> 
>> int main(int argc, char *argv[])
>> {
>>         char buf[1024];
>>         int i;
>>         ssize_t n;
>> 
>>         memset(buf, 0, sizeof(buf));
>>         n = readlink(argv[1], buf, sizeof(buf));
>> 
>>         printf("readlink(%s) = %d\n", argv[1], n);
>> 
>>         for (i = 0; i < sizeof(buf); ++i)
>>                 printf("%02x", buf[i]);
>>         printf("\n");
>> 
>>         return 0;
>> }
>> 
>> It was discovered by kmemcheck:
>> 
>> WARNING: kmemcheck: Caught 32-bit read from uninitialized memory (f6a109e4)
>> 64000000ad4eaddeffffffffffffffff000000000200000000000000c0838ff8
>>  i i u u u u u u u u u u u u u u u u u u u u u u u u u u u u u u
>>          ^
>> 
>> Pid: 21511, comm: readlink Not tainted (2.6.28-rc1 #58) 945P-A
>> EIP: 0060:[<c04f988d>] EFLAGS: 00000296 CPU: 0
>> EIP is at __d_path+0x8d/0x1c0
>> EAX: 0000000e EBX: d7ba0fe7 ECX: 00000001 EDX: f68b0b40
>> ESI: f6a109e4 EDI: d7ba0fef EBP: e58c3f28 ESP: c2569c08
>>  DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
>> CR0: 8005003b CR2: f6c1d704 CR3: 31fc7000 CR4: 00000650
>> DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
>> DR6: ffff4ff0 DR7: 00000400
>>  [<c04fa4b0>] d_path+0xb0/0xd0
>>  [<c052c37c>] proc_pid_readlink+0x6c/0xc0
>>  [<c04eda34>] sys_readlinkat+0x94/0xa0
>>  [<c04eda67>] sys_readlink+0x27/0x30
>>  [<c0422f83>] sysenter_do_call+0x12/0x3f
>>  [<ffffffff>] 0xffffffff
>> 
>> Line numbers are these (as of commit
>> e013e13bf605b9e6b702adffbe2853cfc60e7806 in Linus's tree):
>> 
>> $ addr2line -e vmlinux -i c04f988d c04fa4b0 c052c37c c04eda34 c04eda67
>> fs/dcache.c:1895
>> fs/dcache.c:1901
>> fs/dcache.c:1957
>> fs/dcache.c:2016
>> fs/proc/base.c:1347
>> fs/proc/base.c:1374
>> fs/stat.c:312
>> fs/stat.c:325
>> 
>> I couldn't immediately figure out who/what to blame, please Cc in
>> right direction if you think you know it :-)
>
> Well, I only can say who may be interested (CCs added).

Weird.  The dentry for "udevd" has an incorrect length.
Is something stomping the length somewhere?

What filesystem does /sbin/udevd reside on?

Eric

Re: v2.6.28-rc1: readlink /proc/*/exe returns uninitialized data to userspace

[Al Viro]

On Sat, Oct 25, 2008 at 07:14:21PM +0200, Vegard Nossum wrote:
> It was discovered by kmemcheck:
> 
> WARNING: kmemcheck: Caught 32-bit read from uninitialized memory (f6a109e4)
> 64000000ad4eaddeffffffffffffffff000000000200000000000000c0838ff8
>  i i u u u u u u u u u u u u u u u u u u u u u u u u u u u u u u
>          ^
> 
> Pid: 21511, comm: readlink Not tainted (2.6.28-rc1 #58) 945P-A
> EIP: 0060:[<c04f988d>] EFLAGS: 00000296 CPU: 0
> EIP is at __d_path+0x8d/0x1c0
> EAX: 0000000e EBX: d7ba0fe7 ECX: 00000001 EDX: f68b0b40
> ESI: f6a109e4 EDI: d7ba0fef EBP: e58c3f28 ESP: c2569c08
>  DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
> CR0: 8005003b CR2: f6c1d704 CR3: 31fc7000 CR4: 00000650
> DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
> DR6: ffff4ff0 DR7: 00000400
>  [<c04fa4b0>] d_path+0xb0/0xd0
>  [<c052c37c>] proc_pid_readlink+0x6c/0xc0
>  [<c04eda34>] sys_readlinkat+0x94/0xa0
>  [<c04eda67>] sys_readlink+0x27/0x30
>  [<c0422f83>] sysenter_do_call+0x12/0x3f
>  [<ffffffff>] 0xffffffff
> 
> Line numbers are these (as of commit
> e013e13bf605b9e6b702adffbe2853cfc60e7806 in Linus's tree):
> 
> $ addr2line -e vmlinux -i c04f988d c04fa4b0 c052c37c c04eda34 c04eda67
> fs/dcache.c:1895
> fs/dcache.c:1901
> fs/dcache.c:1957
> fs/dcache.c:2016
> fs/proc/base.c:1347
> fs/proc/base.c:1374
> fs/stat.c:312
> fs/stat.c:325
> 
> I couldn't immediately figure out who/what to blame, please Cc in
> right direction if you think you know it :-)
> 
> (For the record: This didn't show up in 2.6.27-rc with the same
> version of LTP, so it seems to be a recent regression.)

Very odd.  Do you see that for any other processes?  Where does
/sbin/udevd live on your box?  BTW, .config might be useful here...

Can you reproduce that on e.g. amd64 and/or without kmemcheck?

Re: v2.6.28-rc1: readlink /proc/*/exe returns uninitialized data to userspace

[Vegard Nossum]

[Combined result for Eric & Viro]

On Sat, Oct 25, 2008 at 11:28 PM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
>> On Saturday, 25 of October 2008, Vegard Nossum wrote:
>>> Hi,
>>>
>>> When I run readlink on the /proc/*/exe-file for udevd, the kernel
>>> returns some unitialized data to userspace:
>>>
>>> # strace -e trace=readlink readlink /proc/4762/exe
>>> readlink("/proc/4762/exe", "/sbin/udevd", 1025) = 30
>>>
>>> You can see it because the kernel thinks that the string is 30 bytes
>>> long, but in fact it is only 12 (including the '\0').
...

> Weird.  The dentry for "udevd" has an incorrect length.
> Is something stomping the length somewhere?
>
> What filesystem does /sbin/udevd reside on?

Ext3 on a USB flash-disk.

On Sun, Oct 26, 2008 at 1:23 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>> (For the record: This didn't show up in 2.6.27-rc with the same
>> version of LTP, so it seems to be a recent regression.)
>
> Very odd.  Do you see that for any other processes?  Where does
> /sbin/udevd live on your box?  BTW, .config might be useful here...
>
> Can you reproduce that on e.g. amd64 and/or without kmemcheck?

IIRC, it did show up for other processes, but udevd was the only one
which exhibited the problem reliably.

Now, I've been trying to reproduce the problem (with exact same setup)
since I first saw it, but can't :-/ At the time that the machine
started showing the problem, it had been running LTP, scrashme, etc.
for hours, so it seems that it might have had something to do with it.
I couldn't reproduce it after rebooting.

This was my setup:
 - root filesystem (ext3) on USB flash disk
 - mounted LVM2/ext3 from harddisk on /mnt
 - bind-mounted /proc onto /mnt/proc

I noticed the problem from chroot /mnt, but it reproduced afterwards
on the outside as well. I also remember having remounted (with -o
remount) both /mnt (adding user_xattr to options) and /mnt/proc from
within the chroot (so with /mnt prefix removed). This could of course
all be unrelated since it didn't reproduce the problem, but at least
it is what I did.

Cosmic rays may have been involved. Will keep trying to reproduce.
Thanks for attention so far.


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

Re: v2.6.28-rc1: readlink /proc/*/exe returns uninitialized data to userspace

[Vegard Nossum]

On Sun, Oct 26, 2008 at 10:08 PM, Vegard Nossum <vegard.nossum@gmail.com> wrote:
> [Combined result for Eric & Viro]
>
> On Sat, Oct 25, 2008 at 11:28 PM, Eric W. Biederman
> <ebiederm@xmission.com> wrote:
>>> On Saturday, 25 of October 2008, Vegard Nossum wrote:
>>>> Hi,
>>>>
>>>> When I run readlink on the /proc/*/exe-file for udevd, the kernel
>>>> returns some unitialized data to userspace:
>>>>
>>>> # strace -e trace=readlink readlink /proc/4762/exe
>>>> readlink("/proc/4762/exe", "/sbin/udevd", 1025) = 30
>>>>
>>>> You can see it because the kernel thinks that the string is 30 bytes
>>>> long, but in fact it is only 12 (including the '\0').
> ...
>
>> Weird.  The dentry for "udevd" has an incorrect length.
>> Is something stomping the length somewhere?
>>
>> What filesystem does /sbin/udevd reside on?
>
> Ext3 on a USB flash-disk.
>
> On Sun, Oct 26, 2008 at 1:23 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>>> (For the record: This didn't show up in 2.6.27-rc with the same
>>> version of LTP, so it seems to be a recent regression.)
>>
>> Very odd.  Do you see that for any other processes?  Where does
>> /sbin/udevd live on your box?  BTW, .config might be useful here...
>>
>> Can you reproduce that on e.g. amd64 and/or without kmemcheck?
>
> IIRC, it did show up for other processes, but udevd was the only one
> which exhibited the problem reliably.
>
> Now, I've been trying to reproduce the problem (with exact same setup)
> since I first saw it, but can't :-/ At the time that the machine
> started showing the problem, it had been running LTP, scrashme, etc.
> for hours, so it seems that it might have had something to do with it.
> I couldn't reproduce it after rebooting.
>
> This was my setup:
>  - root filesystem (ext3) on USB flash disk
>  - mounted LVM2/ext3 from harddisk on /mnt
>  - bind-mounted /proc onto /mnt/proc
>
> I noticed the problem from chroot /mnt, but it reproduced afterwards
> on the outside as well. I also remember having remounted (with -o
> remount) both /mnt (adding user_xattr to options) and /mnt/proc from
> within the chroot (so with /mnt prefix removed). This could of course
> all be unrelated since it didn't reproduce the problem, but at least
> it is what I did.
>
> Cosmic rays may have been involved. Will keep trying to reproduce.
> Thanks for attention so far.

Hello,

I received an e-mail from Yoshiya Koyama today with a reproducible
test case (see bottom of e-mail).

Here's with linux-next from October 31:

# uname -a
Linux ubuntu 2.6.28-rc2-next-20081031 #60 SMP Sat Nov 1 13:19:49 CET
2008 i686 GNU/Linux
# prelink -mRf /sbin/udevd
# ./a.out /proc/4764/exe
warning: /proc/4764/exe: got return value 38, expected 11
2f7362696e2f756465766400fffffffffdfffffffffffff7ffffbfff202864656c6574656429
/sbin/udevd                  (deleted)

...and in system log:

WARNING: kmemcheck: Caught 32-bit read from uninitialized memory (f61c26c0)
fffffdfffffffffffff7ffffbfffffffffffffefefffffafefffffffffeffffb
 u u u u u u u u u u u u u u u u u u u u u u u u u u u u a a a a
 ^

Pid: 8118, comm: a.out Not tainted (2.6.28-rc2-next-20081031 #60) 945P-A
EIP: 0060:[<c04fb8ed>] EFLAGS: 00000292 CPU: 0
EIP is at __d_path+0x8d/0x1c0
EAX: 00000016 EBX: f2f92fdf ECX: 00000003 EDX: f60aa280
ESI: f61c26c0 EDI: f2f92fe7 EBP: f3115f28 ESP: c24a0308
 DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
CR0: 8005003b CR2: f6629704 CR3: 331ab000 CR4: 00000650
DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
DR6: ffff4ff0 DR7: 00000400
 [<c04fc510>] d_path+0xb0/0xd0
 [<c052f4bc>] proc_pid_readlink+0x6c/0xc0
 [<c04ef794>] sys_readlinkat+0x94/0xa0
 [<c04ef7c7>] sys_readlink+0x27/0x30
 [<c0424fb3>] sysenter_do_call+0x12/0x3f
 [<ffffffff>] 0xffffffff
WARNING: kmemcheck: Caught 8-bit read from uninitialized memory (f61c26cc)
fffffdfffffffffffff7ffffbfffffffffffffefefffffafefffffffffeffffb
 u u u u u u u u u u u u u u u u u u u u u u u u u u u u a a a a
                         ^

Pid: 8118, comm: a.out Not tainted (2.6.28-rc2-next-20081031 #60) 945P-A
EIP: 0060:[<c04fb8f5>] EFLAGS: 00010202 CPU: 0
EIP is at __d_path+0x95/0x1c0
EAX: 00000016 EBX: f2f92fdf ECX: 00000002 EDX: f60aa280
ESI: f61c26cc EDI: f2f92ff3 EBP: f3115f28 ESP: c24a0428
 DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
CR0: 8005003b CR2: f6629704 CR3: 331ab000 CR4: 00000650
DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
DR6: ffff4ff0 DR7: 00000400
 [<c04fc510>] d_path+0xb0/0xd0
 [<c052f4bc>] proc_pid_readlink+0x6c/0xc0
 [<c04ef794>] sys_readlinkat+0x94/0xa0
 [<c04ef7c7>] sys_readlink+0x27/0x30
 [<c0424fb3>] sysenter_do_call+0x12/0x3f
 [<ffffffff>] 0xffffffff
WARNING: kmemcheck: Caught 8-bit read from uninitialized memory (f61c26cd)
fffffdfffffffffffff7ffffbfffffffffffffefefffffafefffffffffeffffb
 u u u u u u u u u u u u u u u u u u u u u u u u u u u u a a a a
                           ^

Pid: 8118, comm: a.out Not tainted (2.6.28-rc2-next-20081031 #60) 945P-A
EIP: 0060:[<c04fb8f6>] EFLAGS: 00000202 CPU: 0
EIP is at __d_path+0x96/0x1c0
EAX: 00000016 EBX: f2f92fdf ECX: 00000001 EDX: f60aa280
ESI: f61c26cd EDI: f2f92ff4 EBP: f3115f28 ESP: c24a0548
 DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
CR0: 8005003b CR2: f6629704 CR3: 331ab000 CR4: 00000650
DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
DR6: ffff4ff0 DR7: 00000400
 [<c04fc510>] d_path+0xb0/0xd0
 [<c052f4bc>] proc_pid_readlink+0x6c/0xc0
 [<c04ef794>] sys_readlinkat+0x94/0xa0
 [<c04ef7c7>] sys_readlink+0x27/0x30
 [<c0424fb3>] sysenter_do_call+0x12/0x3f
 [<ffffffff>] 0xffffffff

Here's Fedora 9 (no kmemcheck):

# uname -a
Linux localhost.localdomain 2.6.25.11-97.fc9.i686 #1 SMP Mon Jul 21
01:31:09 EDT 2008 i686 i686 i386 GNU/Linux
# prelink -mRf /sbin/udevd
# ./a.out /proc/564/exe
warning: /proc/564/exe: got return value 38, expected 11
2f7362696e2f7564657664005f47387942426e5952446e566f306868202864656c6574656429
/sbin/udevd _G8yBBnYRDnVo0hh (deleted)

Yoshiya Koyama reports that the problem exists on RHEL 2.6.9-42.ELsmp too.

I don't think it's exactly the same problem as originally reported,
because I definitely wasn't using prelinking (the prelink binary
wasn't even installed on the machine until today). But finding the
root cause of this may solve both problems.


Vegard


---------- Forwarded message ----------
From: Koyama, Yoshiya <Yoshiya.Koyama@hp.com>
Date: Tue, Nov 4, 2008 at 5:00 AM
Subject: Re: v2.6.28-rc1: readlink /proc/*/exe returns uninitialized
data to userspace
To: "vegard.nossum@gmail.com" <vegard.nossum@gmail.com>


Hello Vegard,

I'm Yoshiya Koyama

I happen to encounter the same readlink problem you reported to the LKML.
I don't know my case is exactly the same one as yours, but I can reproduce it.
How to reproduce:
# prelink -mRf /sbin/udevd
After prelinking the binary, you can see garbages returned from readlink.
When you restart udevd, the problem will disappear.
I'm running RHEL 2.6.9-42.ELsmp.

Hope this help
---
Yoshiya Koyama / Hewlett-Packard Japan, Ltd. 03-5349-7320 (FAX 7464)
mailto:Yoshiya.Koyama@hp.com

Re: v2.6.28-rc1: readlink /proc/*/exe returns uninitialized data to userspace

[Alexey Dobriyan]

On Tue, Nov 04, 2008 at 10:39:19AM +0100, Vegard Nossum wrote:
> # uname -a
> Linux ubuntu 2.6.28-rc2-next-20081031 #60 SMP Sat Nov 1 13:19:49 CET
> 2008 i686 GNU/Linux
> # prelink -mRf /sbin/udevd
> # ./a.out /proc/4764/exe
> warning: /proc/4764/exe: got return value 38, expected 11
> 2f7362696e2f756465766400fffffffffdfffffffffffff7ffffbfff202864656c6574656429
> /sbin/udevd                  (deleted)

reproduced

As I said previously, kmemcheck rocks (slowly). :-)

Re: v2.6.28-rc1: readlink /proc/*/exe returns uninitialized data to userspace

[Alexey Dobriyan]

On Tue, Nov 04, 2008 at 01:00:50PM +0300, Alexey Dobriyan wrote:
> On Tue, Nov 04, 2008 at 10:39:19AM +0100, Vegard Nossum wrote:
> > # uname -a
> > Linux ubuntu 2.6.28-rc2-next-20081031 #60 SMP Sat Nov 1 13:19:49 CET
> > 2008 i686 GNU/Linux
> > # prelink -mRf /sbin/udevd
> > # ./a.out /proc/4764/exe
> > warning: /proc/4764/exe: got return value 38, expected 11
> > 2f7362696e2f756465766400fffffffffdfffffffffffff7ffffbfff202864656c6574656429
> > /sbin/udevd                  (deleted)
> 
> reproduced

prelink does

	rename("/sbin/udevd.#prelink#.N6pvOJ", "/sbin/udevd")

Re: v2.6.28-rc1: readlink /proc/*/exe returns uninitialized data to userspace

[Alexey Dobriyan]

On Tue, Nov 04, 2008 at 01:07:57PM +0300, Alexey Dobriyan wrote:
> On Tue, Nov 04, 2008 at 01:00:50PM +0300, Alexey Dobriyan wrote:
> > On Tue, Nov 04, 2008 at 10:39:19AM +0100, Vegard Nossum wrote:
> > > # uname -a
> > > Linux ubuntu 2.6.28-rc2-next-20081031 #60 SMP Sat Nov 1 13:19:49 CET
> > > 2008 i686 GNU/Linux
> > > # prelink -mRf /sbin/udevd
> > > # ./a.out /proc/4764/exe
> > > warning: /proc/4764/exe: got return value 38, expected 11
> > > 2f7362696e2f756465766400fffffffffdfffffffffffff7ffffbfff202864656c6574656429
> > > /sbin/udevd                  (deleted)
> > 
> > reproduced
> 
> prelink does
> 
> 	rename("/sbin/udevd.#prelink#.N6pvOJ", "/sbin/udevd")

OK, here is obvious (lame) reproducer, second readlink returns 32 while first
returns 18:

#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <stdio.h>
#include <string.h>
#include <unistd.h>

int main(void)
{
	int fd, fd1;
	char buf[64], buf1[64], img[42000];
	ssize_t len;

	fd = open("/proc/self/exe", O_RDONLY);

	readlink("/proc/self/exe", buf, sizeof(buf));
	strcpy(buf1, buf);
	strcat(buf1, ".xxx");
	unlink(buf1);
	fd1 = open(buf1, O_WRONLY|O_CREAT);
	len = read(fd, img, sizeof(img));
	close(fd);
	write(fd1, img, len);
	close(fd1);
	rename(buf1, buf);

	readlink("/proc/self/exe", buf, sizeof(buf));

	return 0;
}

Re: v2.6.28-rc1: readlink /proc/*/exe returns uninitialized data to userspace

[Eric W. Biederman]

Alexey Dobriyan <adobriyan@gmail.com> writes:

> On Tue, Nov 04, 2008 at 10:39:19AM +0100, Vegard Nossum wrote:
>> # uname -a
>> Linux ubuntu 2.6.28-rc2-next-20081031 #60 SMP Sat Nov 1 13:19:49 CET
>> 2008 i686 GNU/Linux
>> # prelink -mRf /sbin/udevd
>> # ./a.out /proc/4764/exe
>> warning: /proc/4764/exe: got return value 38, expected 11
>> 2f7362696e2f756465766400fffffffffdfffffffffffff7ffffbfff202864656c6574656429
>> /sbin/udevd                  (deleted)
>
> reproduced
>
> As I said previously, kmemcheck rocks (slowly). :-)

It is reproducible here as well.  At least to the point of the
strange readlink length.

prelink generates a new executable and renames it on top
of the old executable.  So I'm guessing something on the unlink
and rename path is what is giving us the strange length.

Hmm.  The string: '/sbin/udevd.#prelink#.J9NyXV (deleted)'
is 38 bytes long...   So I'm guessing d_move is doing something
wrong and we are not seeing the name string we expect.

Why do we see /sbin/udevd and not /sbin/udevd.#prelink#.J9NyXV 
after d_move.  It looks like both names are short enough that
they are inline.

Oh.  I see.  switch_names when both names are internal,
does a memcpy of the new name to the target name,
but it doesn't do anything with the source name.
Then later we swap the name lengths.

So the length on the dentry no longer matches the data
we put in the buffer.

Certainly not a resource leak or any kind of deadlock.
And the length is right.  But it is an information leak.

I suppose a clever person could figure out how to steal
information that way.

The nice fix would be to keep the old length in this case,
so we don't have a name mangled because someone renamed
on top of us.  But that is inconsistent.


Eric

Re: v2.6.28-rc1: readlink /proc/*/exe returns uninitialized data to userspace

[Al Viro]

On Tue, Nov 04, 2008 at 10:39:19AM +0100, Vegard Nossum wrote:
> On Sun, Oct 26, 2008 at 10:08 PM, Vegard Nossum <vegard.nossum@gmail.com> wrote:
> # uname -a
> Linux localhost.localdomain 2.6.25.11-97.fc9.i686 #1 SMP Mon Jul 21
> 01:31:09 EDT 2008 i686 i686 i386 GNU/Linux
> # prelink -mRf /sbin/udevd
> # ./a.out /proc/564/exe
> warning: /proc/564/exe: got return value 38, expected 11
> 2f7362696e2f7564657664005f47387942426e5952446e566f306868202864656c6574656429
> /sbin/udevd _G8yBBnYRDnVo0hh (deleted)
> 
> Yoshiya Koyama reports that the problem exists on RHEL 2.6.9-42.ELsmp too.
> 
> I don't think it's exactly the same problem as originally reported,
> because I definitely wasn't using prelinking (the prelink binary
> wasn't even installed on the machine until today). But finding the
> root cause of this may solve both problems.

switch_names() buggered in case of short names on both sides.  That should
help:

>From 2acda856910b774717e0290bbf948c7dee0f2e1a Mon Sep 17 00:00:00 2001
From: Al Viro <viro@zeniv.linux.org.uk>
Date: Mon, 3 Nov 2008 15:03:50 -0500
Subject: [PATCH] fix switch_names() breakage in short-to-short case

We want ->name.len to match the resulting name on *both*
source and target

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/dcache.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/dcache.c b/fs/dcache.c
index a1d86c7..15e4f83 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1620,8 +1620,11 @@ static void switch_names(struct dentry *dentry, struct dentry *target)
 			 */
 			memcpy(dentry->d_iname, target->d_name.name,
 					target->d_name.len + 1);
+			dentry->d_name.len = target->d_name.len;
+			return;
 		}
 	}
+	do_switch(dentry->d_name.len, target->d_name.len);
 }
 
 /*
@@ -1681,7 +1684,6 @@ already_unhashed:
 
 	/* Switch the names.. */
 	switch_names(dentry, target);
-	do_switch(dentry->d_name.len, target->d_name.len);
 	do_switch(dentry->d_name.hash, target->d_name.hash);
 
 	/* ... and switch the parents */
@@ -1791,7 +1793,6 @@ static void __d_materialise_dentry(struct dentry *dentry, struct dentry *anon)
 	struct dentry *dparent, *aparent;
 
 	switch_names(dentry, anon);
-	do_switch(dentry->d_name.len, anon->d_name.len);
 	do_switch(dentry->d_name.hash, anon->d_name.hash);
 
 	dparent = dentry->d_parent;
-- 
1.5.6.5

Re: v2.6.28-rc1: readlink /proc/*/exe returns uninitialized data to userspace

[Al Viro]

On Tue, Nov 04, 2008 at 02:54:35AM -0800, Eric W. Biederman wrote:

> does a memcpy of the new name to the target name,
> but it doesn't do anything with the source name.
> Then later we swap the name lengths.
> 
> So the length on the dentry no longer matches the data
> we put in the buffer.

Aye.  BTW, here's yesterday IRC log, after eparis had pointed to
" (deleted)" in the ends of two more reproducers (auditd spew after
crond upgrade, FWIW - same issue with d_path()):

13:48 #sec-eng: < viro> eparis: it's switch_names()
13:49 #sec-eng: < viro> if both names are internal
13:49 #sec-eng: < viro> in that case we want to copy ->d_name.len instead of swapping those
13:50 #sec-eng: < viro> IOW, I understand what's going on; it's not too nasty, fortunately, but it needs fixing
13:51 #sec-eng: < viro> pathname ends on " (deleted)" and has sane path
13:51 #sec-eng: < viro> i.e. we have dentry->d_name.{name,len} responsible for everything in between
13:52 #sec-eng: < viro> and .len is more than it ought to be
13:52 #sec-eng: < viro> OTOH, it's still within the limit on name length, so it's embedded into struct dentry
13:53 #sec-eng: < viro> i.e. we had either unlink() doing something _very_ odd or rename() buggering the name/len for (unhashed) target
13:53 #sec-eng: < viro> rename() => d_move()
13:54 #sec-eng: < viro> then testing a bit with copying /bin/sh to /tmp/<different names>, starting those and doing mv(1) of one over another had happily reproduced it
13:55 #sec-eng: < viro> so that was d_move() with short-over-short and source name longer than target one
13:56 #sec-eng: < viro> since there are only two lines handling d_name there (
13:56 #sec-eng: < viro>         /* Switch the names.. */
13:56 #sec-eng: < viro>         switch_names(dentry, target);
13:56 #sec-eng: < viro>         do_switch(dentry->d_name.len, target->d_name.len);
13:57 #sec-eng: < viro> and switch_names() is 4-way choice by "is the source name short enough"/"is the target name short enough"...
13:58 #sec-eng: < viro> IOW, after noticing that " (deleted)" it had been absolutely straightforward
13:58 #sec-eng: < viro> it's not junk in the end
13:59 #sec-eng: < viro> it's junk in the _middle_

> Certainly not a resource leak or any kind of deadlock.
> And the length is right.  But it is an information leak.
> 
> I suppose a clever person could figure out how to steal
> information that way.

Not much, but that certainly needs fixing, leak or not.

Re: v2.6.28-rc1: readlink /proc/*/exe returns uninitialized data to userspace

[Ingo Molnar]

* Al Viro <viro@ZenIV.linux.org.uk> wrote:

> On Tue, Nov 04, 2008 at 10:39:19AM +0100, Vegard Nossum wrote:
> > On Sun, Oct 26, 2008 at 10:08 PM, Vegard Nossum <vegard.nossum@gmail.com> wrote:
> > # uname -a
> > Linux localhost.localdomain 2.6.25.11-97.fc9.i686 #1 SMP Mon Jul 21
> > 01:31:09 EDT 2008 i686 i686 i386 GNU/Linux
> > # prelink -mRf /sbin/udevd
> > # ./a.out /proc/564/exe
> > warning: /proc/564/exe: got return value 38, expected 11
> > 2f7362696e2f7564657664005f47387942426e5952446e566f306868202864656c6574656429
> > /sbin/udevd _G8yBBnYRDnVo0hh (deleted)
> > 
> > Yoshiya Koyama reports that the problem exists on RHEL 2.6.9-42.ELsmp too.
> > 
> > I don't think it's exactly the same problem as originally reported,
> > because I definitely wasn't using prelinking (the prelink binary
> > wasn't even installed on the machine until today). But finding the
> > root cause of this may solve both problems.
> 
> switch_names() buggered in case of short names on both sides.  That should
> help:
> 
> >From 2acda856910b774717e0290bbf948c7dee0f2e1a Mon Sep 17 00:00:00 2001
> From: Al Viro <viro@zeniv.linux.org.uk>
> Date: Mon, 3 Nov 2008 15:03:50 -0500
> Subject: [PATCH] fix switch_names() breakage in short-to-short case
> 
> We want ->name.len to match the resulting name on *both*
> source and target
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

please credit kmemcheck in the commit message and use an appropriate 
Reported-by line as well. Thanks,

	Ingo

Re: v2.6.28-rc1: readlink /proc/*/exe returns uninitialized data to userspace

[Greg KH]

On Thu, Nov 06, 2008 at 11:04:10AM +0100, Ingo Molnar wrote:
> 
> * Al Viro <viro@ZenIV.linux.org.uk> wrote:
> 
> > On Tue, Nov 04, 2008 at 10:39:19AM +0100, Vegard Nossum wrote:
> > > On Sun, Oct 26, 2008 at 10:08 PM, Vegard Nossum <vegard.nossum@gmail.com> wrote:
> > > # uname -a
> > > Linux localhost.localdomain 2.6.25.11-97.fc9.i686 #1 SMP Mon Jul 21
> > > 01:31:09 EDT 2008 i686 i686 i386 GNU/Linux
> > > # prelink -mRf /sbin/udevd
> > > # ./a.out /proc/564/exe
> > > warning: /proc/564/exe: got return value 38, expected 11
> > > 2f7362696e2f7564657664005f47387942426e5952446e566f306868202864656c6574656429
> > > /sbin/udevd _G8yBBnYRDnVo0hh (deleted)
> > > 
> > > Yoshiya Koyama reports that the problem exists on RHEL 2.6.9-42.ELsmp too.
> > > 
> > > I don't think it's exactly the same problem as originally reported,
> > > because I definitely wasn't using prelinking (the prelink binary
> > > wasn't even installed on the machine until today). But finding the
> > > root cause of this may solve both problems.
> > 
> > switch_names() buggered in case of short names on both sides.  That should
> > help:
> > 
> > >From 2acda856910b774717e0290bbf948c7dee0f2e1a Mon Sep 17 00:00:00 2001
> > From: Al Viro <viro@zeniv.linux.org.uk>
> > Date: Mon, 3 Nov 2008 15:03:50 -0500
> > Subject: [PATCH] fix switch_names() breakage in short-to-short case
> > 
> > We want ->name.len to match the resulting name on *both*
> > source and target
> > 
> > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> 
> please credit kmemcheck in the commit message and use an appropriate 
> Reported-by line as well. Thanks,

Did this fix ever get merged into Linus's tree?

thanks,

greg k-h

Re: v2.6.28-rc1: readlink /proc/*/exe returns uninitialized data to userspace

[Alexey Dobriyan]

On Fri, Nov 07, 2008 at 11:05:44AM -0800, Greg KH wrote:
> On Thu, Nov 06, 2008 at 11:04:10AM +0100, Ingo Molnar wrote:
> > 
> > * Al Viro <viro@ZenIV.linux.org.uk> wrote:
> > 
> > > On Tue, Nov 04, 2008 at 10:39:19AM +0100, Vegard Nossum wrote:
> > > > On Sun, Oct 26, 2008 at 10:08 PM, Vegard Nossum <vegard.nossum@gmail.com> wrote:
> > > > # uname -a
> > > > Linux localhost.localdomain 2.6.25.11-97.fc9.i686 #1 SMP Mon Jul 21
> > > > 01:31:09 EDT 2008 i686 i686 i386 GNU/Linux
> > > > # prelink -mRf /sbin/udevd
> > > > # ./a.out /proc/564/exe
> > > > warning: /proc/564/exe: got return value 38, expected 11
> > > > 2f7362696e2f7564657664005f47387942426e5952446e566f306868202864656c6574656429
> > > > /sbin/udevd _G8yBBnYRDnVo0hh (deleted)
> > > > 
> > > > Yoshiya Koyama reports that the problem exists on RHEL 2.6.9-42.ELsmp too.
> > > > 
> > > > I don't think it's exactly the same problem as originally reported,
> > > > because I definitely wasn't using prelinking (the prelink binary
> > > > wasn't even installed on the machine until today). But finding the
> > > > root cause of this may solve both problems.
> > > 
> > > switch_names() buggered in case of short names on both sides.  That should
> > > help:
> > > 
> > > >From 2acda856910b774717e0290bbf948c7dee0f2e1a Mon Sep 17 00:00:00 2001
> > > From: Al Viro <viro@zeniv.linux.org.uk>
> > > Date: Mon, 3 Nov 2008 15:03:50 -0500
> > > Subject: [PATCH] fix switch_names() breakage in short-to-short case
> > > 
> > > We want ->name.len to match the resulting name on *both*
> > > source and target
> > > 
> > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> > 
> > please credit kmemcheck in the commit message and use an appropriate 
> > Reported-by line as well. Thanks,
> 
> Did this fix ever get merged into Linus's tree?

So far no.

Re: v2.6.28-rc1: readlink /proc/*/exe returns uninitialized data to userspace

[Andrew Morton]

On Sat, 8 Nov 2008 02:12:05 +0300
Alexey Dobriyan <adobriyan@gmail.com> wrote:

> On Fri, Nov 07, 2008 at 11:05:44AM -0800, Greg KH wrote:
> > On Thu, Nov 06, 2008 at 11:04:10AM +0100, Ingo Molnar wrote:
> > > 
> > > * Al Viro <viro@ZenIV.linux.org.uk> wrote:
> > > 
> > > > On Tue, Nov 04, 2008 at 10:39:19AM +0100, Vegard Nossum wrote:
> > > > > On Sun, Oct 26, 2008 at 10:08 PM, Vegard Nossum <vegard.nossum@gmail.com> wrote:
> > > > > # uname -a
> > > > > Linux localhost.localdomain 2.6.25.11-97.fc9.i686 #1 SMP Mon Jul 21
> > > > > 01:31:09 EDT 2008 i686 i686 i386 GNU/Linux
> > > > > # prelink -mRf /sbin/udevd
> > > > > # ./a.out /proc/564/exe
> > > > > warning: /proc/564/exe: got return value 38, expected 11
> > > > > 2f7362696e2f7564657664005f47387942426e5952446e566f306868202864656c6574656429
> > > > > /sbin/udevd _G8yBBnYRDnVo0hh (deleted)
> > > > > 
> > > > > Yoshiya Koyama reports that the problem exists on RHEL 2.6.9-42.ELsmp too.
> > > > > 
> > > > > I don't think it's exactly the same problem as originally reported,
> > > > > because I definitely wasn't using prelinking (the prelink binary
> > > > > wasn't even installed on the machine until today). But finding the
> > > > > root cause of this may solve both problems.
> > > > 
> > > > switch_names() buggered in case of short names on both sides.  That should
> > > > help:
> > > > 
> > > > >From 2acda856910b774717e0290bbf948c7dee0f2e1a Mon Sep 17 00:00:00 2001
> > > > From: Al Viro <viro@zeniv.linux.org.uk>
> > > > Date: Mon, 3 Nov 2008 15:03:50 -0500
> > > > Subject: [PATCH] fix switch_names() breakage in short-to-short case
> > > > 
> > > > We want ->name.len to match the resulting name on *both*
> > > > source and target
> > > > 
> > > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> > > 
> > > please credit kmemcheck in the commit message and use an appropriate 
> > > Reported-by line as well. Thanks,
> > 
> > Did this fix ever get merged into Linus's tree?
> 
> So far no.

I queued the below for 2.6.28 inclusion and tagged for -stable
backporting.  



From: Al Viro <viro@ZenIV.linux.org.uk>

Vegard sayeth:

When I run readlink on the /proc/*/exe-file for udevd, the kernel
returns some unitialized data to userspace:

# strace -e trace=readlink readlink /proc/4762/exe
readlink("/proc/4762/exe", "/sbin/udevd", 1025) = 30

You can see it because the kernel thinks that the string is 30 bytes
long, but in fact it is only 12 (including the '\0').

If we explicitly clear the buffer before calling readlink, we can also
see that some garbage has been filled in there, after the string:

# ./readlink /proc/4762/exe
readlink(/proc/4762/exe) = 30
2f7362696e2f7564657664000000ffffffad4effffffadffffffdeffffffffffffffff202864656c657465642900000000000000000000000000000

(Output is from following simple program:)

#include <stdio.h>
#include <string.h>
#include <unistd.h>

int main(int argc, char *argv[])
{
        char buf[1024];
        int i;
        ssize_t n;

        memset(buf, 0, sizeof(buf));
        n = readlink(argv[1], buf, sizeof(buf));

        printf("readlink(%s) = %d\n", argv[1], n);

        for (i = 0; i < sizeof(buf); ++i)
                printf("%02x", buf[i]);
        printf("\n");

        return 0;
}

It was discovered by kmemcheck:

WARNING: kmemcheck: Caught 32-bit read from uninitialized memory (f6a109e4)
64000000ad4eaddeffffffffffffffff000000000200000000000000c0838ff8
 i i u u u u u u u u u u u u u u u u u u u u u u u u u u u u u u
         ^

Pid: 21511, comm: readlink Not tainted (2.6.28-rc1 #58) 945P-A
EIP: 0060:[<c04f988d>] EFLAGS: 00000296 CPU: 0
EIP is at __d_path+0x8d/0x1c0
EAX: 0000000e EBX: d7ba0fe7 ECX: 00000001 EDX: f68b0b40
ESI: f6a109e4 EDI: d7ba0fef EBP: e58c3f28 ESP: c2569c08
 DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
CR0: 8005003b CR2: f6c1d704 CR3: 31fc7000 CR4: 00000650
DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
DR6: ffff4ff0 DR7: 00000400
 [<c04fa4b0>] d_path+0xb0/0xd0
 [<c052c37c>] proc_pid_readlink+0x6c/0xc0
 [<c04eda34>] sys_readlinkat+0x94/0xa0
 [<c04eda67>] sys_readlink+0x27/0x30
 [<c0422f83>] sysenter_do_call+0x12/0x3f
 [<ffffffff>] 0xffffffff





We want ->name.len to match the resulting name on *both* source and target.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Cc: "Vegard Nossum" <vegard.nossum@gmail.com>
Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
Cc: Alexey Dobriyan <adobriyan@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
Cc: <stable@kernel.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 fs/dcache.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff -puN fs/dcache.c~vfs-fix-switch_names-breakage-in-short-to-short-case fs/dcache.c
--- a/fs/dcache.c~vfs-fix-switch_names-breakage-in-short-to-short-case
+++ a/fs/dcache.c
@@ -1621,8 +1621,11 @@ static void switch_names(struct dentry *
 			 */
 			memcpy(dentry->d_iname, target->d_name.name,
 					target->d_name.len + 1);
+			dentry->d_name.len = target->d_name.len;
+			return;
 		}
 	}
+	do_switch(dentry->d_name.len, target->d_name.len);
 }
 
 /*
@@ -1682,7 +1685,6 @@ already_unhashed:
 
 	/* Switch the names.. */
 	switch_names(dentry, target);
-	do_switch(dentry->d_name.len, target->d_name.len);
 	do_switch(dentry->d_name.hash, target->d_name.hash);
 
 	/* ... and switch the parents */
@@ -1792,7 +1794,6 @@ static void __d_materialise_dentry(struc
 	struct dentry *dparent, *aparent;
 
 	switch_names(dentry, anon);
-	do_switch(dentry->d_name.len, anon->d_name.len);
 	do_switch(dentry->d_name.hash, anon->d_name.hash);
 
 	dparent = dentry->d_parent;
_

Re: v2.6.28-rc1: readlink /proc/*/exe returns uninitialized data to userspace

[Vegard Nossum]

On Tue, Nov 11, 2008 at 11:14 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> I queued the below for 2.6.28 inclusion and tagged for -stable
> backporting.
>
>
>
> From: Al Viro <viro@ZenIV.linux.org.uk>
>
> Vegard sayeth:
>
> When I run readlink on the /proc/*/exe-file for udevd, the kernel
> returns some unitialized data to userspace:
>
> # strace -e trace=readlink readlink /proc/4762/exe
> readlink("/proc/4762/exe", "/sbin/udevd", 1025) = 30
>
> You can see it because the kernel thinks that the string is 30 bytes
> long, but in fact it is only 12 (including the '\0').
>
> If we explicitly clear the buffer before calling readlink, we can also
> see that some garbage has been filled in there, after the string:
>
> # ./readlink /proc/4762/exe
> readlink(/proc/4762/exe) = 30
> 2f7362696e2f7564657664000000ffffffad4effffffadffffffdeffffffffffffffff202864656c657465642900000000000000000000000000000
>
> (Output is from following simple program:)
>
> #include <stdio.h>
> #include <string.h>
> #include <unistd.h>
>
> int main(int argc, char *argv[])
> {
>        char buf[1024];

Should probably have been unsigned char.

>        int i;
>        ssize_t n;
>
>        memset(buf, 0, sizeof(buf));
>        n = readlink(argv[1], buf, sizeof(buf));
>
>        printf("readlink(%s) = %d\n", argv[1], n);
>
>        for (i = 0; i < sizeof(buf); ++i)
>                printf("%02x", buf[i]);

Or maybe it was the wrong format string. Negative numbers become very
long (with many extra "ff"s) in output. I guess it doesn't really
matter, though...

That said, Alexey also had an error in HIS testcase:

On Tue, Nov 4, 2008 at 11:34 AM, Alexey Dobriyan <adobriyan@gmail.com> wrote:
> #include <sys/types.h>
> #include <sys/stat.h>
> #include <fcntl.h>
> #include <stdio.h>
> #include <string.h>
> #include <unistd.h>
>
> int main(void)
> {
>        int fd, fd1;
>        char buf[64], buf1[64], img[42000];
>        ssize_t len;
>
>        fd = open("/proc/self/exe", O_RDONLY);
>
>        readlink("/proc/self/exe", buf, sizeof(buf));
>        strcpy(buf1, buf);
>        strcat(buf1, ".xxx");
>        unlink(buf1);
>        fd1 = open(buf1, O_WRONLY|O_CREAT);

Without the third argument to open with O_CREAT, file permissions may
be very strange :-)

Is this small program suitable for inclusion in LTP, maybe? We can
verify that kernel does the right thing by testing readlink(buf) ==
strlen(buf).

But thanks for the fix and attribution!


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

Re: v2.6.28-rc1: readlink /proc/*/exe returns uninitialized data to userspace

[Greg KH]

On Tue, Nov 11, 2008 at 02:14:12PM -0800, Andrew Morton wrote:
> On Sat, 8 Nov 2008 02:12:05 +0300
> Alexey Dobriyan <adobriyan@gmail.com> wrote:
> 
> > On Fri, Nov 07, 2008 at 11:05:44AM -0800, Greg KH wrote:
> > > On Thu, Nov 06, 2008 at 11:04:10AM +0100, Ingo Molnar wrote:
> > > > 
> > > > * Al Viro <viro@ZenIV.linux.org.uk> wrote:
> > > > 
> > > > > On Tue, Nov 04, 2008 at 10:39:19AM +0100, Vegard Nossum wrote:
> > > > > > On Sun, Oct 26, 2008 at 10:08 PM, Vegard Nossum <vegard.nossum@gmail.com> wrote:
> > > > > > # uname -a
> > > > > > Linux localhost.localdomain 2.6.25.11-97.fc9.i686 #1 SMP Mon Jul 21
> > > > > > 01:31:09 EDT 2008 i686 i686 i386 GNU/Linux
> > > > > > # prelink -mRf /sbin/udevd
> > > > > > # ./a.out /proc/564/exe
> > > > > > warning: /proc/564/exe: got return value 38, expected 11
> > > > > > 2f7362696e2f7564657664005f47387942426e5952446e566f306868202864656c6574656429
> > > > > > /sbin/udevd _G8yBBnYRDnVo0hh (deleted)
> > > > > > 
> > > > > > Yoshiya Koyama reports that the problem exists on RHEL 2.6.9-42.ELsmp too.
> > > > > > 
> > > > > > I don't think it's exactly the same problem as originally reported,
> > > > > > because I definitely wasn't using prelinking (the prelink binary
> > > > > > wasn't even installed on the machine until today). But finding the
> > > > > > root cause of this may solve both problems.
> > > > > 
> > > > > switch_names() buggered in case of short names on both sides.  That should
> > > > > help:
> > > > > 
> > > > > >From 2acda856910b774717e0290bbf948c7dee0f2e1a Mon Sep 17 00:00:00 2001
> > > > > From: Al Viro <viro@zeniv.linux.org.uk>
> > > > > Date: Mon, 3 Nov 2008 15:03:50 -0500
> > > > > Subject: [PATCH] fix switch_names() breakage in short-to-short case
> > > > > 
> > > > > We want ->name.len to match the resulting name on *both*
> > > > > source and target
> > > > > 
> > > > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> > > > 
> > > > please credit kmemcheck in the commit message and use an appropriate 
> > > > Reported-by line as well. Thanks,
> > > 
> > > Did this fix ever get merged into Linus's tree?
> > 
> > So far no.
> 
> I queued the below for 2.6.28 inclusion and tagged for -stable
> backporting.  


What ever happened to this patch?  Did it not make it into Linus's tree
somehow?

thanks,

greg k-h

(original patch below...)















> From: Al Viro <viro@ZenIV.linux.org.uk>
> 
> Vegard sayeth:
> 
> When I run readlink on the /proc/*/exe-file for udevd, the kernel
> returns some unitialized data to userspace:
> 
> # strace -e trace=readlink readlink /proc/4762/exe
> readlink("/proc/4762/exe", "/sbin/udevd", 1025) = 30
> 
> You can see it because the kernel thinks that the string is 30 bytes
> long, but in fact it is only 12 (including the '\0').
> 
> If we explicitly clear the buffer before calling readlink, we can also
> see that some garbage has been filled in there, after the string:
> 
> # ./readlink /proc/4762/exe
> readlink(/proc/4762/exe) = 30
> 2f7362696e2f7564657664000000ffffffad4effffffadffffffdeffffffffffffffff202864656c657465642900000000000000000000000000000
> 
> (Output is from following simple program:)
> 
> #include <stdio.h>
> #include <string.h>
> #include <unistd.h>
> 
> int main(int argc, char *argv[])
> {
>         char buf[1024];
>         int i;
>         ssize_t n;
> 
>         memset(buf, 0, sizeof(buf));
>         n = readlink(argv[1], buf, sizeof(buf));
> 
>         printf("readlink(%s) = %d\n", argv[1], n);
> 
>         for (i = 0; i < sizeof(buf); ++i)
>                 printf("%02x", buf[i]);
>         printf("\n");
> 
>         return 0;
> }
> 
> It was discovered by kmemcheck:
> 
> WARNING: kmemcheck: Caught 32-bit read from uninitialized memory (f6a109e4)
> 64000000ad4eaddeffffffffffffffff000000000200000000000000c0838ff8
>  i i u u u u u u u u u u u u u u u u u u u u u u u u u u u u u u
>          ^
> 
> Pid: 21511, comm: readlink Not tainted (2.6.28-rc1 #58) 945P-A
> EIP: 0060:[<c04f988d>] EFLAGS: 00000296 CPU: 0
> EIP is at __d_path+0x8d/0x1c0
> EAX: 0000000e EBX: d7ba0fe7 ECX: 00000001 EDX: f68b0b40
> ESI: f6a109e4 EDI: d7ba0fef EBP: e58c3f28 ESP: c2569c08
>  DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
> CR0: 8005003b CR2: f6c1d704 CR3: 31fc7000 CR4: 00000650
> DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
> DR6: ffff4ff0 DR7: 00000400
>  [<c04fa4b0>] d_path+0xb0/0xd0
>  [<c052c37c>] proc_pid_readlink+0x6c/0xc0
>  [<c04eda34>] sys_readlinkat+0x94/0xa0
>  [<c04eda67>] sys_readlink+0x27/0x30
>  [<c0422f83>] sysenter_do_call+0x12/0x3f
>  [<ffffffff>] 0xffffffff
> 
> 
> 
> 
> 
> We want ->name.len to match the resulting name on *both* source and target.
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> Cc: "Vegard Nossum" <vegard.nossum@gmail.com>
> Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
> Cc: Alexey Dobriyan <adobriyan@gmail.com>
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
> Cc: <stable@kernel.org>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> 
>  fs/dcache.c |    5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff -puN fs/dcache.c~vfs-fix-switch_names-breakage-in-short-to-short-case fs/dcache.c
> --- a/fs/dcache.c~vfs-fix-switch_names-breakage-in-short-to-short-case
> +++ a/fs/dcache.c
> @@ -1621,8 +1621,11 @@ static void switch_names(struct dentry *
>  			 */
>  			memcpy(dentry->d_iname, target->d_name.name,
>  					target->d_name.len + 1);
> +			dentry->d_name.len = target->d_name.len;
> +			return;
>  		}
>  	}
> +	do_switch(dentry->d_name.len, target->d_name.len);
>  }
>  
>  /*
> @@ -1682,7 +1685,6 @@ already_unhashed:
>  
>  	/* Switch the names.. */
>  	switch_names(dentry, target);
> -	do_switch(dentry->d_name.len, target->d_name.len);
>  	do_switch(dentry->d_name.hash, target->d_name.hash);
>  
>  	/* ... and switch the parents */
> @@ -1792,7 +1794,6 @@ static void __d_materialise_dentry(struc
>  	struct dentry *dparent, *aparent;
>  
>  	switch_names(dentry, anon);
> -	do_switch(dentry->d_name.len, anon->d_name.len);
>  	do_switch(dentry->d_name.hash, anon->d_name.hash);
>  
>  	dparent = dentry->d_parent;
> _

Re: v2.6.28-rc1: readlink /proc/*/exe returns uninitialized data to userspace

[Andrew Morton]

On Wed, 3 Dec 2008 09:18:54 -0800
Greg KH <greg@kroah.com> wrote:

> On Tue, Nov 11, 2008 at 02:14:12PM -0800, Andrew Morton wrote:
> > On Sat, 8 Nov 2008 02:12:05 +0300
> > Alexey Dobriyan <adobriyan@gmail.com> wrote:
> > 
> > > On Fri, Nov 07, 2008 at 11:05:44AM -0800, Greg KH wrote:
> > > > On Thu, Nov 06, 2008 at 11:04:10AM +0100, Ingo Molnar wrote:
> > > > > 
> > > > > * Al Viro <viro@ZenIV.linux.org.uk> wrote:
> > > > > 
> > > > > > On Tue, Nov 04, 2008 at 10:39:19AM +0100, Vegard Nossum wrote:
> > > > > > > On Sun, Oct 26, 2008 at 10:08 PM, Vegard Nossum <vegard.nossum@gmail.com> wrote:
> > > > > > > # uname -a
> > > > > > > Linux localhost.localdomain 2.6.25.11-97.fc9.i686 #1 SMP Mon Jul 21
> > > > > > > 01:31:09 EDT 2008 i686 i686 i386 GNU/Linux
> > > > > > > # prelink -mRf /sbin/udevd
> > > > > > > # ./a.out /proc/564/exe
> > > > > > > warning: /proc/564/exe: got return value 38, expected 11
> > > > > > > 2f7362696e2f7564657664005f47387942426e5952446e566f306868202864656c6574656429
> > > > > > > /sbin/udevd _G8yBBnYRDnVo0hh (deleted)
> > > > > > > 
> > > > > > > Yoshiya Koyama reports that the problem exists on RHEL 2.6.9-42.ELsmp too.
> > > > > > > 
> > > > > > > I don't think it's exactly the same problem as originally reported,
> > > > > > > because I definitely wasn't using prelinking (the prelink binary
> > > > > > > wasn't even installed on the machine until today). But finding the
> > > > > > > root cause of this may solve both problems.
> > > > > > 
> > > > > > switch_names() buggered in case of short names on both sides.  That should
> > > > > > help:
> > > > > > 
> > > > > > >From 2acda856910b774717e0290bbf948c7dee0f2e1a Mon Sep 17 00:00:00 2001
> > > > > > From: Al Viro <viro@zeniv.linux.org.uk>
> > > > > > Date: Mon, 3 Nov 2008 15:03:50 -0500
> > > > > > Subject: [PATCH] fix switch_names() breakage in short-to-short case
> > > > > > 
> > > > > > We want ->name.len to match the resulting name on *both*
> > > > > > source and target
> > > > > > 
> > > > > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> > > > > 
> > > > > please credit kmemcheck in the commit message and use an appropriate 
> > > > > Reported-by line as well. Thanks,
> > > > 
> > > > Did this fix ever get merged into Linus's tree?
> > > 
> > > So far no.
> > 
> > I queued the below for 2.6.28 inclusion and tagged for -stable
> > backporting.  
> 
> 
> What ever happened to this patch?  Did it not make it into Linus's tree
> somehow?
> 

Stuck in -mm.  Sent to Al on Dec 1, ignored.  As usual.

Al, what's going on?  Are you thinking that I'm planning on merging
these myself?  Because if you're on the To: then my intention is that the
patch be processed by yourself.

Re: v2.6.28-rc1: readlink /proc/*/exe returns uninitialized data to userspace

[Greg KH]

On Wed, Dec 03, 2008 at 12:20:48PM -0800, Andrew Morton wrote:
> On Wed, 3 Dec 2008 09:18:54 -0800
> Greg KH <greg@kroah.com> wrote:
> 
> > On Tue, Nov 11, 2008 at 02:14:12PM -0800, Andrew Morton wrote:
> > > On Sat, 8 Nov 2008 02:12:05 +0300
> > > Alexey Dobriyan <adobriyan@gmail.com> wrote:
> > > 
> > > > On Fri, Nov 07, 2008 at 11:05:44AM -0800, Greg KH wrote:
> > > > > On Thu, Nov 06, 2008 at 11:04:10AM +0100, Ingo Molnar wrote:
> > > > > > 
> > > > > > * Al Viro <viro@ZenIV.linux.org.uk> wrote:
> > > > > > 
> > > > > > > On Tue, Nov 04, 2008 at 10:39:19AM +0100, Vegard Nossum wrote:
> > > > > > > > On Sun, Oct 26, 2008 at 10:08 PM, Vegard Nossum <vegard.nossum@gmail.com> wrote:
> > > > > > > > # uname -a
> > > > > > > > Linux localhost.localdomain 2.6.25.11-97.fc9.i686 #1 SMP Mon Jul 21
> > > > > > > > 01:31:09 EDT 2008 i686 i686 i386 GNU/Linux
> > > > > > > > # prelink -mRf /sbin/udevd
> > > > > > > > # ./a.out /proc/564/exe
> > > > > > > > warning: /proc/564/exe: got return value 38, expected 11
> > > > > > > > 2f7362696e2f7564657664005f47387942426e5952446e566f306868202864656c6574656429
> > > > > > > > /sbin/udevd _G8yBBnYRDnVo0hh (deleted)
> > > > > > > > 
> > > > > > > > Yoshiya Koyama reports that the problem exists on RHEL 2.6.9-42.ELsmp too.
> > > > > > > > 
> > > > > > > > I don't think it's exactly the same problem as originally reported,
> > > > > > > > because I definitely wasn't using prelinking (the prelink binary
> > > > > > > > wasn't even installed on the machine until today). But finding the
> > > > > > > > root cause of this may solve both problems.
> > > > > > > 
> > > > > > > switch_names() buggered in case of short names on both sides.  That should
> > > > > > > help:
> > > > > > > 
> > > > > > > >From 2acda856910b774717e0290bbf948c7dee0f2e1a Mon Sep 17 00:00:00 2001
> > > > > > > From: Al Viro <viro@zeniv.linux.org.uk>
> > > > > > > Date: Mon, 3 Nov 2008 15:03:50 -0500
> > > > > > > Subject: [PATCH] fix switch_names() breakage in short-to-short case
> > > > > > > 
> > > > > > > We want ->name.len to match the resulting name on *both*
> > > > > > > source and target
> > > > > > > 
> > > > > > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> > > > > > 
> > > > > > please credit kmemcheck in the commit message and use an appropriate 
> > > > > > Reported-by line as well. Thanks,
> > > > > 
> > > > > Did this fix ever get merged into Linus's tree?
> > > > 
> > > > So far no.
> > > 
> > > I queued the below for 2.6.28 inclusion and tagged for -stable
> > > backporting.  
> > 
> > 
> > What ever happened to this patch?  Did it not make it into Linus's tree
> > somehow?
> > 
> 
> Stuck in -mm.  Sent to Al on Dec 1, ignored.  As usual.
> 
> Al, what's going on?  Are you thinking that I'm planning on merging
> these myself?  Because if you're on the To: then my intention is that the
> patch be processed by yourself.
> 

Ping?

Re: v2.6.28-rc1: readlink /proc/*/exe returns uninitialized data to userspace

[Al Viro]

On Sat, Dec 06, 2008 at 09:44:57PM -0800, Greg KH wrote:

> > Al, what's going on?  Are you thinking that I'm planning on merging
> > these myself?  Because if you're on the To: then my intention is that the
> > patch be processed by yourself.
> > 
> 
> Ping?

Still sifting through audit and VFS queues; this one (along with a bunch
of other stuff is in the latter).