Fallback library loading when SELinux blocks EXEC permission.

Fallback library loading when SELinux blocks EXEC permission.

It this case copying out into a file does not work, so we have to copy it into memory. This is the initial approach which is simplistic in that you get a copy per process (however, that is still better than not working at all).

BUG=390618
Committed: https://crrev.com/b210e2d62956359840d5c8ea555c3a435cd767f3
Cr-Commit-Position: refs/heads/master@{#302297}

[Anton, 2014-10-29 16:43:18]

anton@chromium.org changed reviewers:
+ petrcermak@chromium.org, simonb@chromium.org

[petrcermak, 2014-10-29 17:22:13]

https://codereview.chromium.org/692593002/diff/1/third_party/android_crazy_li...
File third_party/android_crazy_linker/README.chromium (right):

https://codereview.chromium.org/692593002/diff/1/third_party/android_crazy_li...
third_party/android_crazy_linker/README.chromium:53: - Fallback library loading
when SELinux blocks EXEC permission.
nit - "Add fallback library ..."

https://codereview.chromium.org/692593002/diff/1/third_party/android_crazy_li...
File third_party/android_crazy_linker/src/src/crazy_linker_elf_loader.cpp
(right):

https://codereview.chromium.org/692593002/diff/1/third_party/android_crazy_li...
third_party/android_crazy_linker/src/src/crazy_linker_elf_loader.cpp:315: int
prot_flags = PFLAGS_TO_PROT(phdr->p_flags);
I would move this line above line 309 and use prot_flags in the fd_.Map call.

[Anton, 2014-10-29 17:47:28]

https://codereview.chromium.org/692593002/diff/1/third_party/android_crazy_li...
File third_party/android_crazy_linker/README.chromium (right):

https://codereview.chromium.org/692593002/diff/1/third_party/android_crazy_li...
third_party/android_crazy_linker/README.chromium:53: - Fallback library loading
when SELinux blocks EXEC permission.
On 2014/10/29 17:22:12, petrcermak wrote:
> nit - "Add fallback library ..."

Done.

https://codereview.chromium.org/692593002/diff/1/third_party/android_crazy_li...
File third_party/android_crazy_linker/src/src/crazy_linker_elf_loader.cpp
(right):

https://codereview.chromium.org/692593002/diff/1/third_party/android_crazy_li...
third_party/android_crazy_linker/src/src/crazy_linker_elf_loader.cpp:315: int
prot_flags = PFLAGS_TO_PROT(phdr->p_flags);
On 2014/10/29 17:22:12, petrcermak wrote:
> I would move this line above line 309 and use prot_flags in the fd_.Map call.

Done.

[petrcermak, 2014-10-29 17:52:47]

Thanks, LGTM (my first one :-)).

[simonb (inactive), 2014-10-31 14:55:09]

https://codereview.chromium.org/692593002/diff/40001/third_party/android_craz...
File third_party/android_crazy_linker/src/src/crazy_linker_elf_loader.cpp
(right):

https://codereview.chromium.org/692593002/diff/40001/third_party/android_craz...
third_party/android_crazy_linker/src/src/crazy_linker_elf_loader.cpp:309: int
prot_flags = PFLAGS_TO_PROT(phdr->p_flags);
Could be const.

