brk checks in PR_SET_MM code

brk checks in PR_SET_MM code

[Keno Fischer]

Hi all,

The code in prctl(PR_SET_MM, ...) performs a number of sanity checks,
among them

```
/*
 * @brk should be after @end_data in traditional maps.
 */
if (prctl_map->start_brk <= prctl_map->end_data ||
    prctl_map->brk <= prctl_map->end_data)
goto out;
```

The original commit that introduces this check
(f606b77f1a9e362451aca8f81d8f36a3a112139e) says:

```
4) As in regular Elf loading procedure we require that @start_brk and
   @brk be greater than @end_data.
```

However, it does not appear that this invariant is actually
enforced during regular ELF loading. In particular, at least on my
linux distribution, it does not appear to be satisfied when
invoking the dynamic linker directly.
For example, consider the following test application:

```
#include <sys/prctl.h>
#include <unistd.h>
#include <assert.h>

int main(void) {
    int err = prctl(PR_SET_MM, PR_SET_MM_BRK, sbrk(0), 0, 0);
    assert(err == 0);
    return 0;
}
```
```
$ su
# ./a.out
# /lib64/ld-linux-x86-64.so.2 ./a.out
a.out: test.c:7: main: Assertion `err == 0' failed.
Aborted
```

I don't understand this code well enough to know what the
intended behavior is, but unfortunately this causes some
processes to be non-restorable using the PR_SET_MM
mechanism, which defeats the whole purpose of that API.
Could somebody clarify whether this situation is indeed
supposed to be impossible and if not whether said checks
in PR_SET_MM are actually supposed to be there?
I suppose this is also technically a regression when the
old PR_SET_MM commands were refactored to use this
new validation. Previously only the commands that changed
the brk validated this invariant, but these days it tries
to validate the entire structure at once, so all the PR_SET_MM
calls will fail in a process whose layout violates the sanity
check.

Thanks,
Keno

Re: brk checks in PR_SET_MM code

[Cyrill Gorcunov]

On Wed, Dec 16, 2020 at 08:29:30PM -0500, Keno Fischer wrote:
> Hi all,
> 
> The code in prctl(PR_SET_MM, ...) performs a number of sanity checks,
> among them
> 
> ```
> /*
>  * @brk should be after @end_data in traditional maps.
>  */
> if (prctl_map->start_brk <= prctl_map->end_data ||
>     prctl_map->brk <= prctl_map->end_data)
> goto out;
> ```
> 

Thanks for pointing, Keno! I don't remember the details right now,
gimme some time and once I refresh my memory I'll reply with
details.

Re: brk checks in PR_SET_MM code

[Cyrill Gorcunov]

On Thu, Dec 17, 2020 at 10:42:02AM +0300, Cyrill Gorcunov wrote:
> On Wed, Dec 16, 2020 at 08:29:30PM -0500, Keno Fischer wrote:
> > Hi all,
> > 
> > The code in prctl(PR_SET_MM, ...) performs a number of sanity checks,
> > among them
> > 
> > ```
> > /*
> >  * @brk should be after @end_data in traditional maps.
> >  */
> > if (prctl_map->start_brk <= prctl_map->end_data ||
> >     prctl_map->brk <= prctl_map->end_data)
> > goto out;
> > ```
> > 
> 
> Thanks for pointing, Keno! I don't remember the details right now,
> gimme some time and once I refresh my memory I'll reply with
> details.

Indeed, when loaded via ld directly we've got a different layout:

# /lib64/ld-linux-x86-64.so.2 ~/t

start_code	7fc25b0a4000
end_code	7fc25b0c4524
start_stack	7fffcc6b2400
start_data	7fc25b0ce4c0
end_data	7fc25b0cff98
start_brk	55555710c000
sbrk(0)		55555710c000

Note though that as far as I understand the layout is provided by
ld loader. I contrast the regular load

# ~/t

start_code	401000
end_code	401a15
start_stack	7ffce4577dd0
start_data	403e10
end_data	40408c
start_brk	b5b000
sbrk(0)		b5b000

I fear we've not been using ld's loaded programs in c/r procedure much
that's why it has not been noted earlier. Need to think how to fix it.
Using the whole memory map for verification procedure is a correct way
thus the commit you mention is doing exactly what it should but we need
to figure out how to deal with fdpic loaded files... I'll back once I
figure it out (hopefully more-less soon). Thanks a huge for report!