Re: uprobes misses breakpoint insertion into VM_WRITE mappings
- On Mar 16, 2018, at 12:52 PM, Oleg Nesterov o...@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
Re: uprobes misses breakpoint insertion into VM_WRITE mappings
- On Mar 16, 2018, at 12:52 PM, Oleg Nesterov o...@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
Re: uprobes misses breakpoint insertion into VM_WRITE mappings
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
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.
uprobes misses breakpoint insertion into VM_WRITE mappings
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
uprobes misses breakpoint insertion into VM_WRITE mappings
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
