arm64: bti: Set PROT_BTI on all BTI executables mapped by the kernel

Currently for dynamically linked executables the kernel only enables PROT_BTI for the interpreter, the interpreter is responsible for enabling it for everything else including the main executable. Unfortunately this interacts poorly with systemd's MemoryDenyWriteExecute feature which uses a seccomp filter to prevent setting PROT_EXEC on already mapped memory via mprotect(), it lacks the context to detect that PROT_EXEC is already set and so refuses to allow the mprotect() on the main executable which the kernel has already mapped.

Since we don't want to force users to choose between having MDWX and BTI as these are othogonal features have the kernel enable PROT_BTI for all the ELF objects it loads, not just the dynamic linker. This means that if there is a problem with BTI it will be harder to disable at the executable level but we currently have no conditional support for this in any libc anyway so that would be new development. Ideally we would have interfaces that allowed us to more clearly specify what is enabled and disabled by a given syscall but this would be a far more difficult change to deploy.

Reported-by Jeremy Linton <[email protected]>

Suggested-by Catalin Marinas <[email protected]>

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

Cc Mark Rutland <[email protected]>

Cc Szabolcs Nagy <[email protected]>

Cc Dave Martin <[email protected]>

Cc Kees Cook <[email protected]>

Cc [email protected]

Thanks Mark for putting the patch together. You may want to add a reference to some of the discussions around the ABI, one of them:

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

(so we can keep Szabolcs accountable if something breaks ;))

For this patch:

Reviewed-by Catalin Marinas <[email protected]>

I wouldn't merge it as a fix yet but I'm ok with getting in 5.12 if Will is ok. It would give us some time to revert.

Quoted 20 lines · Mark Brown

Currently for dynamically linked executables the kernel only enables PROT_BTI for the interpreter, the interpreter is responsible for enabling it for everything else including the main executable. Unfortunately this interacts poorly with systemd's MemoryDenyWriteExecute feature which uses a seccomp filter to prevent setting PROT_EXEC on already mapped memory via mprotect(), it lacks the context to detect that PROT_EXEC is already set and so refuses to allow the mprotect() on the main executable which the kernel has already mapped.

Since we don't want to force users to choose between having MDWX and BTI as these are othogonal features have the kernel enable PROT_BTI for all the ELF objects it loads, not just the dynamic linker. This means that if there is a problem with BTI it will be harder to disable at the executable level but we currently have no conditional support for this in any libc anyway so that would be new development. Ideally we would have interfaces that allowed us to more clearly specify what is enabled and disabled by a given syscall but this would be a far more difficult change to deploy.

Quoted 8 lines

Reported-by Jeremy Linton <[email protected]>

Suggested-by Catalin Marinas <[email protected]>

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

Cc Mark Rutland <[email protected]>

Cc Szabolcs Nagy <[email protected]>

Cc Dave Martin <[email protected]>

Cc Kees Cook <[email protected]>

Cc [email protected]

Thanks Mark for putting the patch together. You may want to add a reference to some of the discussions around the ABI, one of them:

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

(so we can keep Szabolcs accountable if something breaks ;))

For this patch:

Reviewed-by Catalin Marinas <[email protected]>

I wouldn't merge it as a fix yet but I'm ok with getting in 5.12 if Will is ok. It would give us some time to revert.

Quoted 39 lines · Catalin Marinas

Quoted 20 lines · Mark Brown

Currently for dynamically linked executables the kernel only enables PROT_BTI for the interpreter, the interpreter is responsible for enabling it for everything else including the main executable. Unfortunately this interacts poorly with systemd's MemoryDenyWriteExecute feature which uses a seccomp filter to prevent setting PROT_EXEC on already mapped memory via mprotect(), it lacks the context to detect that PROT_EXEC is already set and so refuses to allow the mprotect() on the main executable which the kernel has already mapped.

Since we don't want to force users to choose between having MDWX and BTI as these are othogonal features have the kernel enable PROT_BTI for all the ELF objects it loads, not just the dynamic linker. This means that if there is a problem with BTI it will be harder to disable at the executable level but we currently have no conditional support for this in any libc anyway so that would be new development. Ideally we would have interfaces that allowed us to more clearly specify what is enabled and disabled by a given syscall but this would be a far more difficult change to deploy.

Quoted 8 lines

Reported-by Jeremy Linton <[email protected]>

Suggested-by Catalin Marinas <[email protected]>

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

Cc Mark Rutland <[email protected]>

Cc Szabolcs Nagy <[email protected]>

Cc Dave Martin <[email protected]>

Cc Kees Cook <[email protected]>

Cc [email protected]

Thanks Mark for putting the patch together. You may want to add a reference to some of the discussions around the ABI, one of them:

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

(so we can keep Szabolcs accountable if something breaks ;))

For this patch:

Quoted 4 lines

Reviewed-by Catalin Marinas <[email protected]>

I wouldn't merge it as a fix yet but I'm ok with getting in 5.12 if Will is ok. It would give us some time to revert.

