seccomp-b2g support for ICS emulator / on TBPL

RESOLVED FIXED

Status

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jld, Assigned: jld)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

It looks like we're going to be living with ICS for a while, so it'd be nice if the emulator — which is the device everyone has access to, and what TBPL runs tests on — could be used to test sandboxing.

Problem: the ICS emulator is using an even older Linux kernel (2.6.29) than real ICS.

But I've managed to backport the seccomp-bpf patches (our 3.0 version, originally from 3.5), despite nontrivial changes to the BPF engine.  The result probably has local DoS (or worse) if the filter program itself is hostile, but it does seem to work, and for testing/development purposes it should be fine.

I'd like to clean up the patches a little more before asking for comments.  Also, if we push a new kernel to our platform_prebuilts_qemu-kernel repo, I think that will cause TBPL to start using it, so that can't happen until tests pass (and any developer-quality-of-life issues we think are essential are addressed).  Dependent bugs to follow....
Component: General → General
Product: Firefox → Firefox OS
Version: Trunk → unspecified
Depends on: 932104
Whiteboard: [c= p= s= u= ]
Depends on: 941375

Updated

5 years ago
Whiteboard: [c= p= s= u= ]
Depends on: 936320
Summary: seccomp-b2g support for ICS emulator → seccomp-b2g support for ICS emulator / on TBPL
Depends on: 962769
Depends on: 966547
No longer depends on: 966547
A workaround for bug 936320 landed; it's leave-open pending a better fix, but the workaround is good enough for this.
No longer depends on: 936320
Created attachment 8379976 [details] [diff] [review]
0001-Backport-of-Seccomp-bpf-mode-filter-from-Linux.patch

This patch is large, but it's mostly the same as https://github.com/gp-b2g/gp-keon-kernel/commit/ac3566aa70b1 and the parts where I had to make nontrivial changes are noted in the commit message.
Attachment #8379976 - Flags: review?(gdestuynder)
Created attachment 8379977 [details] [diff] [review]
9999-Enable-SECCOMP_FILTER-on-goldfish-Android-emulator.patch
Attachment #8379977 - Flags: review?(gdestuynder)
Comment on attachment 8379976 [details] [diff] [review]
0001-Backport-of-Seccomp-bpf-mode-filter-from-Linux.patch

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

Looks fine to me. Theres the BPF_S_ANC_SECCOMP_LD_W stuff missing but i suppose you had reasons for not including it.
I also looked at seccomp_run_filters and I assume it works fine - at least, it looks ok.

::: include/linux/filter.h
@@ +140,3 @@
>  struct sk_filter
>  {
>  	atomic_t		refcnt;

No BPF_S_ANC_SECCOMP_LD_W, ? (http://www.gossamer-threads.com/lists/linux/kernel/1504443?do=post_view_threaded#1504443)

::: kernel/seccomp.c
@@ +85,5 @@
> + * @syscall: number of the current system call
> + *
> + * Returns valid seccomp BPF response codes.
> + */
> +static u32 seccomp_run_filters(struct pt_regs *regs)

(just adding a comment here to remember that the major diff in the seccomp code is here)

::: kernel/signal.c
@@ +2145,1 @@
>  		err |= __put_user(from->si_uid, &to->si_uid);

No #define SYNCHRONOUS_MASK  I guess?

::: net/core/filter.c
@@ +40,3 @@
>  
>  /* No hurry in this branch */
>  static void *__load_pointer(struct sk_buff *skb, int k)

Again no 	#ifdef CONFIG_SECCOMP_FILTER
		case BPF_S_ANC_SECCOMP_LD_W:
		A = seccomp_bpf_load(fentry->k);
		continue;
		#endif
Attachment #8379976 - Flags: review?(gdestuynder) → review+
(In reply to Guillaume Destuynder [:kang] (use NEEDINFO!) from comment #4)
> Looks fine to me. Theres the BPF_S_ANC_SECCOMP_LD_W stuff missing but i
> suppose you had reasons for not including it.

This kernel predates [01f2f3f], which added the BPF_S_* enum values and the code that rewrites the BPF opcodes into them.  It was introduced as an optimization (so that the switch statement over that enum would compile into something more efficient than the code to decode the BPF opcodes), but once it was there, the seccomp-bpf patch added special enum values for seccomp and modified the translation to turn BPF_LD into them instead of the regular BPF_S_LD (which would read the nonexistent network packet).

This is why I removed it and added the fake_packet code instead.

[01f2f3f]: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=01f2f3f6ef4d076c0c10a8a7b42624416d56b523

> ::: kernel/signal.c
> @@ +2145,1 @@
> >  		err |= __put_user(from->si_uid, &to->si_uid);
> 
> No #define SYNCHRONOUS_MASK  I guess?

Indeed not.  That's from [a27341c], which changes which signal is delivered first if multiple signals are pending.

[a27341c]: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=a27341cd5fcb7cf2d2d4726e9f324009f7162c00
Attachment #8379977 - Attachment description: 0002-Enable-SECCOMP_FILTER-on-goldfish-Android-emulator.patch → 9999-Enable-SECCOMP_FILTER-on-goldfish-Android-emulator.patch
Created attachment 8380756 [details] [diff] [review]
0001-Backport-of-Seccomp-bpf-mode-filter-from-Linux.patch

It turns out that 01f2f3f6ef4d ("net: optimize Berkeley Packet Filter (BPF) processing"), mentioned previously, cherry-picks cleanly onto the 2.6.29 kernel for the ICS emulator.

So, this is a cleaner version of the seccomp-bpf patch, without the fake_packet stuff and with the BPF_S_ANC_SECCOMP_LD_W/seccomp_check_filter code restored; it applies on top the cherry-picked 01f2f3f6ef4d.

I've done basic tests locally.  Try: https://tbpl.mozilla.org/?tree=Try&rev=7f87582f4428
Attachment #8380756 - Flags: review?(gdestuynder)
Comment on attachment 8380756 [details] [diff] [review]
0001-Backport-of-Seccomp-bpf-mode-filter-from-Linux.patch

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

*eyes bleeding*
looks good tho :)
Attachment #8380756 - Flags: review?(gdestuynder) → review+
Attachment #8379976 - Attachment is obsolete: true
Created attachment 8380914 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/kernel_goldfish/pull/6

Pull request for source.  Carrying over r=kang.
Attachment #8380914 - Flags: review+
Created attachment 8380917 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/platform_prebuilts_qemu-kernel/pull/4

Pull request for binaries.  Carrying over r=kang.
Attachment #8380917 - Flags: review+
https://hg.mozilla.org/integration/b2g-inbound/rev/eb03401f4af6

(Also, reopening until this lands on m-c for releng builds, even though the commits in comment #10 will affect a local build of m-c immediately.)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/eb03401f4af6
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.