Re: [Toybox] memcpy overlap in ps

[Rob Landley]

On 10/07/2016 03:38 PM, Evgenii Stepanov wrote:
> Looks like this happens when /proc/$PID/cmdline is empty, which is
> true for "kthreadd" on android.
> Numbered strings below are ptb->str + ptb->offset[i].
> For some reason ptb->slot[SLOT_argv0len] == 8 when cmdline is empty,
> but I don't see where it's coming from.
> And ptb->str + ptb->offset[4] + ptb->slot[SLOT_argv0len] overlaps with
> buf; and there are no "/" characters in cmdline to limit the memcpy
> range.
> Looks like a bug?

Yes it does.

Sigh, I was too clever and omitted a test because the result should be
overwritten later, but there's a case where the result _isn't_
overwritten later, and thus... Actually I can just move the assignment
one curly bracket out and that should fix it.

However, there's another bug, which is manifesting in "CMDLINE" showing
the arguments but not the command name... Ah, I see. "ps -w" isn't
disabling the "truncate this field to fit its assigned width" logic.
(It's making the screen width 9, but only the last field gets
arbitrarily expanded. I tested with "ps -awO cmdline" and that inserts
the new field in the middle, which means even with -w it's getting
truncated to the field width of 27.)

Hmmm... we've already got overflow logic for numeric fields (which can
extend into later fields and then it reclaims extra spaces when it can
to adjust things back to line up later), I'll just have -w trigger that
for all fields.

Ok, I've checked both in, did that fix it for you?

Rob
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

Re: [Toybox] memcpy overlap in ps

[Rob Landley]

On 10/06/2016 04:15 PM, Evgenii Stepanov wrote:
> Hi Rob,
> 
> thanks for the explanation. This is definitely not a false positive -
> the report even contains dst and src ranges for the memcpy() call, and
> they indeed overlap. Should be possible to reproduce w/o ASan by
> checking the addresses in the code.

Right before the memcpy/memmove I added:

printf("buf=%p s=%p len=%ld\n", buf, s, (long)len);
if (s+len >= buf) printf("BANANA!\n");

And did

  ./ps -aO NAME

And I don't see an instance of overlap?