I'd like an Ack from Szabolcs before we queue this.

Will

Quoted 1 line · Will Deacon

I'd like an Ack from Szabolcs before we queue this.

i'm ok with this in principle, but i will rerun
the glibc tests over night to be sure.

Quoted 5 lines · Szabolcs Nagy via Libc-alpha

Quoted 1 line · Will Deacon

I'd like an Ack from Szabolcs before we queue this.

i'm ok with this in principle, but i will rerun
the glibc tests over night to be sure.

the patch applied cleanly on top of arm64 for-next/core

but it does not work as i expected:

executables that do not have the bti property note set seems to get bti guarded by the kernel.

at least i see crash in _start when the dynamic linker (which has bti marking) transfers control to it and the start code has no bti c.

(according to strace the dynamic linker did not remap/mprotect the main exe with bti so i assume this is the kernel's doing)

can somebody verify that the notes are checked on the executable too and not just on ld.so?

Quoted 19 lines · Szabolcs Nagy via Libc-alpha

Quoted 5 lines · Szabolcs Nagy via Libc-alpha

Quoted 1 line · Will Deacon

I'd like an Ack from Szabolcs before we queue this.

i'm ok with this in principle, but i will rerun
the glibc tests over night to be sure.

the patch applied cleanly on top of arm64 for-next/core

but it does not work as i expected:

executables that do not have the bti property note set seems to get bti guarded by the kernel.

at least i see crash in _start when the dynamic linker (which has bti marking) transfers control to it and the start code has no bti c.

Quoted 6 lines

(according to strace the dynamic linker did not remap/mprotect the main exe with bti so i assume this is the kernel's doing)

can somebody verify that the notes are checked on the executable too and not just on ld.so?

Reviewed-by bites the dust...

Aha, looking at the ELF code in the kernel, it looks like some extra refactoring is needed.

We do the heavy lifting only for the image containing the userspace entry point -- i.e., ld.so in the dynamically linked case. This includes the ELF property handling. When ld.so is present, the main executable is just data so we map it in but don't do a whole lot else with it:

static int load_elf_binary(struct linux_binprm *bprm)

{
	/* ... */

	retval = parse_elf_properties(interpreter ?: bprm->file,
				      elf_property_phdata, &arch_state);

The way I originally integrated this therefore just tracks the BTI-ness (and/or equivalently how to mutate PROT_EXEC) once. Looks like we need to do that independently for ld.so and for the executable instead.

We could simplify things by treating it as an error if the executable and ld.so have different BTI properties, but that seems a bit of an own goal, since it breaks foreseeable backwards compatibility / hybrid use cases.

Currently for dynamically linked executables the kernel only enables PROT_BTI for the interpreter, the interpreter is responsible for enabling it for everything else including the main executable. Unfortunately this interacts poorly with systemd's MemoryDenyWriteExecute feature which uses a seccomp filter to prevent setting PROT_EXEC on already mapped memory via mprotect(), it lacks the context to detect that PROT_EXEC is already set and so refuses to allow the mprotect() on the main executable which the kernel has already mapped.

Since we don't want to force users to choose between having MDWX and BTI as these are othogonal features have the kernel enable PROT_BTI for all the ELF objects it loads, not just the dynamic linker. This means that if there is a problem with BTI it will be harder to disable at the executable level but we currently have no conditional support for this in any libc anyway so that would be new development. Ideally we would have interfaces that allowed us to more clearly specify what is enabled and disabled by a given syscall but this would be a far more difficult change to deploy.

Reported-by Jeremy Linton <[email protected]>

Suggested-by Catalin Marinas <[email protected]>

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

Cc Mark Rutland <[email protected]>

Cc Szabolcs Nagy <[email protected]>

Cc Dave Martin <[email protected]>

Cc Kees Cook <[email protected]>

Cc [email protected]

---

This solution was proposed by Catalin, I'm just writing it up into a patch since it looks to be what we've converged on as the most practical solution and but things seemed to have stalled out.

 arch/arm64/kernel/process.c | 8 --------
 1 file changed, 8 deletions(-)

arch/arm64/kernel/process.c +0 -0

index 71c8265b9139..0967f9e1f9fd 100644
@@ -717,14 +717,6 @@ asmlinkage void __sched arm64_preempt_schedule_irq(void)
 int arch_elf_adjust_prot(int prot, const struct arch_elf_state *state,

Question: will this change prevent BTI executables from working under a non-BTI-aware ld.so? And do we care? (I think "probably not" for both, but I'd be interested in others' views.)

Quoted 3 lines · Dave Martin

Question: will this change prevent BTI executables from working under a non-BTI-aware ld.so? And do we care? (I think "probably not" for both, but I'd be interested in others' views.)

Yes, I'd expect that we'd end up allowing a BTIed main executable without a BTI ld.so. We already have the case where we will enable BTI for the vDSO even if nothing outside the kernel supports BTI and BTI is intended to interwork happily so I'd be surprised if there were problems.