arm64: Enable BTI for the executable as well as the interpreter

Currently the ELF code only attempts to parse properties on the image that will start execution, either the interpreter or for statically linked executables the main executable. The expectation is that any property handling for the main executable will be done by the interpreter. This is a bit inconsistent since we do map the executable and is causing problems for the arm64 BTI support when used in conjunction with systemd's use of seccomp to implement MemoryDenyWriteExecute which stops the dynamic linker adjusting the permissions of executable segments.

Allow architectures to handle properties for both the dynamic linker and main executable, adjusting arch_parse_elf_properties() to have a new flag is_interp flag as with arch_elf_adjust_prot() and calling it for both the main executable and any intepreter.

The user of this code, arm64, is adapted to ensure that there is no functional change.

Signed-off-by Mark Brown <[email protected]>

Deployments of BTI on arm64 have run into issues interacting with systemd's MemoryDenyWriteExecute feature. Currently for dynamically linked executables the kernel will only handle architecture specific properties like BTI for the interpreter, the expectation is that the interpreter will then handle any properties on the main executable. For BTI this means remapping the executable segments PROT_EXEC | PROT_BTI.

[...]

Quoted 8 lines · Mark Brown

Deployments of BTI on arm64 have run into issues interacting with systemd's MemoryDenyWriteExecute feature. Currently for dynamically linked executables the kernel will only handle architecture specific properties like BTI for the interpreter, the expectation is that the interpreter will then handle any properties on the main executable. For BTI this means remapping the executable segments PROT_EXEC | PROT_BTI.

Quoted 1 line

[...]

Applied to for-next/execve, thanks!

[1/2] elf: Allow architectures to parse properties on the main executable

      https://git.kernel.org/kees/c/b2f2553c8e89
[2/2] arm64: Enable BTI for main executable as well as the interpreter
      https://git.kernel.org/kees/c/b65c760600e2

Quoted 9 lines · Kees Cook

Quoted 8 lines · Mark Brown

Deployments of BTI on arm64 have run into issues interacting with systemd's MemoryDenyWriteExecute feature. Currently for dynamically linked executables the kernel will only handle architecture specific properties like BTI for the interpreter, the expectation is that the interpreter will then handle any properties on the main executable. For BTI this means remapping the executable segments PROT_EXEC | PROT_BTI.

Quoted 8 lines

Quoted 1 line

[...]

Applied to for-next/execve, thanks!

[1/2] elf: Allow architectures to parse properties on the main executable

      https://git.kernel.org/kees/c/b2f2553c8e89
[2/2] arm64: Enable BTI for main executable as well as the interpreter
      https://git.kernel.org/kees/c/b65c760600e2

Kees, please can you drop this series while Catalin's alternative solution is under discussion (his Reviewed-by preceded the other patches)?

https://lore.kernel.org/r/[email protected]

Both series expose new behaviours to userspace and we don't need both.

Quoted 2 lines · Will Deacon

Kees, please can you drop this series while Catalin's alternative solution is under discussion (his Reviewed-by preceded the other patches)?

Quoted 3 lines

https://lore.kernel.org/r/[email protected]

Both series expose new behaviours to userspace and we don't need both.

