Details

Reviewers

kib
• ian
imp

Commits

rS355127: MFC rr354733:
rP517868: MFH: r515926 r517694
rS354733: Disable ntpd stack gap. When ASLR with STACK GAP != 0 ntpd suffers SIGSEGV.
rP517694: Disable ntpd stack gap. When ASLR with STACK GAP != 0 ntpd suffers SIGSEGV.

Summary

ntpd fails again with ASLR stack gap != 0. This patch circumvents the issue by duplicating proccontrol within ntpd until a proper solution can be developed with nwtime.org.

This has been submitted upstream and may be included in the next release of ntp.

This is a resolution for PR/241421 and PR/241960.

A corresponding ports patch will be created.

Test Plan

Currently in use locally.

Submitted upstream.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 27519

Event Timeline

cy created this revision.Nov 14 2019, 3:02 AM

Herald added a subscriber: imp. · View Herald TranscriptNov 14 2019, 3:02 AM

Harbormaster completed remote builds in B27507: Diff 64299.Nov 14 2019, 3:02 AM

Seems sane to me, but I'd feel better about getting it committed if others more deeply familiar with the aslr stuff give it their blessing.

This revision is now accepted and ready to land.Nov 14 2019, 3:20 AM

If you want to disable ASLR, it is enough to issue procctl(), checking global system settings gives no value and is in fact racy.

If the problem is only with the gap, then you can just disable the gap using procctl, I believe this is what jdk does.

That said, why the gap is the problem ? Wasn't the rlimit(2) call that created problems, removed from ntpd ?

contrib/ntp/ntpd/ntpd.c
427	And if __FreeBSD_cc_version is undefined, this code is ending with the error.

The rlimit() for mlock() was removed. The rlimit() for stack wasn't. Removing the second rlimit() causes an an assertion callback when an IPv6 packet is received. Your suggestion for simply calling procctl() works as well.

Unconditionally call procctl() as suggested by kib@.

This revision now requires review to proceed.Nov 14 2019, 9:55 PM

Harbormaster completed remote builds in B27519: Diff 64341.Nov 14 2019, 9:55 PM

kib added inline comments.Nov 14 2019, 10:25 PM

contrib/ntp/ntpd/ntpd.c
141	Why not `#ifdef __FreeBSD` ?
415	I suggest to disable stack gap instead of ASLR. Use PROC_STACKGAP_CTL with DISABLE when available. If not available, it is safe to assume that stackgap is not supported as well.

• ian added inline comments.Nov 14 2019, 10:33 PM

contrib/ntp/ntpd/ntpd.c
409	Is there a PR for this problem? I think this whole block needs a comment, something simple like /* Must disable STACKGAP on FreeBSD to avoid <whatever the problem was>. See PR NNNN. */

A better solution.

Harbormaster completed remote builds in B27523: Diff 64348.Nov 15 2019, 12:47 AM

cy added inline comments.Nov 15 2019, 12:53 AM

contrib/ntp/ntpd/ntpd.c
141	FreeBSD is not defined by llvm or gcc. FreeBSD_cc_version is defined by our cc, without including any headers. It's portable and can be shared with our ntp upline, nwtime.org.

imp added inline comments.Nov 15 2019, 4:21 AM

contrib/ntp/ntpd/ntpd.c
141	Ummm, FreeBSD is defined to be the major version of FreeBSD without needing a header. FreeBSD_version needs sys/param.h....

imp added inline comments.Nov 15 2019, 4:33 AM

contrib/ntp/ntpd/ntpd.c
141	FreeBSD is defined, I should have said

s/__FreeBSD_cc_version/__FreeBSD__/

Harbormaster completed remote builds in B27527: Diff 64360.Nov 15 2019, 5:40 AM

kib added inline comments.Nov 15 2019, 8:46 AM

contrib/ntp/ntpd/ntpd.c
421	I suggest to silently ignore the error from procctl(2). The only real situation where it can occur is when PROC_STACKGAP_CTL is not supported, because the kernel we running on is too old. Then there is no harm from the error, and running the daemon instead of abruptly terminating is much better outcome. I might even go further with this suggestion and propose to do e.g. #ifdef __FreeBSD__ #include <sys/procctl.h> #ifndef PROC_STACKGAP_CTL #define PROC_STACKGAP_CTL 17 #define PROC_STACKGAP_DISABLE 0x0002 #endif ... So that even if we compile on the older system, we still can run on newer kernels. That said, we probably need to allocate a bit for stack gap in image flags. Unfortunately, the image flag editor is still not installed.