Bug 239873

Summary: www/firefox and mail/thunderbird don't like the new ASLR "stackgap" feature
Product: Ports & Packages Reporter: sigsys
Component: Individual Port(s)Assignee: freebsd-gecko (Nobody) <gecko>
Status: New ---    
Severity: Affects Only Me CC: emaste, greg, kib, lantw44, matt-freebsd, mbeis, monwarez
Priority: --- Flags: bugzilla: maintainer-feedback? (gecko)
Version: Latest   
Hardware: Any   
OS: Any   
See Also: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=252629
Bug Depends on:    
Bug Blocks: 252629    

Description sigsys 2019-08-15 09:39:44 UTC
Both fail with a "too much recursion" error message during start up.

I'm guessing the feature confuses them about the depth of the stack somehow.

Tested on 12.0-STABLE r351060.

They both work fine with the stackgap sysctls set to 0.

And they both have been working fine with the other ASLR features on ever since this was committed to 12-STABLE BTW.
Comment 1 Greg V 2020-05-08 12:22:12 UTC
hm, according to https://wiki.freebsd.org/ASLR the base ntpd also doesn't like stackgap..
Comment 2 Thibault Payet 2020-08-11 17:46:57 UTC
Do we know how to make it working by using 

/usr/bin/proccontrol -m stackgap -s disable firefox

This command still get the too much recursion error.

Currently I could either disable aslr completely for firefox, or just globally not enabling stackgap and keep aslr.
Comment 3 sigsys 2020-08-12 20:15:04 UTC
(In reply to Thibault Payet from comment #2)
Same problem here.

Looks like the proccontrol stackgap toggle only affects the stack "guard page" (handled by vm_map_stack_locked() in sys/vm/vm_map.c), not the ASLR randomized stackgap.

This patch makes it affect the ASLR stackgap too and that makes firefox work with proccontrol.

diff --git a/sys/kern/imgact_elf.c b/sys/kern/imgact_elf.c
index fe71acabe0b..56623f29d4e 100644
--- a/sys/kern/imgact_elf.c
+++ b/sys/kern/imgact_elf.c
@@ -2766,6 +2766,9 @@ __elfN(stackgap)(struct image_params *imgp, uintptr_t *stack_base)
 
 	if ((imgp->map_flags & MAP_ASLR) == 0)
 		return;
+	if ((imgp->proc->p_flag2 & P2_STKGAP_DISABLE) != 0 ||
+	    (imgp->proc->p_fctl0 & NT_FREEBSD_FCTL_STKGAP_DISABLE) != 0)
+		return;
 	pct = __elfN(aslr_stack_gap);
 	if (pct == 0)
 		return;

Also if you mark firefox's binary with the new ELF feature flag to disable stackgap like so:

# elfctl -e +stackgap /usr/local/bin/firefox

Then firefox just works without needing to start with it proccontrol.
Comment 4 commit-hook freebsd_committer 2020-12-18 23:14:49 UTC
A commit references this bug:

Author: kib
Date: Fri Dec 18 23:14:41 UTC 2020
New revision: 368772
URL: https://svnweb.freebsd.org/changeset/base/368772

Log:
  Add ELF flag to disable ASLR stack gap.

  Also centralize and unify checks to enable ASLR stack gap in a new
  helper exec_stackgap().

  PR:	239873
  Sponsored by:	The FreeBSD Foundation
  MFC after:	1 week

Changes:
  head/sys/compat/freebsd32/freebsd32_misc.c
  head/sys/kern/imgact_elf.c
  head/sys/kern/kern_exec.c
  head/sys/sys/elf_common.h
  head/sys/sys/imgact.h
  head/usr.bin/elfctl/elfctl.c
Comment 5 Konstantin Belousov freebsd_committer 2020-12-18 23:17:24 UTC
(In reply to sigsys from comment #3)
These are different stack gaps.  One that is controlled by STKFGAP_DISABLE is at
the bottom of the stacks and prevent stack overflow from stomping on the
nearby mappings.

ASLR stack gap is at top, and it only makes an impression of being useful.
I just added an ELF feature control bit to disable it.

I considered adding procctl(2) but decided that it is not very useful,
ELF flag should be enough.
Comment 6 sigsys 2020-12-19 01:19:44 UTC
(In reply to Konstantin Belousov from comment #5)
Ok. Yeah... I see it's much better to disable just the ASLR stackgap separately like this (since the "bottom" one isn't the problem).

Firefox/Thunderbird work fine with this new "aslrstkgap" elfctl(1) toggle. Thanks for doing this!

Now all that would be missing is to have the ports set it automatically on the installed binaries.  Am I right to assume that setting unknown ELF flags like this should be harmless for older FreeBSD versions that don't support them?
Comment 7 Konstantin Belousov freebsd_committer 2020-12-19 01:51:20 UTC
(In reply to sigsys from comment #6)
Unknown flags are ignored.
Comment 8 sigsys 2020-12-19 04:10:54 UTC
This does it for all USE_GECKO ports (www/firefox, www/firefox-esr and mail/thunderbird).

diff --git a/Mk/bsd.commands.mk b/Mk/bsd.commands.mk
index f1a229d04948..0d38d7b321bb 100644
--- a/Mk/bsd.commands.mk
+++ b/Mk/bsd.commands.mk
@@ -36,6 +36,7 @@ DIALOG4PORTS?=		${LOCALBASE}/bin/dialog4ports
 DIFF?=			/usr/bin/diff
 DIRNAME?=		/usr/bin/dirname
 EGREP?=			/usr/bin/egrep
+ELFCTL?=		/usr/bin/elfctl
 EXPR?=			/bin/expr
 FALSE?=			false	# Shell builtin
 FILE?=			/usr/bin/file
diff --git a/Mk/bsd.gecko.mk b/Mk/bsd.gecko.mk
index b58e697c52a9..1881080a9d87 100644
--- a/Mk/bsd.gecko.mk
+++ b/Mk/bsd.gecko.mk
@@ -110,6 +110,7 @@ PLISTF?=	${WRKDIR}/plist_files
 
 MOZCONFIG?=		${WRKSRC}/.mozconfig
 MOZILLA_PLIST_DIRS?=	bin lib share/pixmaps share/applications
+MOZILLA_ELFCTLFIX_BINS?=	lib/${MOZILLA}/${MOZILLA} lib/${MOZILLA}/${MOZILLA_BIN}
 
 # Adjust -C target-cpu if -march/-mcpu is set by bsd.cpu.mk
 .if ${ARCH} == amd64 || ${ARCH} == i386
@@ -376,7 +377,14 @@ pre-configure-script:
 	@${SETENV} CC="${CC}" OPSYS="${OPSYS}" OSVERSION="${OSVERSION}" WRKDIR="${WRKDIR}" \
 		${SH} ${SCRIPTSDIR}/rust-compat11-canary.sh
 
-post-install-script: gecko-create-plist
+post-install-script: gecko-elfctlfix gecko-create-plist
+
+gecko-elfctlfix:
+# Avoids "too much recursion" errors when the ASLR "stackgap" is globally enabled.
+.for bin in ${MOZILLA_ELFCTLFIX_BINS}
+	@if test -x ${ELFCTL} && ${ELFCTL} -l | ${GREP} -q aslrstkgap; then \
+		${ELFCTL} -e +aslrstkgap ${STAGEDIR}${PREFIX}/${bin}; fi
+.endfor
 
 gecko-create-plist:
 # Create the plist
Comment 9 Ed Maste freebsd_committer 2021-01-13 19:26:28 UTC
See also https://reviews.freebsd.org/D28139 where I propose prefixing the disable flags with "no"

I've proposed an additional review to accept the flag with and without the "no" prefix for now, https://reviews.freebsd.org/D28140
Comment 10 commit-hook freebsd_committer 2021-01-14 20:12:11 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=3dfcb70b6ae9bcb9fd6a66721bebdb8c6a53c329

commit 3dfcb70b6ae9bcb9fd6a66721bebdb8c6a53c329
Author:     Ed Maste <emaste@FreeBSD.org>
AuthorDate: 2021-01-13 19:21:38 +0000
Commit:     Ed Maste <emaste@FreeBSD.org>
CommitDate: 2021-01-14 20:09:08 +0000

    elfctl: add backwards compatibility for "no" prefixes

    I am going to prefix opt-out ELF feature flag names with "no" to make
    their meaning more clear (review D28139), but there are some uses of the
    existing names already (e.g., the PR referenced below).

    For now accept the older, unprefixed name as well, and emit a warning.
    We can revert this after FreeBSD 13 branches.

    % elfctl -e +aslr foo
    elfctl: interpreting aslr as noaslr; please specify noaslr

    PR:             239873 (related)
    MFC after:      1 week
    Sponsored by:   The FreeBSD Foundation
    Differential Revision: https://reviews.freebsd.org/D28140

 usr.bin/elfctl/elfctl.c | 10 ++++++++++
 1 file changed, 10 insertions(+)
Comment 11 commit-hook freebsd_committer 2021-01-26 14:45:52 UTC
A commit in branch stable/12 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=49d3dcb041f058880486e3489ca79c9476ac7abf

commit 49d3dcb041f058880486e3489ca79c9476ac7abf
Author:     Ed Maste <emaste@FreeBSD.org>
AuthorDate: 2021-01-13 19:21:38 +0000
Commit:     Ed Maste <emaste@FreeBSD.org>
CommitDate: 2021-01-26 14:43:42 +0000

    elfctl: add backwards compatibility for "no" prefixes

    I am going to prefix opt-out ELF feature flag names with "no" to make
    their meaning more clear (review D28139), but there are some uses of the
    existing names already (e.g., the PR referenced below).

    For now accept the older, unprefixed name as well, and emit a warning.
    We can revert this after FreeBSD 13 branches.

    % elfctl -e +aslr foo
    elfctl: interpreting aslr as noaslr; please specify noaslr

    PR:             239873 (related)
    MFC after:      1 week
    Sponsored by:   The FreeBSD Foundation
    Differential Revision: https://reviews.freebsd.org/D28140

    (cherry picked from commit 3dfcb70b6ae9bcb9fd6a66721bebdb8c6a53c329)

 usr.bin/elfctl/elfctl.c | 10 ++++++++++
 1 file changed, 10 insertions(+)