I agree. Even though the patches have my reviewed-by, I think we should postpone them until we figure out a better W^X solution that does not affect BTI (and if we can't, we revisit these patches).

Arguably, the two approaches are complementary but the way this series turned out is for the BTI on main executable to be default off. I have a worry that the feature won't get used, so we just carry unnecessary code in the kernel. Jeremy also found this approach less than ideal:

https://lore.kernel.org/r/[email protected]

Quoted 5 lines · Catalin Marinas

Quoted 2 lines · Will Deacon

Kees, please can you drop this series while Catalin's alternative solution is under discussion (his Reviewed-by preceded the other patches)?

Quoted 7 lines

Quoted 3 lines

https://lore.kernel.org/r/[email protected]

Both series expose new behaviours to userspace and we don't need both.

I agree. Even though the patches have my reviewed-by, I think we should postpone them until we figure out a better W^X solution that does not affect BTI (and if we can't, we revisit these patches).

Indeed. I had been expecting this to follow the pattern of the previous nine months or so and be mostly ignored for the time being while Catalin's new series goes forward. Now that it's applied it might be worth keeping the first patch still in case someone else needs it but the second patch can probably wait.

Quoted 4 lines

Arguably, the two approaches are complementary but the way this series turned out is for the BTI on main executable to be default off. I have a worry that the feature won't get used, so we just carry unnecessary code in the kernel. Jeremy also found this approach less than ideal:

Quoted 1 line

https://lore.kernel.org/r/[email protected]

I'm not sure there was a fundamental concern with the approach there but rather some pushback on the instance on turning it off by default.

Quoted 25 lines · Mark Brown

Quoted 5 lines · Catalin Marinas

Quoted 2 lines · Will Deacon

Kees, please can you drop this series while Catalin's alternative solution is under discussion (his Reviewed-by preceded the other patches)?

Quoted 7 lines

Quoted 3 lines

https://lore.kernel.org/r/[email protected]

Both series expose new behaviours to userspace and we don't need both.

I agree. Even though the patches have my reviewed-by, I think we should postpone them until we figure out a better W^X solution that does not affect BTI (and if we can't, we revisit these patches).

Indeed. I had been expecting this to follow the pattern of the previous nine months or so and be mostly ignored for the time being while Catalin's new series goes forward. Now that it's applied it might be worth keeping the first patch still in case someone else needs it but the second patch can probably wait.

Quoted 4 lines

Arguably, the two approaches are complementary but the way this series turned out is for the BTI on main executable to be default off. I have a worry that the feature won't get used, so we just carry unnecessary code in the kernel. Jeremy also found this approach less than ideal:

Quoted 4 lines

Quoted 1 line

https://lore.kernel.org/r/[email protected]

I'm not sure there was a fundamental concern with the approach there but rather some pushback on the instance on turning it off by default.

Right, this one seems to have the smallest impact on systemd as it exists today. I would have expected the default to be on, because IMHO this set corrects what at first glance just looks like a small oversight. I find the ABI questions a bit theoretical, given that this should only affect environments that don't exist outside of labs/development orgs at this point (aka systemd services on HW that implements BTI).

The other approach works, and if the systemd folks are on board with it also should solve the underlying problem, but it creates a bit of a compatibility problem with existing containers/etc that might exist today (although running systemd/services in a container is itself a discussion).

So, frankly, I don't see why they aren't complementary. This fixes a bug we have today, the other set creates a generic mechanism for the future.

Quoted 8 lines · Mark Brown

Quoted 7 lines · Catalin Marinas

Quoted 6 lines · Will Deacon

Kees, please can you drop this series while Catalin's alternative solution is under discussion (his Reviewed-by preceded the other patches)?

https://lore.kernel.org/r/[email protected]

Both series expose new behaviours to userspace and we don't need both.

Quoted 5 lines

Quoted 4 lines

Arguably, the two approaches are complementary but the way this series turned out is for the BTI on main executable to be default off. I have a worry that the feature won't get used, so we just carry unnecessary code in the kernel. Jeremy also found this approach less than ideal:

Quoted 4 lines

Quoted 1 line

https://lore.kernel.org/r/[email protected]

I'm not sure there was a fundamental concern with the approach there but rather some pushback on the instance on turning it off by default.

Right, this one seems to have the smallest impact on systemd as it exists today.

I would have expected the default to be on, because IMHO this set corrects what at first glance just looks like a small oversight.

I find the ABI questions a bit theoretical, given that this should only affect environments that don't exist outside of labs/development orgs at this point (aka systemd services on HW that implements BTI).

The other approach works, and if the systemd folks are on board with it also should solve the underlying problem, but it creates a bit of a compatibility problem with existing containers/etc that might exist today (although running systemd/services in a container is itself a discussion).

So, frankly, I don't see why they aren't complementary.

Quoted 9 lines · Jeremy Linton

Quoted 8 lines · Mark Brown

Quoted 7 lines · Catalin Marinas

Quoted 6 lines · Will Deacon

Kees, please can you drop this series while Catalin's alternative solution is under discussion (his Reviewed-by preceded the other patches)?

https://lore.kernel.org/r/[email protected]

Both series expose new behaviours to userspace and we don't need both.

[...]

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

Arguably, the two approaches are complementary but the way this series turned out is for the BTI on main executable to be default off. I have a worry that the feature won't get used, so we just carry unnecessary code in the kernel. Jeremy also found this approach less than ideal:

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

https://lore.kernel.org/r/[email protected]

I'm not sure there was a fundamental concern with the approach there but rather some pushback on the instance on turning it off by default.

Right, this one seems to have the smallest impact on systemd as it exists today.

It had a bigger impact on glibc which had to rework the dynamic library mapping to use munmap/mmap() instead of an mprotect() (though that's already done). I think glibc still prefers the mprotect() approach for dynamic libraries.

Quoted 2 lines

I would have expected the default to be on, because IMHO this set corrects what at first glance just looks like a small oversight.

This was a design decision at the time, maybe not the best but it gives us some flexibility (and we haven't thought of MDWE).

Quoted 3 lines

I find the ABI questions a bit theoretical, given that this should only affect environments that don't exist outside of labs/development orgs at this point (aka systemd services on HW that implements BTI).

The worry is not what breaks now but rather what happens when today's distros will eventually be deployed on large-scale BTI-capable hardware. It's a very small risk but non-zero. The idea is that if we come across some weird problem, a fixed-up dynamic loader could avoid enabling BTI on a per-process basis without the need to do this at the system level.

Personally I'm fine with this risk. Will is not and I respect his position, hence I started the other thread to come up with a MDWE alternative.

Quoted 7 lines

The other approach works, and if the systemd folks are on board with it also should solve the underlying problem, but it creates a bit of a compatibility problem with existing containers/etc that might exist today (although running systemd/services in a container is itself a discussion).

So, frankly, I don't see why they aren't complementary.

They are complementary, though if we change the MDWE approach, there's less of a need for this patchset.

Or maybe simpler yet, we provide a tool which wipes out the gnu BTI note on binaries that are found to have BTI bugs, thereby (correctly) fixing the problem at its source. This is at least presumably doable if we are also assuming we can update glibc/etc in any environment with the problem.

Quoted 9 lines · Kees Cook

Quoted 8 lines · Mark Brown

Deployments of BTI on arm64 have run into issues interacting with systemd's MemoryDenyWriteExecute feature. Currently for dynamically linked executables the kernel will only handle architecture specific properties like BTI for the interpreter, the expectation is that the interpreter will then handle any properties on the main executable. For BTI this means remapping the executable segments PROT_EXEC | PROT_BTI.

Quoted 8 lines

Quoted 1 line

[...]

Applied to for-next/execve, thanks!

[1/2] elf: Allow architectures to parse properties on the main executable

      https://git.kernel.org/kees/c/b2f2553c8e89
[2/2] arm64: Enable BTI for main executable as well as the interpreter
      https://git.kernel.org/kees/c/b65c760600e2

Kees, please can you drop this series while Catalin's alternative solution is under discussion (his Reviewed-by preceded the other patches)?

https://lore.kernel.org/r/[email protected]

Both series expose new behaviours to userspace and we don't need both.

Quoted 8 lines · Mark Brown

Deployments of BTI on arm64 have run into issues interacting with systemd's MemoryDenyWriteExecute feature. Currently for dynamically linked executables the kernel will only handle architecture specific properties like BTI for the interpreter, the expectation is that the interpreter will then handle any properties on the main executable. For BTI this means remapping the executable segments PROT_EXEC | PROT_BTI.

Quoted 1 line

[...]

Applied to for-next/execve, thanks!