[Crash-utility] [PATCH] avoid read_string() for no terminated buf.

[Toshikazu Nakayama]

(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 at redhat.com
> https://www.redhat.com/mailman/listinfo/crash-utility
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-ppc32-fix-stack-corruption-by-strcasecmp.patch
Type: text/x-patch
Size: 873 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/crash-utility/attachments/20120322/9fa8976e/attachment.bin>