(I shouldn't have to test the other way because buf should always be > len?)

> I've added some debug printfs to this code, and got this:

I'm guessing on line 753?

> j = 5
> ptb 0x0 tb 0x5564dfb750 buf 0x5564dfb961

This doesn't have s and buf, it has tb and buf. Still, it lets us know
you're looking at the non-thread case during the PS_NAME copy. Line 758
reassigns ptb so you'd have to have your printf before that.

Then we do a slightly funky basename():

  i = ptb->slot[SLOT_argv0len];
  s = ptb->str+ptb->offset[4];
  while (-1!=(k = stridx(s, '/')) && kslot[SLOT_argv0len] winding up bigger than
strlen(ptb->str+ptb->offset[4]) somehow? I'm not seeing it in my tests
here, is it a 32-bit thing? Bionic? Android kernel?)


> USER   PID  PPID VSZRSS WCHAN  PC S NAME
> root 1 0   10628   1272 SyS_epoll_ 4d805c S init
> j = 5
> ptb 0x0 tb 0x5564dfb750 buf 0x5564dfb94b

Basically the same data as above. We're not copying from buf to tb,
we're copying len bytes from s to buf. This doesn't tell me what the bad
memcpy's inputs are.

> The change (the one that requires a google login) simply replaces
> memcpy with memmove, which sounds like a bad idea at this point.

Well... maybe? I still don't know what's going on.

Rob
___
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net

Re: [Toybox] memcpy overlap in ps

[Evgenii Stepanov]

Hi Rob,

thanks for the explanation. This is definitely not a false positive -
the report even contains dst and src ranges for the memcpy() call, and
they indeed overlap. Should be possible to reproduce w/o ASan by
checking the addresses in the code.

I've added some debug printfs to this code, and got this:

j = 5
ptb 0x0 tb 0x5564dfb750 buf 0x5564dfb961
USER   PID  PPID VSZRSS WCHAN  PC S NAME
root 1 0   10628   1272 SyS_epoll_ 4d805c S init
j = 5
ptb 0x0 tb 0x5564dfb750 buf 0x5564dfb94b

The change (the one that requires a google login) simply replaces
memcpy with memmove, which sounds like a bad idea at this point.


On Wed, Oct 5, 2016 at 3:15 AM, Rob Landley  wrote:
> On 10/03/2016 07:22 PM, enh wrote:
>> from the AOSP gerrit (fixing internal bug 30074257). i've been meaning
>> to look at this more closely for a couple of months, but haven't found
>> the time. i too wasn't sure whether switching to memmove was actually
>> the fix or just papering over a real problem...
>
> Since it took me a little to work through it too, and you're "not sure",
> here's a refresher on the logic. (Feel free to skip down to the ===
> where I start talking about the code directly setting up the memcpy this
> patch would change.)
>
> The code this patch touches (line numbers are circa commit 703c49e0cb97)
> initializes the miscellaneous string slots, which store data we didn't
> get out of /proc/$PID/stat but came from somewhere else under /proc/$PID.
>
> This code lives in the function get_ps(). The miscellanous string
> processing depends on an array of structures called fetch[] at the start
> of get_ps() (line 601) listing various files under the current
> /proc/$PID, each with a set of field bits that tell us when we need this
> data (so we don't open and read from files our -o BLAH,BLAH,BLAH list
> isn't displaying). Down around line 728 we iterate through the fetch[]
> array, opening and reading each file and doing the additional processing
> it requires. Each of these files produces variable length string data,
> which we append to tb->str and store an array of tb->offset[] values to
> where each one starts. (Well, after the first; the first is at offset
> zero so we don't need to store it.)
>
> All this code lives in function get_ps(), which reads all the /proc data
> for a given process into "toybuf" in one gulp, and then at the end of
> the function (line 850) either displays it immediately (if we haven't
> got -k) or copies into into a malloced buffer attached tree we later
> turn into an array and sort before displaying it. Either way, we filter
> out the nodes we don't want (line 693) and return about halfway through
> get_ps() if it's not a process we eventually want to display.
> This means that all the data we read about each process, including the
> miscelaneous string slots, has to collectively fit in 4096 bytes.
>
> The variable name "tb" is short for "toybuf" bcause it's the structure I
> was was overlaying on toybuf to store the data in, and at the top of the
> function (line 610) it's pointed to toybuf (and on line 621 toybuf is
> memset back to all zeroes), so that's what all its fields are scribbling
> over as we read in data: "struct carveup *tb" is how we carve up toybuf
> into individual fields.
>
> Before we read the miscelaneous string data, lines 621 through 726 read
> a bunch of other stuff into toybuf first (starting with all the numeric
> data from the file "stat" going into the long long tb->slot[SLOT_count]
> array, which is a TAGGED_ARRAY as described in the checkin comment of
> commit f96bb3d8e7ec). Then we read other stuff out of the files
> "status", "io", "statm", "exe", and android's get_sched_policy().
>
> By the time we start iterating through that fetch[] array to initialize
> the miscelanous string slots, we've used maybe 1/4 of toybuf's 4096
> bytes, and a lot of these strings can be arbitrarily long (on modern
> kernels argv[] data can be gigabytes), so we give each one a size budget
> (see line 740). The theory is each field we HAVEN'T read yet has a
> minimum reserved size (256 bytes), but any space that wasn't used by
> fields we've already read gets added to the current field's budget, so
> if one of them needs to be 2k it can without totally squeezing out the
> others. (This gives us more space for string matching or displaying in
> -w mode.)
>
> (Note: there's a design tension here: processes can go away at any time,
> they can exit _while_ we're reading them, so we snapshot the data for
> each process all at once (and if we fail halfway through we discard the
> process; if it had gone away a fraction of a second earlier we wouldn't
> have seen it at all). The only AFTER we've read everything do we sort
> and/or display it. We never go back and read more data from a process
> after get_ps() returns, to avoid race conditions. But we don't want
> unlimited memory usage (imagine a process with 1000 threads and 

Re: [Toybox] memcpy overlap in ps

[Rob Landley]

On 10/03/2016 07:22 PM, enh wrote:
> from the AOSP gerrit (fixing internal bug 30074257). i've been meaning
> to look at this more closely for a couple of months, but haven't found
> the time. i too wasn't sure whether switching to memmove was actually
> the fix or just papering over a real problem...

Since it took me a little to work through it too, and you're "not sure",
here's a refresher on the logic. (Feel free to skip down to the ===
where I start talking about the code directly setting up the memcpy this
patch would change.)

The code this patch touches (line numbers are circa commit 703c49e0cb97)
initializes the miscellaneous string slots, which store data we didn't
get out of /proc/$PID/stat but came from somewhere else under /proc/$PID.

This code lives in the function get_ps(). The miscellanous string
processing depends on an array of structures called fetch[] at the start
of get_ps() (line 601) listing various files under the current
/proc/$PID, each with a set of field bits that tell us when we need this
data (so we don't open and read from files our -o BLAH,BLAH,BLAH list
isn't displaying). Down around line 728 we iterate through the fetch[]
array, opening and reading each file and doing the additional processing
it requires. Each of these files produces variable length string data,
which we append to tb->str and store an array of tb->offset[] values to
where each one starts. (Well, after the first; the first is at offset
zero so we don't need to store it.)

All this code lives in function get_ps(), which reads all the /proc data
for a given process into "toybuf" in one gulp, and then at the end of
the function (line 850) either displays it immediately (if we haven't
got -k) or copies into into a malloced buffer attached tree we later
turn into an array and sort before displaying it. Either way, we filter
out the nodes we don't want (line 693) and return about halfway through
get_ps() if it's not a process we eventually want to display.
This means that all the data we read about each process, including the
miscelaneous string slots, has to collectively fit in 4096 bytes.

The variable name "tb" is short for "toybuf" bcause it's the structure I
was was overlaying on toybuf to store the data in, and at the top of the
function (line 610) it's pointed to toybuf (and on line 621 toybuf is
memset back to all zeroes), so that's what all its fields are scribbling
over as we read in data: "struct carveup *tb" is how we carve up toybuf
into individual fields.

Before we read the miscelaneous string data, lines 621 through 726 read
a bunch of other stuff into toybuf first (starting with all the numeric
data from the file "stat" going into the long long tb->slot[SLOT_count]
array, which is a TAGGED_ARRAY as described in the checkin comment of
commit f96bb3d8e7ec). Then we read other stuff out of the files
"status", "io", "statm", "exe", and android's get_sched_policy().

By the time we start iterating through that fetch[] array to initialize
the miscelanous string slots, we've used maybe 1/4 of toybuf's 4096
bytes, and a lot of these strings can be arbitrarily long (on modern
kernels argv[] data can be gigabytes), so we give each one a size budget
(see line 740). The theory is each field we HAVEN'T read yet has a
minimum reserved size (256 bytes), but any space that wasn't used by
fields we've already read gets added to the current field's budget, so
if one of them needs to be 2k it can without totally squeezing out the
others. (This gives us more space for string matching or displaying in
-w mode.)

(Note: there's a design tension here: processes can go away at any time,
they can exit _while_ we're reading them, so we snapshot the data for
each process all at once (and if we fail halfway through we discard the
process; if it had gone away a fraction of a second earlier we wouldn't
have seen it at all). The only AFTER we've read everything do we sort
and/or display it. We never go back and read more data from a process
after get_ps() returns, to avoid race conditions. But we don't want
unlimited memory usage (imagine a process with 1000 threads and a
megabyte-long command line), hence the 4k cap per process on data we
store. Alas, this means if your /proc/self/exe symlink is 3000 bytes
long, it's trucated to the space budget we had for reading that
miscellaneous string slot.)

The "buf" pointer advances through toybuf as we use it, and it always
points to the next unused byte in toybuf. So on line 742 we set "len" to
how much space this miscellaneous string field is allowed to use, and
use it as temporary scratch space to write the path to the filename we
want to look at into buf. ("fd" is an open filehandle to /proc so we can
openat() or readlinkat() paths relative to that. The upcoming read or
readlink command will overwrite the filename data with the data it fetches.)

Then we special case the fetch[] entries were we DON'T just open a file
and read all its contents, but instead do something like