Skip to content
This repository has been archived by the owner on May 1, 2023. It is now read-only.

Systrace Snooping Supports Less Than Half the Market? #4

Closed
AttwellBrian opened this issue Mar 13, 2018 · 3 comments
Closed

Systrace Snooping Supports Less Than Half the Market? #4

AttwellBrian opened this issue Mar 13, 2018 · 3 comments

Comments

@AttwellBrian
Copy link

Systrace snooping doesn't work on/above Marshmallow. Is it impossible to setup the hooks on these OS versions? Or just not done yet?

Aside: thanks for open sourcing this :)

@BurntBrunch
Copy link
Contributor

BurntBrunch commented Mar 13, 2018 via email

@AttwellBrian
Copy link
Author

Awesome!

If you don't mind I'll leave this Issue open to track the update. Or, if you folks don't like using github Issues internally I don't mind if you close this Issue :)

ricardorey10 referenced this issue Mar 21, 2018
Summary:
Since Android N, Google changed the semantics of `dlopen()`, causing it to return a handle instead of a pointer to a `soinfo` struct. Also, using `dlopen()` on system libraries is not allowed, so we're in double trouble.

There's currently a solution for this in our linker library (parsing `/proc/self/maps`), but it's quite slow, so this diff introduces a new mechanism for installing hooks.

The new mechanism is based on `dl_iterate_phdr`. We basically call `dl_iterate_phdr` to iterate over every loaded shared object and, for each one of those shared objects, we stash:

- The relocation table for PLT entries (DT_JMPREL)
- The size of the relocation table
- The symbols table
- The strings table

With these, it is possible for us to find the address of the relocation of a symbol - in this case, of functions such as `open()`, `close()`, etc - and install our hooks.

Reviewed By: aandreyeu

Differential Revision: D6757893

fbshipit-source-id: 2e6cb644a37056a4d5d162944705dcf88c245f01
@ricardorey10
Copy link
Contributor

ce50933 fixes this.

facebook-github-bot pushed a commit that referenced this issue May 17, 2018
…d Thumb2

Summary:
linux_syscall_support.h contains syscall-entry inline `__asm__` that contains this sequence for ARM:
```
push {r7}
mov r7,%1
svc
pop {r7}
```
There are two major problems:
1) With gcc+gas, `mov` is a pseudo opcode that expands to `mov.w` for Thumb2 (e.g., armv7), and `movs` for Thumb16 (e.g., armv5te). With clang+LLVM, `mov` is a real opcode for Thumb2, but yields "error: instruction requires: thumb2" for Thumb16.
2) `movs` takes an 8-bit immediate operand constrained to the range [0..255], but ARM has syscall numbers greater-than 255.

The complete solution for numeric range and compatibility with all combos of gcc+gas vs. clang+LLVM is this sequence:
```
movs r7, %1 >> 4
lsls r7, #4
adds r7, %1 & 15
```
There is an additional challenges in making all assemblers happy in all modes, which is described in the comment for the new macro `LSS_S(opcode)`.

Reviewed By: mzlee

Differential Revision: D8023474

fbshipit-source-id: 253ad03f3c2e5e43f4f55c576bbd882b443b634e
facebook-github-bot pushed a commit that referenced this issue Apr 3, 2020
Summary:
With the `__cxa_throw` hook attached for gnustl, apps will crash if an object without a destructor is thrown:

```
try { throw 0; } catch(...) {}

// or

class SimpleException {};
try { throw SimpleException{}; } catch(...) {}
```

The issue is that the `__cxa_throw` abi has a destructor function pointer parameter, which may be null according to the documentation. In the gnustl region of the code, Lyra doesn't do a null check before invoking it at the following stack:

```
[???] 0X0 [unknown] + 0x0
+libfbjni.so  facebook::lyra::(anonymous namespace)::HijackedExceptionTypeInfo::destructor(void*) (./fbandroid/libraries/fbjni/cxx/lyra/cxa_throw.cpp:213)
libgnustl_shared.so 0X759F86716C [unknown] + 0x6116c
libgnustl_shared.so 0X759F8DD2E0 _Unwind_DeleteException + 0x18
```

I wrote a test which repros the issue, and without the fix it crashes  with a similar stack:

```
backtrace:
      #00 pc 00000000  <unknown>
      #1 pc 00022e01  /data/app/com.facebook.builds.fb4a-vhhkGO4NTAZmUTmfS5wpfw==/lib/arm/libfbjni.so (BuildId: 7a0f9db0801e4f451162c28a392cd35d4ba46b9a)
      #2 pc 0004fb2d  /data/app/com.facebook.lyra.tests-MJNqCBmU7StfOjPoAyfEgg==/lib/arm/libgnustl_shared.so!libgnustl_shared.so (offset 0x4f000) (BuildId: 059ab3ea3d764339fe3ceea2904f54637c89594e)
      #3 pc 0009b8e3  /data/app/com.facebook.lyra.tests-MJNqCBmU7StfOjPoAyfEgg==/lib/arm/libgnustl_shared.so!libgnustl_shared.so (offset 0x50000) (_Unwind_DeleteException+12) (BuildId: 059ab3ea3d764339fe3ceea2904f54637c89594e)
      #4 pc 0000aeb9  /data/app/com.facebook.lyra.tests-MJNqCBmU7StfOjPoAyfEgg==/lib/arm/liblyra-tests.so (testThatCxaThrowHookThrowsAndSetsTraceTrivialException(facebook::jni::alias_ref<_jclass*>)+220) (BuildId: 31cc0552d685f1ce9e896d1628656cd97096678d)
```

To fix, add a null check before invoking `mutable_info->orig_dest_`, which originates from the destructor parameter.

Reviewed By: smeenai

Differential Revision: D20841003

fbshipit-source-id: 907a13ebf994c5bad511b13ab9684efcbc2ce474
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants