Bug 211924 - lib/libc/sys/mmap_test:mmap_truncate_signal fails with SIGSEGV instead of SIGBUS
Summary: lib/libc/sys/mmap_test:mmap_truncate_signal fails with SIGSEGV instead of SIGBUS
Status: Open
Alias: None
Product: Base System
Classification: Unclassified
Component: tests (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Only Me
Assignee: freebsd-testing mailing list
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-08-17 06:38 UTC by Enji Cooper
Modified: 2019-10-06 20:37 UTC (History)
3 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Enji Cooper freebsd_committer 2016-08-17 06:38:02 UTC
NetBSD expects this testcase to fail with SIGBUS, but instead it fails with SIGSEGV on FreeBSD.

This test case is new to FreeBSD and only available on ^/projects/netbsd-tests-update-12 so far..

$ svn info /usr/src/svn/ | grep ^URL:
URL: svn+ssh://ngie@repo.freebsd.org/base/projects/netbsd-tests-update-12
$ svnversion /usr/src/svn
304238M
$ sudo kyua debug -k /usr/tests/lib/libc/sys/Kyuafile mmap_test:mmap_truncate_signal
mmap_test:mmap_truncate_signal  ->  failed: child process got SIGSEGV instead of SIGBUS
Comment 1 commit-hook freebsd_committer 2016-08-17 06:41:13 UTC
A commit references this bug:

Author: ngie
Date: Wed Aug 17 06:41:06 UTC 2016
New revision: 304257
URL: https://svnweb.freebsd.org/changeset/base/304257

Log:
  Expect :mmap_truncate_signal to fail on FreeBSD

  Additional investigation is being done as part of bug 211924

  PR:	211924
  Sponsored by:	EMC / Isilon Storage Division

Changes:
  projects/netbsd-tests-update-12/contrib/netbsd-tests/lib/libc/sys/t_mmap.c
Comment 2 Ed Maste freebsd_committer 2019-07-31 13:23:16 UTC
kib, the test here does a

map = mmap(NULL, page, PROT_READ, MAP_FILE|MAP_PRIVATE, fd, 0);

then fork() and tries to access the mapping in the child. The test expects SIGBUS but gets SIGSEGV. Perhaps the right change is to just expect SIGSEGV on FreeBSD but I wanted to check if you have an opinion on the choice of signal.
Comment 3 Konstantin Belousov freebsd_committer 2019-08-02 16:46:51 UTC
(In reply to Ed Maste from comment #2)
We return SIGSEGV/SEGV_MAPERR, which is explained by POSIX as
'Address not mapped to object', which it is.

FreeBSD used to deliver SIGBUS in this situation, but it was rightfully corrected
to SIGSEGV by davidxu AFAIR, and then I introduced compat sysctls to select
the specific signal to deliver.  See the story in PR 118304.
Comment 4 Ed Maste freebsd_committer 2019-08-02 17:36:05 UTC
Reopen for a change to the test case - we should remove the

atf_tc_expect_fail("testcase fails with SIGSEGV on FreeBSD; bug # 211924");

and check for the desired signal on FreeBSD (or allow either SIGSEGV or SIGBUS).
Comment 5 Jilles Tjoelker freebsd_committer 2019-09-04 21:36:15 UTC
I think FreeBSD is wrong here. Reading the mmap page of SUSv4tc2 XSH 3 System Interfaces:

] The system shall always zero-fill any partial page at the end of an
] object.  Further, the system shall never write out any modified portions
] of the last page of an object which are beyond its end. References
] within the address range starting at pa and continuing for len bytes to
] whole pages following the end of an object shall result in delivery of a
] SIGBUS signal.

As far as I understand, SEGV_MAPERR is for the case where no mapping exists for the address, and SEGV_ACCERR is for the case where the access conflicts with the prot parameter passed to mmap() (PROT_READ/PROT_WRITE/PROT_EXEC). The size of the mapping is equal to the len parameter passed to mmap(), whether the underlying object is that long or not.

So this does not mean that the change mentioned in 118304 is completely wrong. There is only a problem for pages of the mapped region beyond the end of the object.

By the way, the testcase is not POSIX-compliant either, since the aforementioned mmap page also says:

] If the size of the mapped file changes after the call to mmap() as a
] result of some other operation on the mapped file, the effect of
] references to portions of the mapped region that correspond to added or
] removed portions of the file is unspecified.
Comment 6 Konstantin Belousov freebsd_committer 2019-09-06 12:54:00 UTC
(In reply to Jilles Tjoelker from comment #5)
I believe we are compliant with the behavior for the partial page.  We always zero-fill invalid parts of the partially-valid pages that are mapped into userspace.

Was there a history behind the SIGBUS requirement ?  I believe that the change for SIGBUS->SIGSEGV came with r151316, and I cannot remember such explicit language at that time.

Anyway, I am certain that we do not want to change the signal definitions back, because there are programs that depend on it and which already did the switch to accommodate to the change.
Comment 7 Jilles Tjoelker freebsd_committer 2019-09-08 12:33:36 UTC
Here is a real-world example of code expecting SIGBUS: https://gitlab.freedesktop.org/xorg/xserver/blob/master/os/busfault.c

(Note that this is the same "busfault" I referred to in the file sealing review https://reviews.freebsd.org/D21391 . I think preventing truncation via sealing is generally a better option than handling signals, but it may not always be possible.)

Like the NetBSD testcase, this assumes that truncating after the mapping has been created behaves normally, as if the truncation has been done beforehand.

Sending SIGSEGV where SIGBUS is expected will expose the X server to an easy DoS by a client truncating a file or posixshm mapped by the X server. This only applies to certain modern protocol extensions since older ones use SysV shared memory segments that have a fixed size.

Accepting SIGSEGV in the X server code runs the risk of trying to "fix", for example, an incorrect write to a read-only mapping or a jump to a non-executable mapping by treating it as if the client unexpectedly truncated the object. Checking si_code does not help since FreeBSD currently sends SEGV_ACCERR in both situations.

The signal definitions should indeed not be changed back to what they were. Instead, the number of possible signal/code combinations from page faults should be increased from two to three. In sys/amd64/amd64/trap.c, this corresponds to the return value from trap_pfault(). This will also need changes to sys/vm/vm_fault.c, since vm_fault() currently returns KERN_PROTECTION_FAILURE for both accesses violating the protection such as writes to a read-only mapping (which should result in SIGSEGV with code SEGV_ACCERR) and accesses inside a valid mapping but outside the underlying object (which should result in SIGBUS with code BUS_OBJERR).
Comment 8 Konstantin Belousov freebsd_committer 2019-09-08 16:33:56 UTC
https://reviews.freebsd.org/D21566
Comment 9 commit-hook freebsd_committer 2019-09-18 23:40:10 UTC
A commit references this bug:

Author: jilles
Date: Wed Sep 18 21:00:32 UTC 2019
New revision: 352495
URL: https://svnweb.freebsd.org/changeset/base/352495

Log:
  Add some tests for page fault signals and codes

  It is useful to have some tests for page fault signals.

  More tests would be useful but creating the conditions (such as various
  kinds of running out of memory and I/O errors) is more complicated.

  The tests page_fault_signal__bus_objerr_1 and
  page_fault_signal__bus_objerr_2 depend on https://reviews.freebsd.org/D21566
  before they can pass.

  PR:		211924
  Reviewed by:	kib
  Differential Revision:	https://reviews.freebsd.org/D21624

Changes:
  head/tests/sys/vm/Makefile
  head/tests/sys/vm/page_fault_signal.c
Comment 10 commit-hook freebsd_committer 2019-09-27 18:44:05 UTC
A commit references this bug:

Author: kib
Date: Fri Sep 27 18:43:39 UTC 2019
New revision: 352807
URL: https://svnweb.freebsd.org/changeset/base/352807

Log:
  Improve MD page fault handlers.

  Centralize calculation of signal and ucode delivered on unhandled page
  fault in new function vm_fault_trap().  MD trap_pfault() now almost
  always uses the signal numbers and error codes calculated in
  consistent MI way.

  This introduces the protection fault compatibility sysctls to all
  non-x86 architectures which did not have that bug, but apparently they
  were already much more wrong in selecting delivered signals on
  protection violations.

  Change the delivered signal for accesses to mapped area after the
  backing object was truncated.  According to POSIX description for
  mmap(2):
     The system shall always zero-fill any partial page at the end of an
     object. Further, the system shall never write out any modified
     portions of the last page of an object which are beyond its
     end. References within the address range starting at pa and
     continuing for len bytes to whole pages following the end of an
     object shall result in delivery of a SIGBUS signal.

     An implementation may generate SIGBUS signals when a reference
     would cause an error in the mapped object, such as out-of-space
     condition.
  Adjust according to the description, keeping the existing
  compatibility code for SIGSEGV/SIGBUS on protection failures.

  For situations where kernel cannot handle page fault due to resource
  limit enforcement, SIGBUS with a new error code BUS_OBJERR is
  delivered.  Also, provide a new error code SEGV_PKUERR for SIGSEGV on
  amd64 due to protection key access violation.

  vm_fault_hold() is renamed to vm_fault().  Fixed some nits in
  trap_pfault()s like mis-interpreting Mach errors as errnos.  Removed
  unneeded truncations of the fault addresses reported by hardware.

  PR:	211924
  Reviewed by:	alc
  Discussed with:	jilles, markj
  Sponsored by:	The FreeBSD Foundation
  MFC after:	1 week
  Differential revision:	https://reviews.freebsd.org/D21566

Changes:
  head/sys/amd64/amd64/trap.c
  head/sys/amd64/vmm/vmm.c
  head/sys/arm/arm/trap-v4.c
  head/sys/arm/arm/trap-v6.c
  head/sys/arm64/arm64/trap.c
  head/sys/i386/i386/trap.c
  head/sys/kern/sys_process.c
  head/sys/mips/mips/trap.c
  head/sys/powerpc/powerpc/trap.c
  head/sys/riscv/riscv/trap.c
  head/sys/sparc64/sparc64/trap.c
  head/sys/sys/signal.h
  head/sys/vm/vm_extern.h
  head/sys/vm/vm_fault.c
  head/sys/vm/vm_map.c
  head/sys/vm/vm_param.h
Comment 11 commit-hook freebsd_committer 2019-09-29 15:18:24 UTC
A commit references this bug:

Author: jilles
Date: Sun Sep 29 15:17:59 UTC 2019
New revision: 352869
URL: https://svnweb.freebsd.org/changeset/base/352869

Log:
  Adjust tests after page fault changes in r352807

  Commit r352807 fixed various signal numbers and codes from page faults;
  adjust the tests so they expect the fixes to be present.

  PR:		211924

Changes:
  head/contrib/netbsd-tests/lib/libc/sys/t_mmap.c
  head/tests/sys/posixshm/posixshm_test.c
  head/tests/sys/vm/page_fault_signal.c
Comment 12 commit-hook freebsd_committer 2019-10-04 12:18:26 UTC
A commit references this bug:

Author: kib
Date: Fri Oct  4 12:18:06 UTC 2019
New revision: 353102
URL: https://svnweb.freebsd.org/changeset/base/353102

Log:
  MFC r352807:
  Improve MD page fault handlers.

  PR:	211924

Changes:
_U  stable/12/
  stable/12/sys/amd64/amd64/trap.c
  stable/12/sys/amd64/vmm/vmm.c
  stable/12/sys/arm/arm/trap-v4.c
  stable/12/sys/arm/arm/trap-v6.c
  stable/12/sys/arm64/arm64/trap.c
  stable/12/sys/i386/i386/trap.c
  stable/12/sys/kern/sys_process.c
  stable/12/sys/mips/mips/trap.c
  stable/12/sys/powerpc/powerpc/trap.c
  stable/12/sys/riscv/riscv/trap.c
  stable/12/sys/sparc64/sparc64/trap.c
  stable/12/sys/sys/signal.h
  stable/12/sys/vm/vm_extern.h
  stable/12/sys/vm/vm_fault.c
  stable/12/sys/vm/vm_map.c
  stable/12/sys/vm/vm_param.h
Comment 13 commit-hook freebsd_committer 2019-10-06 20:37:16 UTC
A commit references this bug:

Author: jilles
Date: Sun Oct  6 20:36:26 UTC 2019
New revision: 353148
URL: https://svnweb.freebsd.org/changeset/base/353148

Log:
  MFC r352495,r352869: Adjust tests for page fault changes in r353102

  PR:		211924

Changes:
_U  stable/12/
  stable/12/contrib/netbsd-tests/lib/libc/sys/t_mmap.c
  stable/12/tests/sys/posixshm/posixshm_test.c
  stable/12/tests/sys/vm/Makefile
  stable/12/tests/sys/vm/page_fault_signal.c