(2012/03/22 4:18), Dave Anderson wrote:
>
>
> ----- Original Message -----
>>
>>
>> ----- Original Message -----
>>> Hi Dave,
>>>
>>> I met stack smashing detection by glibc at read_string()
>>> then this patch is proposal.
>>>
>>> *** stack smashing detected ***: crash terminated
>>> ======= Backtrace: =========
>>> /lib/libc.so.6(__fortify_fail+0x4c)[0xfe12380]
>>> /lib/libc.so.6(__fortify_fail+0x0)[0xfe12334]
>>> ./crash[0x10147bf0]
>>> ./crash(display_sys_stats+0xcf8)[0x1011cd74]
>>> ./crash(main_loop+0x300)[0x10068960]
>>> ./crash(current_interp_command_loop+0x48)[0x1021ac2c]
>>> ./crash[0x1021bcc4]
>>> ./crash(catch_errors+0x84)[0x1021a0c4]
>>> ./crash[0x1021d37c]
>>> ./crash(catch_errors+0x84)[0x1021a0c4]
>>> ./crash(gdb_main+0x58)[0x1021d3e8]
>>> ./crash(gdb_main_entry+0x6c)[0x1021d490]
>>> ./crash(gdb_main_loop+0x3b4)[0x10130e5c]
>>> ./crash(main+0x38c0)[0x10068650]
>>> /lib/libc.so.6(+0x1f568)[0xfd36568]
>>> /lib/libc.so.6(+0x1f728)[0xfd36728]
>>>
>>> An failed vmalloc() including non terminated with NULLCHAR is root cause,
>>> but I think it is better to keep other utilities without killed.
>>
>> This patch changes the return value of read_string() in a
>> situation where the requested number of bytes does not include
>> a NULL terminator. Note that the function is described like
>> this:
>>
>> /*
>> * Try to read a string of non-NULL characters from a memory location,
>> * returning the number of characters read.
>> */
>> int
>> read_string(ulong kvaddr, char *buf, int maxlen)
>> {
>>
>> The "maxlen" parameter is there to handle case where the requested
>> memory read does not contain a NULL character. And there may be
>> other callers that use the function to read until a NULL *or* until
>> the maxlen is reached.
>>
>> That being said, there may be a bug in there somewhere, or it
>> could be written differently, but I don't want to change the
>> function's behavior (return value).
>>
>> You mention:
>>
>>> an failed vmalloc() including non terminated with NULLCHAR
>>> is the root cause".
>>
>> Can you elaborate on what you mean by that? I want to be able
>> to reproduce this, but I cannot.
>
> Hi Toshi,
>
> I'm still not clear on how you were able to make this happen in
> the "normal" course of events, but I was able to kludge up a test
> with more than a page-size of non-NULL characters, and did manage
> to force a segmentation violation.
Hi Dave,
Thanks for your support and I'm sorry that my previous analysis was very poor.
> There's really no reason for read_string() to buffer the data given
> that it has aways zeroed out the user buffer first. And there's also
> no reason for it to break up the request into per-page segments.
Yes, you're right and I've been mistaken about root cause.
(By skipping read_string(), true root cause was also skipped...)
I added debug messages in ppc_processor_speed() and found out
a direct tie to "*** stack smashing detected ***".
It's a ppc specific problem, and fixed with attached patch.
I'm not sure about strcasecmp() implementation or specification
but if "buflen - 1" size characters are passed,
looked to be detected as stack corruption.
Thanks a lots for your support,
Toshi
> So I'm just re-writing/simplifying the read_string() function to
> zero out and then try to read maxlen bytes into the user buffer, then
> zero out everything after the first NULL that it finds, and return
> the number of consecutive non-NULL characters from the starting
> address.
>
> Thanks,
> Dave
>
> --
> Crash-utility mailing list
> Crash-utility@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/crash-utility
>
Date: Thu, 22 Mar 2012 18:13:38 +0900
Subject: [PATCH] ppc32: fix stack corruption by strcasecmp()
If read_string(str_buf) is read as "clock-frequency",
strcasecmp() looks to corrupts stack.
if (len && (strcasecmp(str_buf, "clock-frequency") == 0))
"clock-frequency" is 15 bytes + nul and str_buf[16].
I'm not sure that buflen requirement from strcasecmp()
but extend str_buf from 16 bytes to 32 bytes.
Signed-off-by: Toshikazu Nakayama <nakayama.ts@xxxxxxxxxxxxxx>
---
ppc.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/ppc.c b/ppc.c
index 3e11664..7037207 100755
--- a/ppc.c
+++ b/ppc.c
@@ -595,7 +595,7 @@ ppc_processor_speed(void)
ulong res, value, ppc_md, md_setup_res;
ulong prep_setup_res;
ulong node, type, name, properties;
- char str_buf[16];
+ char str_buf[32];
ulong len, mhz = 0;
if (machdep->mhz)
--
1.7.0.4
--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/crash-utility
