New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 888212 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Closed: Oct 31
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug



Sign in to add a comment

crosvm crashes due to openat() call inside libc free()

Project Member Reported by dverkamp@chromium.org, Sep 22

Issue description

I have only observed this once on a Kevin, but it seems like it could happen on any device (maybe more likely on 32-bit systems since they are under more memory pressure?).

It looks like free() will sometimes try to open /proc/sys/vm/overcommit_memory in order to decide whether to return freed heap memory to the kernel; this explodes in the sandboxed crosvm child processes, since the openat call isn't in the whitelist.

The backtrace looks like this:
(gdb) bt
#0  __libc_do_syscall () at ../sysdeps/unix/sysv/linux/arm/libc-do-syscall.S:47
#1  0xf369d862 in check_may_shrink_heap () at ../sysdeps/unix/sysv/linux/malloc-sysdep.h:45
#2  shrink_heap (diff=65536, h=0x40500000) at arena.c:519
#3  heap_trim (pad=131072, heap=0x40500000) at arena.c:608
#4  _int_free (av=<optimized out>, p=<optimized out>, have_lock=0) at malloc.c:4087

This logic seems to have been added in this glibc commit: 
 https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=9fab36eb;hp=2b4f00d1a42b705521ca205ad8285dde82d84f2b

It is also referenced in this SELinux bug report: https://bugzilla.redhat.com/show_bug.cgi?id=872729

I'm not sure if we can somehow filter on just this path in the seccomp whitelist, or at least cause openat() to return an error code rather than killing the process (the glibc code does appear to handle failure gracefully).
 
doing string compares in seccomp filters is effectively not possible today.  from a technical perspective, there are ideas bouncing around to pull it off, but it's non-trivial and not implemented today.

if you do have to whitelist openat, you could at least restrict the flags field to disallow the write/create/tmpfile bits so you know it's only ever used to open a path as read-only.
Labels: -Pri-3 Pri-1
Owner: dverkamp@chromium.org
Status: Assigned (was: Untriaged)
Cc: cywang@chromium.org wuchengli@chromium.org derat@chromium.org dverkamp@chromium.org dgreid@chromium.org smbar...@chromium.org
 Issue 899484  has been merged into this issue.
Labels: M-72
Project Member

Comment 5 by bugdroid1@chromium.org, Oct 31

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/crosvm/+/5656c124af2bb956dba19e409a269ca588c685e3

commit 5656c124af2bb956dba19e409a269ca588c685e3
Author: Daniel Verkamp <dverkamp@chromium.org>
Date: Wed Oct 31 19:42:43 2018

devices: block: fix seccomp failures from free()

It looks like free() will sometimes try to open
/proc/sys/vm/overcommit_memory in order to decide whether to return
freed heap memory to the kernel; change the seccomp filter to fail the
open syscalls with an error code (ENOENT) rather than killing the
process.

Also allow madvise to free memory for the same free() codepath.

BUG= chromium:888212 
TEST=Run fio loop test on kevin

Change-Id: I1c27b265b822771f76b7d9572d9759476770000e
Signed-off-by: Daniel Verkamp <dverkamp@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1305756
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Reviewed-by: Dylan Reid <dgreid@chromium.org>

[modify] https://crrev.com/5656c124af2bb956dba19e409a269ca588c685e3/seccomp/x86_64/block_device.policy
[modify] https://crrev.com/5656c124af2bb956dba19e409a269ca588c685e3/seccomp/arm/block_device.policy

Status: Fixed (was: Assigned)
Labels: Merge-Request-71
I would like to backport the fix above to R71.  This is a low-risk change, since it only adds a few new syscall entries to the crosvm block device seccomp policy.  It fixes a stability issue: certain patterns of disk I/O in the guest can cause the VMM to crash due to an open() call that was not allowed in the seccomp policy.
Project Member

Comment 8 by sheriffbot@chromium.org, Nov 19

Labels: -Merge-Request-71 Hotlist-Merge-Review Merge-Review-71
This bug requires manual review: M71 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Merge-Review-71 Merge-Approved-71
Approved for ChromeOS M71
Project Member

Comment 10 by bugdroid1@chromium.org, Nov 20

Labels: merge-merged-release-R71-11151.B
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/crosvm/+/19493bf90d0407a63fb5470aa687873e62475de4

commit 19493bf90d0407a63fb5470aa687873e62475de4
Author: Daniel Verkamp <dverkamp@chromium.org>
Date: Tue Nov 20 17:51:51 2018

devices: block: fix seccomp failures from free()

It looks like free() will sometimes try to open
/proc/sys/vm/overcommit_memory in order to decide whether to return
freed heap memory to the kernel; change the seccomp filter to fail the
open syscalls with an error code (ENOENT) rather than killing the
process.

Also allow madvise to free memory for the same free() codepath.

BUG= chromium:888212 
TEST=Run fio loop test on kevin

Change-Id: I1c27b265b822771f76b7d9572d9759476770000e
Signed-off-by: Daniel Verkamp <dverkamp@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1305756
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Reviewed-by: Dylan Reid <dgreid@chromium.org>
(cherry picked from commit 5656c124af2bb956dba19e409a269ca588c685e3)
Reviewed-on: https://chromium-review.googlesource.com/c/1344220

[modify] https://crrev.com/19493bf90d0407a63fb5470aa687873e62475de4/seccomp/x86_64/block_device.policy
[modify] https://crrev.com/19493bf90d0407a63fb5470aa687873e62475de4/seccomp/arm/block_device.policy

Project Member

Comment 11 by sheriffbot@chromium.org, Nov 26

Cc: geo...@google.com
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Merge-Approved-71
Cc: vovoy@chromium.org

Sign in to add a comment