pthread_kill is broken on ICS B2G because the sandbox rejects tkill

RESOLVED FIXED in mozilla36

Status

()

Core
Security
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jld, Assigned: jld)

Tracking

Trunk
mozilla36
All
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
On ICS B2G(/Android) and older, pthread_kill() uses tkill(tid, sig) instead of tgkill(getpid(), tid, sig); the latter is allowed by the current sandbox policy, but not the former.  It's simple enough to “polyfill” this by having the SIGSYS handler check for __NR_tkill and do the corresponding tgkill instead.

In principle this affects all earlier B2G branches with seccomp sandboxing, but I don't think this needs to be uplifted unless it's blocking something upliftable.
(Assignee)

Comment 1

3 years ago
Created attachment 8517653 [details] [diff] [review]
bug1093893-ics-tkill-hg0.diff

Passes try on top of Luke's patches from bug 1091916: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5a5d048ae730

Fails try without this patch: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=066f76915a23 (i.e., proves that applying those other patches on top of m-c and doing mochitests is sufficient to reproduce the bug)
Attachment #8517653 - Flags: review?(gdestuynder)

Comment 2

3 years ago
Thank you so much!

Comment 3

3 years ago
(Unrelated to this bug) looking at your patch, you #ifdef on "ANDROID_VERSION < 16".  I had guess (hopefully wrongly) that we compiled a single binary for Android and thus we'd have to do a dynamic query on the Android version.  Is that not the case?  Do we compile for several major Android versions and upload the lot of them to the Play store?

Updated

3 years ago
Blocks: 1091912
No longer blocks: 1091916

Comment 4

3 years ago
n/m question in comment 3; I was confusing Android version with NDK version.
(Assignee)

Comment 5

3 years ago
(In reply to Luke Wagner [:luke] from comment #3)
> (Unrelated to this bug) looking at your patch, you #ifdef on
> "ANDROID_VERSION < 16".  I had guess (hopefully wrongly) that we compiled a
> single binary for Android and thus we'd have to do a dynamic query on the
> Android version.  Is that not the case?  Do we compile for several major
> Android versions and upload the lot of them to the Play store?

That might need an a comment.   As I understand it, ANDROID_VERSION is the SDK version and thus the lowest version of Android the compiled code will run on.

Maybe more importantly, this isn't used on Firefox for Android (yet); we don't have sandboxable child processes there (yet), and we don't have seccomp-bpf support there (yet; Chrome for Android might have plans for it).  B2G has seccomp-bpf support because we convinced hardware manufacturers to take kernel patches.

B2G, as I understand it, always has ANDROID_VERSION equal to (rather than ≤) the base version for the device.
Comment on attachment 8517653 [details] [diff] [review]
bug1093893-ics-tkill-hg0.diff

Review of attachment 8517653 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/sandbox/linux/Sandbox.cpp
@@ +137,5 @@
>    args[3] = SECCOMP_PARM4(ctx);
>    args[4] = SECCOMP_PARM5(ctx);
>    args[5] = SECCOMP_PARM6(ctx);
>  
> +#if defined(ANDROID) && ANDROID_VERSION < 16

nit: would be good to have something that indicates which "human readable" version is android 16 (like "4.3") - eventually in this bug, not necessarily in the code
Attachment #8517653 - Flags: review?(gdestuynder) → review+
(Assignee)

Comment 7

3 years ago
Created attachment 8518354 [details] [diff] [review]
bug1093893-ics-tkill-hg1.diff

Updated with improved comment (API level 16 = Android JB 4.1; https://en.wikipedia.org/wiki/Android_version_history is a useful resource); carrying over r+.
Attachment #8517653 - Attachment is obsolete: true
Attachment #8518354 - Flags: review+
(Assignee)

Comment 8

3 years ago
Try runs in comment #1 should be sufficient.
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/2881d59c61f2
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2881d59c61f2
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.