avoid read_string() for no terminated buf.
 

----- 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.

Thanks,
Dave






--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility

avoid read_string() for no terminated buf.
 

----- 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.

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.

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

avoid read_string() for no terminated buf.
 

----- Original Message -----
> (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

Queued for crash-6.0.5.

Thanks,
Dave

--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility