Page MenuHomeFreeBSD

Enable PIE by default on 64-bit architectures
ClosedPublic

Authored by mw on Jan 25 2021, 9:51 AM.

Details

Summary

This patch adds Position Independent Executables (PIE)
flags for building OS. It allows to enable the ASLR
feature based only on the sysctl knobs, without
need to rebuild the image. Tests showed that
no problems with stability / performance degradation
were seen when using PIEs with ASLR disabled.

The change is limited only for 64-bit architectures.

Use bsd.opts.mk instead of the src.opts.mk in order
to satisfy MK_PIE dependency in bsd.prog.mk file.

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Yes lets get this into -CURRENT soon.

I've asked @imp to weigh in on the __T move.

We should also add a note to UPDATING, something like: On 64-bit target architectures binaries are now built as Position-Independent Executables (PIE) by default. A WITHOUT_CLEAN build will not work across the transition so you will need to run make cleanworld, or set WITH_CLEAN for the first build.

We'll also want to send a heads-up mail to -current suggesting things that users should watch for; it should be transparent for most things, but things like application launchers might misbehave, and we will want to check for ports build failures. We should also run this through an exp-run.

PR253275 submitted for exp-run

kevans added inline comments.
share/mk/bsd.opts.mk
103

I note that the placement in bsd.opts.mk means we're enabling this by default for any port using bmake and including bsd.prog.mk/bsd.lib.mk, which I think is outside the scope of what's been discussed thus far... I think that's a fine choice, but definitely requires mentioning if we're wanting to explicitly do that or not and an exp-run to go with it.

If we wanted to avoid it, it should be easy enough to do with something in local.init.mk, thinking along the lines of a WANT_DEFAULT_YES=PIE or even WANT_DEFAULT_PIE=yes and checking ${WANT_DEFAULT_PIE:Uno} here.

imp requested changes to this revision.Feb 5 2021, 5:55 PM

No. Let's not do this.

share/mk/bsd.opts.mk
102

I don't think we need to do this dance.

Either the options are broken on 32-bit archs (arm, mips, powerpc and i386) in which case we should use BROKEN+=, or we can use MACHINE_ARCH because we don't seem to test MK_PIE in Makefile.inc1...

This revision now requires changes to proceed.Feb 5 2021, 5:55 PM
share/mk/bsd.opts.mk
101

We can do this dance w/o the TARGET_ARCH stuff. My objection is to moving __T here since that's a src build concept...

I have no objection to using MACHINE_ARCH in these if.

share/mk/bsd.opts.mk
102

Oh, and I think the test should be *FOR* 32-bit archs and set them to NO for each. They are the exception, not the rule. No need to list all 64-bit archs that may expand in the future. 32-bit archs will only contract from here on out.

Adding a comment that was accidentally unsubmitted for a while

share/mk/bsd.opts.mk
102

BROKEN isn't really right IMO, there might be a bug or two but generally the option is applicable to 32-bit, just not so useful / has open performance questions.

I believe it's only tested in:

share/mk/bsd.lib.mk
share/mk/bsd.prog.mk
share/mk/src.libnames.mk
usr.bin/svn/Makefile.inc

Hi,

I have looked at *.mk files and if I am not mistaken it seems that there is currently no clean way of having a separate default option for PIE for base, as the __DEFAULT_YES_OPTIONS from bsd.opts.mk takes precedence and moving this option from bsd.opts.mk will leave us with no MK_PIE defined.

The original purpose of this patch was to enable PIE in base and to enable it for ports later, when any issues that arise will be eliminated. Since I was tasked by @mw with fixing this patch, I want to ask how to fix this. Should I try to somehow work around this in src or would it be better to leave it enabled for everything and instead create additional patch which would allow ports to opt out in a clean way? While enabling this for everything was not the original intention, I think that in the long term enabling this for both ports and base as soon as possible could be beneficial.

It's only O(20) ports using bmake that are affected, IMO the most straightforward path is going to be to fix that small number of ports. I'd guess the changes are going to fall into one or two different categories.

Oh, one issue: when MK_PIE is true we still have non-pie static libs. This is the case with e.g. net/freevrrpd

ld: error: can't create dynamic relocation R_X86_64_32 against local symbol in readonly segment; recompile object files with -fPIC or pass '-Wl,-z,notext' to allow text relocations in the output
>>> defined in /usr/lib/libnetgraph.a(sock.o)
>>> referenced by sock.c:78 (/usr/home/emaste/src/freebsd-git/laptop/lib/libnetgraph/sock.c:78)
>>>               sock.o:(NgMkSockNode) in archive /usr/lib/libnetgraph.a