https://codereview.chromium.org/692593002/diff/40001/third_party/android_craz...
third_party/android_crazy_linker/src/src/crazy_linker_elf_loader.cpp:316: if
(errno != EACCES || (prot_flags & PROT_EXEC) != PROT_EXEC) {
Any reason for avoiding "... || !(prot_flags & PROT_EXEC)) {" ?

https://codereview.chromium.org/692593002/diff/40001/third_party/android_craz...
third_party/android_crazy_linker/src/src/crazy_linker_elf_loader.cpp:327: // Add
PROT_WRITE to the pages.
Worth adding commentary here that even though the Map() above failed, these
pages are in fact already mapped and available, because of the previous
ReserveAddressSpace() call.  Otherwise it looks odd mprotect()ing pages after
Map() failed.

https://codereview.chromium.org/692593002/diff/40001/third_party/android_craz...
third_party/android_crazy_linker/src/src/crazy_linker_elf_loader.cpp:337: void*
lib_addr = fd_.Map((void*)NULL,
Not sure you need the cast here.

https://codereview.chromium.org/692593002/diff/40001/third_party/android_craz...
third_party/android_crazy_linker/src/src/crazy_linker_elf_loader.cpp:359: if
((prot_flags & PROT_WRITE) != PROT_WRITE) {
"if (!(prot_flags & PROT_WRITE)) {" ?

[Anton, 2014-10-31 15:18:59]

https://codereview.chromium.org/692593002/diff/40001/third_party/android_craz...
File third_party/android_crazy_linker/src/src/crazy_linker_elf_loader.cpp
(right):

https://codereview.chromium.org/692593002/diff/40001/third_party/android_craz...
third_party/android_crazy_linker/src/src/crazy_linker_elf_loader.cpp:309: int
prot_flags = PFLAGS_TO_PROT(phdr->p_flags);
On 2014/10/31 14:55:09, simonb wrote:
> Could be const.

Done.

https://codereview.chromium.org/692593002/diff/40001/third_party/android_craz...
third_party/android_crazy_linker/src/src/crazy_linker_elf_loader.cpp:316: if
(errno != EACCES || (prot_flags & PROT_EXEC) != PROT_EXEC) {
On 2014/10/31 14:55:09, simonb wrote:
> Any reason for avoiding "... || !(prot_flags & PROT_EXEC)) {" ?

Done. I was thinking there was some rule about not using boolean logic on
integers, but I couldn't find it so I have changed it.

https://codereview.chromium.org/692593002/diff/40001/third_party/android_craz...
third_party/android_crazy_linker/src/src/crazy_linker_elf_loader.cpp:327: // Add
PROT_WRITE to the pages.
On 2014/10/31 14:55:09, simonb wrote:
> Worth adding commentary here that even though the Map() above failed, these
> pages are in fact already mapped and available, because of the previous
> ReserveAddressSpace() call.  Otherwise it looks odd mprotect()ing pages after
> Map() failed.

Done.

https://codereview.chromium.org/692593002/diff/40001/third_party/android_craz...
third_party/android_crazy_linker/src/src/crazy_linker_elf_loader.cpp:337: void*
lib_addr = fd_.Map((void*)NULL,
On 2014/10/31 14:55:09, simonb wrote:
> Not sure you need the cast here.

Done.

https://codereview.chromium.org/692593002/diff/40001/third_party/android_craz...
third_party/android_crazy_linker/src/src/crazy_linker_elf_loader.cpp:359: if
((prot_flags & PROT_WRITE) != PROT_WRITE) {
On 2014/10/31 14:55:09, simonb wrote:
> "if (!(prot_flags & PROT_WRITE)) {" ?

Done.

[simonb (inactive), 2014-10-31 16:53:16]

lgtm

[Anton, 2014-10-31 16:53:52]

The CQ bit was checked by anton@chromium.org

[commit-bot: I haz the power, 2014-10-31 16:54:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/692593002/60001

[commit-bot: I haz the power, 2014-10-31 18:58:52]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-10-31 19:00:10]

Patchset 4 (id:??) landed as
https://crrev.com/b210e2d62956359840d5c8ea555c3a435cd767f3
Cr-Commit-Position: refs/heads/master@{#302297}

[rmcilroy, 2014-11-02 14:08:54]

rmcilroy@chromium.org changed reviewers:
+ rmcilroy@chromium.org

[rmcilroy, 2014-11-02 14:08:54]

We can't do this without knowing how often this is happening in the wild. Please
add UMA statistics for this.

[Anton, 2014-11-03 14:40:17]

On 2014/11/02 14:08:54, rmcilroy wrote:
> We can't do this without knowing how often this is happening in the wild.
Please
> add UMA statistics for this.

UMA was already added in:
https://codereview.chromium.org/643803003/

Are you looking for more than that?

[rmcilroy, 2014-11-03 18:15:04]

On 2014/11/03 14:40:17, Anton wrote:
> On 2014/11/02 14:08:54, rmcilroy wrote:
> > We can't do this without knowing how often this is happening in the wild.
> Please
> > add UMA statistics for this.
> 
> UMA was already added in:
> https://codereview.chromium.org/643803003/
> 
> Are you looking for more than that?

Yes we need more than this.  The UMA that Petr added checks whether we can map
with execute permission on all devices (including those which don't load from
APK).  We also want UMA stats for how often we invoke this fallback on builds
which do load from the APK (e.g., 64bit or L) to ensure that it never happens
when we don't expect it to happen.  I chatted to Petr and he is going to look
into adding this support.