Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1669043003:
[Android] Fix Microdump generation when Seccomp-BPF is enabled. (Closed)
|
Created:
4 years, 10 months ago by Robert Sesek Modified:
4 years, 10 months ago Reviewers:
mdempsky CC:
chromium-reviews, darin-cc_chromium.org, jam, rickyz+watch_chromium.org, jln+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Android] Fix Microdump generation when Seccomp-BPF is enabled.
BUG=584518, 439573
Originally Committed: https://crrev.com/4fe32a5a3c3c5db910517f70f45ea03ee1c676ed
Reverted: https://crrev.com/d8166bd9a4c900c9a8079c9e7b4d3e750b497f51
Committed: https://crrev.com/91272670b07051ea6aa323c14598977c9a82942a
Cr-Commit-Position: refs/heads/master@{#374043}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Restrict socket() args #
Total comments: 5
Patch Set 3 : Address comments #Patch Set 4 : Fix i386 #
Messages
Total messages: 38 (19 generated)
Description was changed from ========== [Android] Fix Microdump generation when Seccomp-BPF is enabled. BUG= ========== to ========== [Android] Fix Microdump generation when Seccomp-BPF is enabled. BUG=584518,439573 ==========
rsesek@chromium.org changed reviewers: + mdempsky@chromium.org
The CQ bit was checked by rsesek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1669043003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1669043003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1669043003/diff/1/content/common/sandbox_linu... File content/common/sandbox_linux/android/sandbox_bpf_base_policy_android.cc (right): https://codereview.chromium.org/1669043003/diff/1/content/common/sandbox_linu... content/common/sandbox_linux/android/sandbox_bpf_base_policy_android.cc:64: case __NR_socket: These seem scary. Is there anything in place to prevent renderers from making other arbitrary socket/connect calls? Can we at least restrict socket() to only create domain==AF_LOCAL/AF_UNIX sockets?
https://codereview.chromium.org/1669043003/diff/1/content/common/sandbox_linu... File content/common/sandbox_linux/android/sandbox_bpf_base_policy_android.cc (right): https://codereview.chromium.org/1669043003/diff/1/content/common/sandbox_linu... content/common/sandbox_linux/android/sandbox_bpf_base_policy_android.cc:64: case __NR_socket: On 2016/02/05 18:31:46, mdempsky wrote: > These seem scary. Is there anything in place to prevent renderers from making > other arbitrary socket/connect calls? Can we at least restrict socket() to only > create domain==AF_LOCAL/AF_UNIX sockets? Yeah, I wasn't wild about having to add these, but it's necessary on Android. I restricted it to PF_LOCAL/PF_UNIX and SOCK_DGRAM/SOCK_STREAM for now. SELinux also provides some defense-in-depth here against opening network sockets from isolated_app.
The CQ bit was checked by rsesek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1669043003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1669043003/20001
lgtm https://codereview.chromium.org/1669043003/diff/20001/content/common/sandbox_... File content/common/sandbox_linux/android/sandbox_bpf_base_policy_android.cc (right): https://codereview.chromium.org/1669043003/diff/20001/content/common/sandbox_... content/common/sandbox_linux/android/sandbox_bpf_base_policy_android.cc:88: return If(AllOf(AnyOf(domain == PF_LOCAL, domain == PF_UNIX), Nit: Unless Android is goofy, I would just check for "domain == AF_UNIX". AF_UNIX is the POSIX standardized name; the PF_* and AF_* names are equivalent on Linux; and AF_UNIX and AF_LOCAL are synonyms too. Also, maybe check that "protocol == 0". https://codereview.chromium.org/1669043003/diff/20001/content/common/sandbox_... content/common/sandbox_linux/android/sandbox_bpf_base_policy_android.cc:89: AnyOf((type & SOCK_TYPE_MASK) == SOCK_DGRAM, Any opinions on writing this instead as: const int kSockFlags = SOCK_CLOEXEC | SOCK_NONBLOCK; ... (type & ~kSockFlags) == SOCK_DGRAM || (type & ~kSockFlags) == SOCK_NONBLOCK ?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1669043003/diff/20001/content/common/sandbox_... File content/common/sandbox_linux/android/sandbox_bpf_base_policy_android.cc (right): https://codereview.chromium.org/1669043003/diff/20001/content/common/sandbox_... content/common/sandbox_linux/android/sandbox_bpf_base_policy_android.cc:88: return If(AllOf(AnyOf(domain == PF_LOCAL, domain == PF_UNIX), On 2016/02/05 20:14:02, mdempsky wrote: > Nit: Unless Android is goofy, I would just check for "domain == AF_UNIX". > AF_UNIX is the POSIX standardized name; the PF_* and AF_* names are equivalent > on Linux; and AF_UNIX and AF_LOCAL are synonyms too. > > Also, maybe check that "protocol == 0". Android isn't goofy, IMO PF is more semantically correct for socket(), AF is for sockaddrs. But just limited to AF_UNIX for readability. Also check protocol==0. https://codereview.chromium.org/1669043003/diff/20001/content/common/sandbox_... content/common/sandbox_linux/android/sandbox_bpf_base_policy_android.cc:89: AnyOf((type & SOCK_TYPE_MASK) == SOCK_DGRAM, On 2016/02/05 20:14:02, mdempsky wrote: > Any opinions on writing this instead as: > > const int kSockFlags = SOCK_CLOEXEC | SOCK_NONBLOCK; > ... > (type & ~kSockFlags) == SOCK_DGRAM || (type & ~kSockFlags) == SOCK_NONBLOCK > > ? I like that. Prevent extra bits being set.
The CQ bit was checked by rsesek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1669043003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1669043003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rsesek@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mdempsky@chromium.org Link to the patchset: https://codereview.chromium.org/1669043003/#ps40001 (title: "Address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1669043003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1669043003/40001
https://codereview.chromium.org/1669043003/diff/20001/content/common/sandbox_... File content/common/sandbox_linux/android/sandbox_bpf_base_policy_android.cc (right): https://codereview.chromium.org/1669043003/diff/20001/content/common/sandbox_... content/common/sandbox_linux/android/sandbox_bpf_base_policy_android.cc:88: return If(AllOf(AnyOf(domain == PF_LOCAL, domain == PF_UNIX), On 2016/02/05 21:27:06, Robert Sesek wrote: > Android isn't goofy, IMO PF is more semantically correct for socket(), AF is for > sockaddrs. Agreed that historically PF was more semantically correct here, but POSIX decided to only standardize the AF_* names.
Message was sent while issue was closed.
Description was changed from ========== [Android] Fix Microdump generation when Seccomp-BPF is enabled. BUG=584518,439573 ========== to ========== [Android] Fix Microdump generation when Seccomp-BPF is enabled. BUG=584518,439573 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [Android] Fix Microdump generation when Seccomp-BPF is enabled. BUG=584518,439573 ========== to ========== [Android] Fix Microdump generation when Seccomp-BPF is enabled. BUG=584518,439573 Committed: https://crrev.com/4fe32a5a3c3c5db910517f70f45ea03ee1c676ed Cr-Commit-Position: refs/heads/master@{#373934} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/4fe32a5a3c3c5db910517f70f45ea03ee1c676ed Cr-Commit-Position: refs/heads/master@{#373934}
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/1671333003/ by sgurun@chromium.org. The reason for reverting is: broke android x86 builder. Bug: 584857.
Message was sent while issue was closed.
Description was changed from ========== [Android] Fix Microdump generation when Seccomp-BPF is enabled. BUG=584518,439573 Committed: https://crrev.com/4fe32a5a3c3c5db910517f70f45ea03ee1c676ed Cr-Commit-Position: refs/heads/master@{#373934} ========== to ========== [Android] Fix Microdump generation when Seccomp-BPF is enabled. BUG=584518,439573 Originally Committed: https://crrev.com/4fe32a5a3c3c5db910517f70f45ea03ee1c676ed Reverted: https://crrev.com/d8166bd9a4c900c9a8079c9e7b4d3e750b497f51 ==========
Description was changed from ========== [Android] Fix Microdump generation when Seccomp-BPF is enabled. BUG=584518,439573 Originally Committed: https://crrev.com/4fe32a5a3c3c5db910517f70f45ea03ee1c676ed Reverted: https://crrev.com/d8166bd9a4c900c9a8079c9e7b4d3e750b497f51 ========== to ========== [Android] Fix Microdump generation when Seccomp-BPF is enabled. BUG=584518,439573 Originally Committed: https://crrev.com/4fe32a5a3c3c5db910517f70f45ea03ee1c676ed Reverted: https://crrev.com/d8166bd9a4c900c9a8079c9e7b4d3e750b497f51 ==========
Description was changed from ========== [Android] Fix Microdump generation when Seccomp-BPF is enabled. BUG=584518,439573 Originally Committed: https://crrev.com/4fe32a5a3c3c5db910517f70f45ea03ee1c676ed Reverted: https://crrev.com/d8166bd9a4c900c9a8079c9e7b4d3e750b497f51 ========== to ========== [Android] Fix Microdump generation when Seccomp-BPF is enabled. BUG=584518,439573 Originally Committed: https://crrev.com/4fe32a5a3c3c5db910517f70f45ea03ee1c676ed Reverted: https://crrev.com/d8166bd9a4c900c9a8079c9e7b4d3e750b497f51 ==========
The CQ bit was checked by rsesek@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mdempsky@chromium.org Link to the patchset: https://codereview.chromium.org/1669043003/#ps60001 (title: "Fix i386")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1669043003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1669043003/60001
Message was sent while issue was closed.
Description was changed from ========== [Android] Fix Microdump generation when Seccomp-BPF is enabled. BUG=584518,439573 Originally Committed: https://crrev.com/4fe32a5a3c3c5db910517f70f45ea03ee1c676ed Reverted: https://crrev.com/d8166bd9a4c900c9a8079c9e7b4d3e750b497f51 ========== to ========== [Android] Fix Microdump generation when Seccomp-BPF is enabled. BUG=584518,439573 Originally Committed: https://crrev.com/4fe32a5a3c3c5db910517f70f45ea03ee1c676ed Reverted: https://crrev.com/d8166bd9a4c900c9a8079c9e7b4d3e750b497f51 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [Android] Fix Microdump generation when Seccomp-BPF is enabled. BUG=584518,439573 Originally Committed: https://crrev.com/4fe32a5a3c3c5db910517f70f45ea03ee1c676ed Reverted: https://crrev.com/d8166bd9a4c900c9a8079c9e7b4d3e750b497f51 ========== to ========== [Android] Fix Microdump generation when Seccomp-BPF is enabled. BUG=584518,439573 Originally Committed: https://crrev.com/4fe32a5a3c3c5db910517f70f45ea03ee1c676ed Reverted: https://crrev.com/d8166bd9a4c900c9a8079c9e7b4d3e750b497f51 Committed: https://crrev.com/91272670b07051ea6aa323c14598977c9a82942a Cr-Commit-Position: refs/heads/master@{#374043} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/91272670b07051ea6aa323c14598977c9a82942a Cr-Commit-Position: refs/heads/master@{#374043} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
