uprobes misses breakpoint insertion into VM_WRITE mappings

uprobes misses breakpoint insertion into VM_WRITE mappings

[Mathieu Desnoyers]

Hi,

Erica has been working on extending test-cases for uprobes, and found
something unexpected:

Since commit e40cfce626a5 "uprobes: Restrict valid_vma(false) to skip VM_SHARED vmas"
uprobes does not insert breakpoints into mappings mprotect'd as writeable.

This issue can be reproduced by compiling a library without PIC (not using GOT),
and then concurrently:

A) Load the library (dynamic loader mprotect the code as writeable to do
   the relocations, and then mprotect as executable),

B) Enable a uprobe through perf.

(it is a race window between the two mprotect syscalls)

It appears that the following restriction in valid_vma() is responsible
for this behavior:

        if (is_register)
                flags |= VM_WRITE;

I don't figure a clear explanation for this flag based on the function
comment nor the commit changelog. Any idea on whether this is really
needed ?

Note that on uprobes unregister, it allows removing a breakpoint event
on a writeable mapping, so there is clearly a discrepancy between the
level of paranoia associated with registration and unregistration.

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: uprobes misses breakpoint insertion into VM_WRITE mappings

[Oleg Nesterov]

On 03/15, Mathieu Desnoyers wrote:
>
> Hi,
>
> Erica has been working on extending test-cases for uprobes, and found
> something unexpected:
>
> Since commit e40cfce626a5 "uprobes: Restrict valid_vma(false) to skip VM_SHARED vmas"
> uprobes does not insert breakpoints into mappings mprotect'd as writeable.

Not really, VM_WRITE was illegal from the very beginning, this commit only
affects the "is_register == false" case.

> This issue can be reproduced by compiling a library without PIC (not using GOT),
> and then concurrently:
>
> A) Load the library (dynamic loader mprotect the code as writeable to do
>    the relocations, and then mprotect as executable),
>
> B) Enable a uprobe through perf.
>
> (it is a race window between the two mprotect syscalls)
>
> It appears that the following restriction in valid_vma() is responsible
> for this behavior:
>
>         if (is_register)
>                 flags |= VM_WRITE;
>
> I don't figure a clear explanation for this flag based on the function
> comment nor the commit changelog. Any idea on whether this is really
> needed ?

Because we do not want to modify the writable area. If nothing else, this
can break the application which writes to the page we are going to replace.

> Note that on uprobes unregister, it allows removing a breakpoint event
> on a writeable mapping,

Yes. Because a probed apllication can do mprotect() after the kernel installs
the breakpoint. And we have to remove this breakpoint in any case, even if
this is unsafe too.

Oleg.

Re: uprobes misses breakpoint insertion into VM_WRITE mappings

[Mathieu Desnoyers]

----- On Mar 16, 2018, at 12:52 PM, Oleg Nesterov oleg@redhat.com wrote:

> On 03/15, Mathieu Desnoyers wrote:
>>
>> Hi,
>>
>> Erica has been working on extending test-cases for uprobes, and found
>> something unexpected:
>>
>> Since commit e40cfce626a5 "uprobes: Restrict valid_vma(false) to skip VM_SHARED
>> vmas"
>> uprobes does not insert breakpoints into mappings mprotect'd as writeable.
> 
> Not really, VM_WRITE was illegal from the very beginning, this commit only
> affects the "is_register == false" case.

Good point. I only noticed it after further archeology research through git blame. ;)

> 
>> This issue can be reproduced by compiling a library without PIC (not using GOT),
>> and then concurrently:
>>
>> A) Load the library (dynamic loader mprotect the code as writeable to do
>>    the relocations, and then mprotect as executable),
>>
>> B) Enable a uprobe through perf.
>>
>> (it is a race window between the two mprotect syscalls)
>>
>> It appears that the following restriction in valid_vma() is responsible
>> for this behavior:
>>
>>         if (is_register)
>>                 flags |= VM_WRITE;
>>
>> I don't figure a clear explanation for this flag based on the function
>> comment nor the commit changelog. Any idea on whether this is really
>> needed ?
> 
> Because we do not want to modify the writable area. If nothing else, this
> can break the application which writes to the page we are going to replace.

I fully agree on never poking writeable pages. However, I think something is
missing here.

The plausible scenario that's inherently racy is:

1) dynamic loader mprotect(write) the mapping,
2) dynamic loader performs relocations
3) (concurrently) uprobe is enabled for this mapping
4) dynamic loader mprotect(read|exec) the mapping.

Again, I fully agree that step (3) should *not* touch the mapping while it is
writeable, because there is no way to synchronize the the application which is
expecting to be sole owner of the code being modified.

However, what I think is missing here is that step (4) (mprotect(read|exec))
should apply all "pending" modifications for that mapping.

This means that if some code is always writeable, uprobes will not
mess with it. However, if the pattern is to make it writeable for a short
while and then make it read|exec again, then the queued modifications
could be applied by mprotect().

What is not entirely clear to me is how to bound the number of pending
modifications and where to these pending uprobes operation queues should
reside.

Thoughts ?

Thanks,

Mathieu


> 
>> Note that on uprobes unregister, it allows removing a breakpoint event
>> on a writeable mapping,
> 
> Yes. Because a probed apllication can do mprotect() after the kernel installs
> the breakpoint. And we have to remove this breakpoint in any case, even if
> this is unsafe too.
> 
> Oleg.

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com