Closed Bug 909658 Opened 7 years ago Closed 7 years ago

B2G Emulator-x86: build failure due to __NR_recv, __NR_msgget, __NR_semget undeclared when compiling seccomp-bfp sandboxing


(Firefox OS Graveyard :: General, defect)

Gonk (Firefox OS)
Not set


(Not tracked)



(Reporter: vicamo, Assigned: vicamo)




(3 files, 2 obsolete files)

Attached file compiler-errors.txt
No description provided.
Attached file Sandbox.e
From AOSP bionic documents[1]:

> Bionic intentionally does not provide support for System-V IPCs mechanisms,
> like the ones provided by semget(), shmget(), msgget(). The reason for this
> is to avoid denial-of-service.

However, it does include __NR_foo definitions on ARM and SH[2].

semget/msgget are only implemented internally with __NR_ipc in bug 859779.

__NR_recv was deprecated but still defined in both ARM and SH[2].  Never used in i386.

[2]: ,
Assignee: nobody → vyang
Attached patch patch (obsolete) — Splinter Review
Attachment #795886 - Flags: review?(gdestuynder)
Comment on attachment 795886 [details] [diff] [review]

Review of attachment 795886 [details] [diff] [review]:

Since I believe this patch currently will build but not run properly with a seccomp enabled kernel on i386, I set review-. However, with the fix (or any similar fix that allows __NR_ipc) and perhaps a test run I believe this should be fine.

Thanks for catching this, btw (and sorry for the review delay, was in PTO)

::: security/sandbox/seccomp_filter.h
@@ +25,5 @@
> +/**
> + * Bug 909658: no __NR_recv, __NR_msgget, __NR_semget in emulator-x86.
> + *
> + * Bionic explicitly excludes some SYSV-ipc to avoid DoS.  These NRs
> + * are even not defined on i386 platform.

The systemcalls are defined (when missing, too) in gecko/security/sandbox/{arm,x86_32,x86_64}_linux_syscalls.h

x86_32 (i.e. i386) does not define those calls (as you pointed out), hence the issue.

I would rather just say - for clarity:
* Bug 909658: no __NR_recv, __NR_msgget, __NR_semget in emulator-x86.
* These system calls are not defined on i386.

@@ +47,5 @@
> +  ALLOW_SYSCALL(semget),
> +#else
> +#define SECCOMP_WHITELIST_ADD_semget
> +#endif
> +

I am wondering if it wouldn't be clearer to use a single define switch such as:
#if defined(__i386__)

Also, note the ALLOW_SYSCALL(ipc) (this one is only defined for i386), which I believe is necessary for the content processes to work properly (after the build, try running apps otherwise, it probably gets killed by seccomp without it).
Attachment #795886 - Flags: review?(gdestuynder) → review-
Attached patch patch : v2 (obsolete) — Splinter Review
Don't have a seccomp enabled kernel on i386 either, request for feedback first.  Or, do you have a way to test it? Enable seccomp on B2G emulator-x86?
Attachment #795886 - Attachment is obsolete: true
Attachment #800663 - Flags: feedback?(gdestuynder)
Comment on attachment 800663 [details] [diff] [review]
patch : v2

Review of attachment 800663 [details] [diff] [review]:

Yes - that's probably the best way
Attachment #800663 - Flags: feedback?(gdestuynder) → feedback+
Attachment #800663 - Flags: review?(gdestuynder)
Comment on attachment 800663 [details] [diff] [review]
patch : v2

Review of attachment 800663 [details] [diff] [review]:

::: security/sandbox/seccomp_filter.h
@@ +36,5 @@
> +  ALLOW_SYSCALL(msgget), \
> +  ALLOW_SYSCALL(msgget),
> +#endif
> +

while the overall logic is fine, the ipc calls have to be at the top of the list for performance (its the most used call, so when its at the top it shortcuts the checks more quickly, when a call is matched in the filter we exit the loop)
sorry, somehow didn't catch it this morning
it's fine to just have SECCOMP_WHITELIST_ADD_i386 at the very top of the whitelist
Comment on attachment 800663 [details] [diff] [review]
patch : v2

Going to Oslo.  Will update the patch and re-request for your review.
Attachment #800663 - Flags: review?(gdestuynder)
Attached patch patch : v3Splinter Review
Address comment 7.
Attachment #800663 - Attachment is obsolete: true
Attachment #801573 - Flags: review?(gdestuynder)
Attachment #801573 - Flags: review?(gdestuynder) → review+
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.