in bsd.lib.mk we create a special libfoo_pie.a if INTERNALLIB but /usr/lib/lib*.a is always non-PIE.

Hi,

I have looked at *.mk files and if I am not mistaken it seems that there is currently no clean way of having a separate default option for PIE for base, as the __DEFAULT_YES_OPTIONS from bsd.opts.mk takes precedence and moving this option from bsd.opts.mk will leave us with no MK_PIE defined.

That's not what I'm suggesting at all. I'm saying you should not move the TARGET stuff.

MK_PIE Will be defined if you (a) always define it and have it marked BROKEN on 32-bit targets or (b) define it based on MACHINE_ARCH

(a) seem to be undesirable, which is fine (I didn't know if the selected enabling was done for performance or functional reasons). Doing (b) is fine.

The original purpose of this patch was to enable PIE in base and to enable it for ports later, when any issues that arise will be eliminated. Since I was tasked by @mw with fixing this patch, I want to ask how to fix this. Should I try to somehow work around this in src or would it be better to leave it enabled for everything and instead create additional patch which would allow ports to opt out in a clean way? While enabling this for everything was not the original intention, I think that in the long term enabling this for both ports and base as soon as possible could be beneficial.

The original patch would not have accomplished that goal. TARGET_ARCH is not something we can test here to differentiate between the two.

It's only O(20) ports using bmake that are affected, IMO the most straightforward path is going to be to fix that small number of ports. I'd guess the changes are going to fall into one or two different categories.

I agree. Having different defaults for src and ports is something I generally strongly oppose because it causes too much confusion unless there's a compelling reason to have the defaults be different. I think given the few ports that are affected, there's not really a compelling reason: we can do an exp run to clear this up.

share/mk/bsd.opts.mk
102

OK. If it works on 32-bit, but isn't desirable to be enabled there, then I agree BROKEN is the wrong mechanism...

103

I tend to agree. *IF* we wanted to have it be different, then we should set something like this in local.init.mk or src.init.mk (I think the latter might be better, but that's a bdrewery question as to which one is better). I'm not yet convinced it should be different, but am open to the possibility.

share/mk/bsd.opts.mk
102

During testing I've seen that enabling PIE doesn't cause any noticeable problems on 32-bit architectures by itself, however enabling ASLR caused problems with running out of memory. Since PIE is not really interesting without ASLR, it was left disabled for 32-bit platforms.

Fix/workaround for net/freevrrpd in D28595

Update the patch by:

  • using MACHINE_ARCH only
  • inverting the if logic

The next step is to go over failing ports as listed in https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=253275

I'm quite happy with this. Thanks!

This revision is now accepted and ready to land.Feb 13 2021, 7:25 AM

Thanks @imp ! Regarding the logistics, 2 questions:

  • should it land after all ports exp-run failures are fixed?
  • should we consider MFC (at least to stable/13)?

Affected ports from the exp-run have been fixed, or have patches available and are waiting on maintainer approval. So, this should be ready to commit any time, although I think we should have a comment explaining which architectures have been opted-out (32-bit) and a brief sentence why.

share/mk/bsd.opts.mk
86

We may want to have a comment here with the reasoning behind the list. ("Default to disabling PIE on 32-bit architectures, because ...")

share/mk/bsd.opts.mk
86

Agreed.

Update revision:

  • Add comment justifying disabling feature on 32-bit archs
  • Sort MACHINE_ARCH's alphabetically
This revision now requires review to proceed.Feb 23 2021, 8:51 AM
This revision is now accepted and ready to land.Feb 24 2021, 6:31 PM

The comment about why could use some help, though, I supposed.

share/mk/bsd.opts.mk
88

My suggestion is:

# Default to disabling PIE on 32-bit architectures.  The small address space
# means that ASLR is of limited effectiveness, and it may cause issues with 
# some memory-hungry workloads.

I can commit it with this.

share/mk/bsd.opts.mk
88

I'll commit with the updated comment of yours. Before this one lands, we must merge those two:
https://reviews.freebsd.org/D28370
https://reviews.freebsd.org/D28893
Otherwise MIPS and PPC64 builds will break. Can you please take a look at those patches?

Additional questions:

  1. Should we wait for ports' merge?
  2. Should we consider any MFC?

I'll commit with the updated comment of yours.

Ah, right - I was looking at a number of ports patches from dgr@semihalf and forgot this one was yours.

Should we wait for ports' merge?

We don't have a dependency on resolving every last issue; only a couple of leaf ports are affected and patches are available for all of them. We can make sure they're all resolved before MFC though.

Should we consider any MFC?

I think we should, but this is probably a separate discussion to have in a little while based on what we see after this is turned on in main.

This revision was automatically updated to reflect the committed changes.