mbox series

[v10,00/13] arm64: Branch Target Identification support

Message ID 20200316165055.31179-1-broonie@kernel.org (mailing list archive)
Headers show
Series arm64: Branch Target Identification support | expand

Message

Mark Brown March 16, 2020, 4:50 p.m. UTC
This patch series implements support for ARMv8.5-A Branch Target
Identification (BTI), which is a control flow integrity protection
feature introduced as part of the ARMv8.5-A extensions.

Changes:

v10:
 - Fix build for !COMPAT configurations.
v9:
 - Move Kconfig addition to final patch in series.
 - Add patch from Daniel Kiss adding BTI information to smaps, this has
   a trivial conflict with a .rst conversion in -next.
v8:
 - Remove a redundant IS_ENABLED(CONFIG_ARM64_BTI) check.
v7:
 - Rebase onto v5.6-rc3.
 - Move comment about keeping NT_GNU_PROPERTY_TYPE_0 internal into first
   patch.
 - Add an explicit check for system_supports_bti() when parsing BTI ELF
   property for improved robustness.
v6:
 - Rebase onto v5.6-rc1.
 - Fix typos s/BYTPE/BTYPE/ in commit log for "arm64: BTI: Decode BYTPE
   bits when printing PSTATE".
v5:
 - Changed a bunch of -EIO to -ENOEXEC in the ELF parsing code.
 - Move PSR_BTYPE defines to UAPI.
 - Use compat_user_mode() rather than open coding.
 - Fix a typo s/BYTPE/BTYPE/ in syscall.c
v4:
 - Dropped patch fixing existing documentation as it has already been merged.
 - Convert WARN_ON() to WARN_ON_ONCE() in "ELF: Add ELF program property
   parsing support".
 - Added display of guarded pages to ptdump.
 - Updated for conversion of exception handling from assembler to C.

Notes:

 * GCC 9 can compile backwards-compatible BTI-enabled code with
   -mbranch-protection=bti or -mbranch-protection=standard.

 * Binutils 2.33 and later support the new ELF note.

   Creation of a BTI-enabled binary requires _everything_ linked in to
   be BTI-enabled.  For now ld --force-bti can be used to override this,
   but some things may break until the required C library support is in
   place.

   There is no straightforward way to mark a .s file as BTI-enabled:
   scraping the output from gcc -S works as a quick hack for now.

   readelf -n can be used to examing the program properties in an ELF
   file.

 * Runtime mmap() and mprotect() can be used to enable BTI on a
   page-by-page basis using the new PROT_BTI, but the code in the
   affected pages still needs to be written or compiled to contain the
   appropriate BTI landing pads.

Daniel Kiss (1):
  mm: smaps: Report arm64 guarded pages in smaps

Dave Martin (11):
  ELF: UAPI and Kconfig additions for ELF program properties
  ELF: Add ELF program property parsing support
  arm64: Basic Branch Target Identification support
  elf: Allow arch to tweak initial mmap prot flags
  arm64: elf: Enable BTI at exec based on ELF program properties
  arm64: BTI: Decode BYTPE bits when printing PSTATE
  arm64: unify native/compat instruction skipping
  arm64: traps: Shuffle code to eliminate forward declarations
  arm64: BTI: Reset BTYPE when skipping emulated instructions
  KVM: arm64: BTI: Reset BTYPE when skipping emulated instructions
  arm64: BTI: Add Kconfig entry for userspace BTI

Mark Brown (1):
  arm64: mm: Display guarded pages in ptdump

 Documentation/arm64/cpu-feature-registers.rst |   2 +
 Documentation/arm64/elf_hwcaps.rst            |   5 +
 Documentation/filesystems/proc.txt            |   1 +
 arch/arm64/Kconfig                            |  25 +++
 arch/arm64/include/asm/cpucaps.h              |   3 +-
 arch/arm64/include/asm/cpufeature.h           |   6 +
 arch/arm64/include/asm/elf.h                  |  50 ++++++
 arch/arm64/include/asm/esr.h                  |   2 +-
 arch/arm64/include/asm/exception.h            |   1 +
 arch/arm64/include/asm/hwcap.h                |   1 +
 arch/arm64/include/asm/kvm_emulate.h          |   6 +-
 arch/arm64/include/asm/mman.h                 |  37 +++++
 arch/arm64/include/asm/pgtable-hwdef.h        |   1 +
 arch/arm64/include/asm/pgtable.h              |   2 +-
 arch/arm64/include/asm/ptrace.h               |   1 +
 arch/arm64/include/asm/sysreg.h               |   4 +
 arch/arm64/include/uapi/asm/hwcap.h           |   1 +
 arch/arm64/include/uapi/asm/mman.h            |   9 ++
 arch/arm64/include/uapi/asm/ptrace.h          |   9 ++
 arch/arm64/kernel/cpufeature.c                |  33 ++++
 arch/arm64/kernel/cpuinfo.c                   |   1 +
 arch/arm64/kernel/entry-common.c              |  11 ++
 arch/arm64/kernel/process.c                   |  36 ++++-
 arch/arm64/kernel/ptrace.c                    |   2 +-
 arch/arm64/kernel/signal.c                    |  16 ++
 arch/arm64/kernel/syscall.c                   |  18 +++
 arch/arm64/kernel/traps.c                     | 131 ++++++++--------
 arch/arm64/mm/dump.c                          |   5 +
 fs/Kconfig.binfmt                             |   6 +
 fs/binfmt_elf.c                               | 145 +++++++++++++++++-
 fs/compat_binfmt_elf.c                        |   4 +
 fs/proc/task_mmu.c                            |   3 +
 include/linux/elf.h                           |  43 ++++++
 include/linux/mm.h                            |   3 +
 include/uapi/linux/elf.h                      |  11 ++
 35 files changed, 560 insertions(+), 74 deletions(-)
 create mode 100644 arch/arm64/include/asm/mman.h
 create mode 100644 arch/arm64/include/uapi/asm/mman.h


base-commit: f8788d86ab28f61f7b46eb6be375f8a726783636

Comments

Catalin Marinas March 17, 2020, 6:49 p.m. UTC | #1
On Mon, Mar 16, 2020 at 04:50:42PM +0000, Mark Brown wrote:
> Daniel Kiss (1):
>   mm: smaps: Report arm64 guarded pages in smaps
> 
> Dave Martin (11):
>   ELF: UAPI and Kconfig additions for ELF program properties
>   ELF: Add ELF program property parsing support
>   arm64: Basic Branch Target Identification support
>   elf: Allow arch to tweak initial mmap prot flags
>   arm64: elf: Enable BTI at exec based on ELF program properties
>   arm64: BTI: Decode BYTPE bits when printing PSTATE
>   arm64: unify native/compat instruction skipping
>   arm64: traps: Shuffle code to eliminate forward declarations
>   arm64: BTI: Reset BTYPE when skipping emulated instructions
>   KVM: arm64: BTI: Reset BTYPE when skipping emulated instructions
>   arm64: BTI: Add Kconfig entry for userspace BTI
> 
> Mark Brown (1):
>   arm64: mm: Display guarded pages in ptdump

I provisionally pushed this patches to linux-next (and arm64
for-next/bti).

I'm not sure whether they'll make it into 5.7 yet (still missing acks on
the fs/* changes) but at least they'll get some wider exposure,
especially as they go outside arch/arm64/.
Szabolcs Nagy March 20, 2020, 5:39 p.m. UTC | #2
The 03/16/2020 16:50, Mark Brown wrote:
> This patch series implements support for ARMv8.5-A Branch Target
> Identification (BTI), which is a control flow integrity protection
> feature introduced as part of the ARMv8.5-A extensions.

i was playing with this and it seems the kernel does not add
PROT_BTI to non-static executables (i.e. there is an interpreter).

i thought any elf that the kernel maps would get PROT_BTI from the
kernel. (i want to remove the mprotect in glibc when not necessary)

i tested by linking a hello world exe with -Wl,-z,force-bti (and
verified that the property note is there) and expected it to crash
(with SIGILL) when the dynamic linker jumps to _start in the exe,
but it executed without errors (if i do the mprotect in glibc then
i get SIGILL as expected).

is this deliberate? does the kernel map static exe and dynamic
linked exe differently?

i cant tell looking at the patches where this logic comes from.

> 
> Changes:
> 
> v10:
>  - Fix build for !COMPAT configurations.
> v9:
>  - Move Kconfig addition to final patch in series.
>  - Add patch from Daniel Kiss adding BTI information to smaps, this has
>    a trivial conflict with a .rst conversion in -next.
> v8:
>  - Remove a redundant IS_ENABLED(CONFIG_ARM64_BTI) check.
> v7:
>  - Rebase onto v5.6-rc3.
>  - Move comment about keeping NT_GNU_PROPERTY_TYPE_0 internal into first
>    patch.
>  - Add an explicit check for system_supports_bti() when parsing BTI ELF
>    property for improved robustness.
> v6:
>  - Rebase onto v5.6-rc1.
>  - Fix typos s/BYTPE/BTYPE/ in commit log for "arm64: BTI: Decode BYTPE
>    bits when printing PSTATE".
> v5:
>  - Changed a bunch of -EIO to -ENOEXEC in the ELF parsing code.
>  - Move PSR_BTYPE defines to UAPI.
>  - Use compat_user_mode() rather than open coding.
>  - Fix a typo s/BYTPE/BTYPE/ in syscall.c
> v4:
>  - Dropped patch fixing existing documentation as it has already been merged.
>  - Convert WARN_ON() to WARN_ON_ONCE() in "ELF: Add ELF program property
>    parsing support".
>  - Added display of guarded pages to ptdump.
>  - Updated for conversion of exception handling from assembler to C.
> 
> Notes:
> 
>  * GCC 9 can compile backwards-compatible BTI-enabled code with
>    -mbranch-protection=bti or -mbranch-protection=standard.
> 
>  * Binutils 2.33 and later support the new ELF note.
> 
>    Creation of a BTI-enabled binary requires _everything_ linked in to
>    be BTI-enabled.  For now ld --force-bti can be used to override this,
>    but some things may break until the required C library support is in
>    place.
> 
>    There is no straightforward way to mark a .s file as BTI-enabled:
>    scraping the output from gcc -S works as a quick hack for now.
> 
>    readelf -n can be used to examing the program properties in an ELF
>    file.
> 
>  * Runtime mmap() and mprotect() can be used to enable BTI on a
>    page-by-page basis using the new PROT_BTI, but the code in the
>    affected pages still needs to be written or compiled to contain the
>    appropriate BTI landing pads.
> 
> Daniel Kiss (1):
>   mm: smaps: Report arm64 guarded pages in smaps
> 
> Dave Martin (11):
>   ELF: UAPI and Kconfig additions for ELF program properties
>   ELF: Add ELF program property parsing support
>   arm64: Basic Branch Target Identification support
>   elf: Allow arch to tweak initial mmap prot flags
>   arm64: elf: Enable BTI at exec based on ELF program properties
>   arm64: BTI: Decode BYTPE bits when printing PSTATE
>   arm64: unify native/compat instruction skipping
>   arm64: traps: Shuffle code to eliminate forward declarations
>   arm64: BTI: Reset BTYPE when skipping emulated instructions
>   KVM: arm64: BTI: Reset BTYPE when skipping emulated instructions
>   arm64: BTI: Add Kconfig entry for userspace BTI
> 
> Mark Brown (1):
>   arm64: mm: Display guarded pages in ptdump
> 
>  Documentation/arm64/cpu-feature-registers.rst |   2 +
>  Documentation/arm64/elf_hwcaps.rst            |   5 +
>  Documentation/filesystems/proc.txt            |   1 +
>  arch/arm64/Kconfig                            |  25 +++
>  arch/arm64/include/asm/cpucaps.h              |   3 +-
>  arch/arm64/include/asm/cpufeature.h           |   6 +
>  arch/arm64/include/asm/elf.h                  |  50 ++++++
>  arch/arm64/include/asm/esr.h                  |   2 +-
>  arch/arm64/include/asm/exception.h            |   1 +
>  arch/arm64/include/asm/hwcap.h                |   1 +
>  arch/arm64/include/asm/kvm_emulate.h          |   6 +-
>  arch/arm64/include/asm/mman.h                 |  37 +++++
>  arch/arm64/include/asm/pgtable-hwdef.h        |   1 +
>  arch/arm64/include/asm/pgtable.h              |   2 +-
>  arch/arm64/include/asm/ptrace.h               |   1 +
>  arch/arm64/include/asm/sysreg.h               |   4 +
>  arch/arm64/include/uapi/asm/hwcap.h           |   1 +
>  arch/arm64/include/uapi/asm/mman.h            |   9 ++
>  arch/arm64/include/uapi/asm/ptrace.h          |   9 ++
>  arch/arm64/kernel/cpufeature.c                |  33 ++++
>  arch/arm64/kernel/cpuinfo.c                   |   1 +
>  arch/arm64/kernel/entry-common.c              |  11 ++
>  arch/arm64/kernel/process.c                   |  36 ++++-
>  arch/arm64/kernel/ptrace.c                    |   2 +-
>  arch/arm64/kernel/signal.c                    |  16 ++
>  arch/arm64/kernel/syscall.c                   |  18 +++
>  arch/arm64/kernel/traps.c                     | 131 ++++++++--------
>  arch/arm64/mm/dump.c                          |   5 +
>  fs/Kconfig.binfmt                             |   6 +
>  fs/binfmt_elf.c                               | 145 +++++++++++++++++-
>  fs/compat_binfmt_elf.c                        |   4 +
>  fs/proc/task_mmu.c                            |   3 +
>  include/linux/elf.h                           |  43 ++++++
>  include/linux/mm.h                            |   3 +
>  include/uapi/linux/elf.h                      |  11 ++
>  35 files changed, 560 insertions(+), 74 deletions(-)
>  create mode 100644 arch/arm64/include/asm/mman.h
>  create mode 100644 arch/arm64/include/uapi/asm/mman.h
> 
> 
> base-commit: f8788d86ab28f61f7b46eb6be375f8a726783636
> -- 
> 2.20.1
> 

--
Catalin Marinas March 23, 2020, 12:21 p.m. UTC | #3
On Fri, Mar 20, 2020 at 05:39:46PM +0000, Szabolcs Nagy wrote:
> The 03/16/2020 16:50, Mark Brown wrote:
> > This patch series implements support for ARMv8.5-A Branch Target
> > Identification (BTI), which is a control flow integrity protection
> > feature introduced as part of the ARMv8.5-A extensions.
> 
> i was playing with this and it seems the kernel does not add
> PROT_BTI to non-static executables (i.e. there is an interpreter).
> 
> i thought any elf that the kernel maps would get PROT_BTI from the
> kernel. (i want to remove the mprotect in glibc when not necessary)

I haven't followed the early discussions but I think this makes sense.

> i tested by linking a hello world exe with -Wl,-z,force-bti (and
> verified that the property note is there) and expected it to crash
> (with SIGILL) when the dynamic linker jumps to _start in the exe,
> but it executed without errors (if i do the mprotect in glibc then
> i get SIGILL as expected).
> 
> is this deliberate? does the kernel map static exe and dynamic
> linked exe differently?

I think the logic is in patch 5:

+int arch_elf_adjust_prot(int prot, const struct arch_elf_state *state,
+                        bool has_interp, bool is_interp)
+{
+       if (is_interp != has_interp)
+               return prot;
+
+       if (!(state->flags & ARM64_ELF_BTI))
+               return prot;
+
+       if (prot & PROT_EXEC)
+               prot |= PROT_BTI;
+
+       return prot;
+}

At a quick look, for dynamic binaries we have has_interp == true and
is_interp == false. I don't know why but, either way, the above code
needs a comment with some justification.
Mark Brown March 23, 2020, 1:24 p.m. UTC | #4
On Mon, Mar 23, 2020 at 12:21:44PM +0000, Catalin Marinas wrote:
> On Fri, Mar 20, 2020 at 05:39:46PM +0000, Szabolcs Nagy wrote:

> +int arch_elf_adjust_prot(int prot, const struct arch_elf_state *state,
> +                        bool has_interp, bool is_interp)
> +{
> +       if (is_interp != has_interp)
> +               return prot;
> +
> +       if (!(state->flags & ARM64_ELF_BTI))
> +               return prot;
> +
> +       if (prot & PROT_EXEC)
> +               prot |= PROT_BTI;
> +
> +       return prot;
> +}

> At a quick look, for dynamic binaries we have has_interp == true and
> is_interp == false. I don't know why but, either way, the above code
> needs a comment with some justification.

I don't really know for certain either, I inherited this code as is with
the understanding that this was all agreed with the toolchain and libc
people - the actual discussion that lead to the decisions being made
happened before I was involved.  My understanding is that the idea was
that the dynamic linker would be responsible for mapping everything in
dynamic applications other than itself but other than consistency I
don't know why.  I guess it defers more decision making to userspace but
I'm having a hard time thinking of sensible cases where one might wish
to make a decision other than enabling PROT_BTI.

I'd be perfectly happy to drop the check if that makes more sense to
people, otherwise I can send a patch adding a comment explaining the
situation.
Mark Rutland March 23, 2020, 1:57 p.m. UTC | #5
On Mon, Mar 23, 2020 at 01:24:12PM +0000, Mark Brown wrote:
> On Mon, Mar 23, 2020 at 12:21:44PM +0000, Catalin Marinas wrote:
> > On Fri, Mar 20, 2020 at 05:39:46PM +0000, Szabolcs Nagy wrote:
> 
> > +int arch_elf_adjust_prot(int prot, const struct arch_elf_state *state,
> > +                        bool has_interp, bool is_interp)
> > +{
> > +       if (is_interp != has_interp)
> > +               return prot;
> > +
> > +       if (!(state->flags & ARM64_ELF_BTI))
> > +               return prot;
> > +
> > +       if (prot & PROT_EXEC)
> > +               prot |= PROT_BTI;
> > +
> > +       return prot;
> > +}
> 
> > At a quick look, for dynamic binaries we have has_interp == true and
> > is_interp == false. I don't know why but, either way, the above code
> > needs a comment with some justification.
> 
> I don't really know for certain either, I inherited this code as is with
> the understanding that this was all agreed with the toolchain and libc
> people - the actual discussion that lead to the decisions being made
> happened before I was involved.  My understanding is that the idea was
> that the dynamic linker would be responsible for mapping everything in
> dynamic applications other than itself but other than consistency I
> don't know why.  I guess it defers more decision making to userspace but
> I'm having a hard time thinking of sensible cases where one might wish
> to make a decision other than enabling PROT_BTI.

My understanding was this had been agreed with the toolchain folk a
while back -- anything static loaded by the kernel (i.e. a static
executable or the dynamic linker) would get GP set. In other cases the
linker will mess with the permissions on the pages anyhow, and needs to
be aware of BTI in order to do the right thing, so it was better to
leave it to userspace consistently (e.g. as that had the least risk of
subtle changes in behaviour leading to ABI difficulties).

> I'd be perfectly happy to drop the check if that makes more sense to
> people, otherwise I can send a patch adding a comment explaining the
> situation.

I think it would be best to document the current behaviour, as it's a
simple ABI that we can guarantee, and the dynamic linker will have to be
aware of BTI in order to do the right thing anyhow.

Thanks,
Mark.
Catalin Marinas March 23, 2020, 2:39 p.m. UTC | #6
On Mon, Mar 23, 2020 at 01:57:22PM +0000, Mark Rutland wrote:
> On Mon, Mar 23, 2020 at 01:24:12PM +0000, Mark Brown wrote:
> > On Mon, Mar 23, 2020 at 12:21:44PM +0000, Catalin Marinas wrote:
> > > On Fri, Mar 20, 2020 at 05:39:46PM +0000, Szabolcs Nagy wrote:
> > 
> > > +int arch_elf_adjust_prot(int prot, const struct arch_elf_state *state,
> > > +                        bool has_interp, bool is_interp)
> > > +{
> > > +       if (is_interp != has_interp)
> > > +               return prot;
> > > +
> > > +       if (!(state->flags & ARM64_ELF_BTI))
> > > +               return prot;
> > > +
> > > +       if (prot & PROT_EXEC)
> > > +               prot |= PROT_BTI;
> > > +
> > > +       return prot;
> > > +}
> > 
> > > At a quick look, for dynamic binaries we have has_interp == true and
> > > is_interp == false. I don't know why but, either way, the above code
> > > needs a comment with some justification.
> > 
> > I don't really know for certain either, I inherited this code as is with
> > the understanding that this was all agreed with the toolchain and libc
> > people - the actual discussion that lead to the decisions being made
> > happened before I was involved.  My understanding is that the idea was
> > that the dynamic linker would be responsible for mapping everything in
> > dynamic applications other than itself but other than consistency I
> > don't know why.  I guess it defers more decision making to userspace but
> > I'm having a hard time thinking of sensible cases where one might wish
> > to make a decision other than enabling PROT_BTI.
> 
> My understanding was this had been agreed with the toolchain folk a
> while back -- anything static loaded by the kernel (i.e. a static
> executable or the dynamic linker) would get GP set. In other cases the
> linker will mess with the permissions on the pages anyhow, and needs to
> be aware of BTI in order to do the right thing, so it was better to
> leave it to userspace consistently (e.g. as that had the least risk of
> subtle changes in behaviour leading to ABI difficulties).

So this means that the interpreter will have to mprotect(PROT_BTI) the
text section of the primary executable. For subsequent libraries, it
calls mmap() explicitly anyway but not for the main executable (IIUC).

> > I'd be perfectly happy to drop the check if that makes more sense to
> > people, otherwise I can send a patch adding a comment explaining the
> > situation.
> 
> I think it would be best to document the current behaviour, as it's a
> simple ABI that we can guarantee, and the dynamic linker will have to be
> aware of BTI in order to do the right thing anyhow.

That's a valid point. If we have an old dynamic linker and the kernel
enabled BTI automatically for the main executable, could things go wrong
(e.g. does the PLT need to be BTI-aware)?
Mark Rutland March 23, 2020, 2:55 p.m. UTC | #7
On Mon, Mar 23, 2020 at 02:39:55PM +0000, Catalin Marinas wrote:
> On Mon, Mar 23, 2020 at 01:57:22PM +0000, Mark Rutland wrote:
> > On Mon, Mar 23, 2020 at 01:24:12PM +0000, Mark Brown wrote:
> > > On Mon, Mar 23, 2020 at 12:21:44PM +0000, Catalin Marinas wrote:
> > > > On Fri, Mar 20, 2020 at 05:39:46PM +0000, Szabolcs Nagy wrote:
> > > 
> > > > +int arch_elf_adjust_prot(int prot, const struct arch_elf_state *state,
> > > > +                        bool has_interp, bool is_interp)
> > > > +{
> > > > +       if (is_interp != has_interp)
> > > > +               return prot;
> > > > +
> > > > +       if (!(state->flags & ARM64_ELF_BTI))
> > > > +               return prot;
> > > > +
> > > > +       if (prot & PROT_EXEC)
> > > > +               prot |= PROT_BTI;
> > > > +
> > > > +       return prot;
> > > > +}
> > > 
> > > > At a quick look, for dynamic binaries we have has_interp == true and
> > > > is_interp == false. I don't know why but, either way, the above code
> > > > needs a comment with some justification.
> > > 
> > > I don't really know for certain either, I inherited this code as is with
> > > the understanding that this was all agreed with the toolchain and libc
> > > people - the actual discussion that lead to the decisions being made
> > > happened before I was involved.  My understanding is that the idea was
> > > that the dynamic linker would be responsible for mapping everything in
> > > dynamic applications other than itself but other than consistency I
> > > don't know why.  I guess it defers more decision making to userspace but
> > > I'm having a hard time thinking of sensible cases where one might wish
> > > to make a decision other than enabling PROT_BTI.
> > 
> > My understanding was this had been agreed with the toolchain folk a
> > while back -- anything static loaded by the kernel (i.e. a static
> > executable or the dynamic linker) would get GP set. In other cases the
> > linker will mess with the permissions on the pages anyhow, and needs to
> > be aware of BTI in order to do the right thing, so it was better to
> > leave it to userspace consistently (e.g. as that had the least risk of
> > subtle changes in behaviour leading to ABI difficulties).
> 
> So this means that the interpreter will have to mprotect(PROT_BTI) the
> text section of the primary executable.

Yes, but after fixing up any relocations in that section it's going to
have to call mprotect() on it anyhow (e.g. in order to make it
read-only), and in doing so would throw away BTI unless it was BTI
aware.

> > I think it would be best to document the current behaviour, as it's a
> > simple ABI that we can guarantee, and the dynamic linker will have to be
> > aware of BTI in order to do the right thing anyhow.
> 
> That's a valid point. If we have an old dynamic linker and the kernel
> enabled BTI automatically for the main executable, could things go wrong
> (e.g. does the PLT need to be BTI-aware)?

I believe that a PLT in an unguarded page needs no special treatment. A
PLT within a guarded page needs to be built specially for BTI.

Thanks,
Mark.
Mark Rutland March 23, 2020, 3:02 p.m. UTC | #8
On Mon, Mar 23, 2020 at 02:39:55PM +0000, Catalin Marinas wrote:
> On Mon, Mar 23, 2020 at 01:57:22PM +0000, Mark Rutland wrote:
> > On Mon, Mar 23, 2020 at 01:24:12PM +0000, Mark Brown wrote:
> > > On Mon, Mar 23, 2020 at 12:21:44PM +0000, Catalin Marinas wrote:
> > > > On Fri, Mar 20, 2020 at 05:39:46PM +0000, Szabolcs Nagy wrote:
> > > 
> > > > +int arch_elf_adjust_prot(int prot, const struct arch_elf_state *state,
> > > > +                        bool has_interp, bool is_interp)
> > > > +{
> > > > +       if (is_interp != has_interp)
> > > > +               return prot;
> > > > +
> > > > +       if (!(state->flags & ARM64_ELF_BTI))
> > > > +               return prot;
> > > > +
> > > > +       if (prot & PROT_EXEC)
> > > > +               prot |= PROT_BTI;
> > > > +
> > > > +       return prot;
> > > > +}

> > I think it would be best to document the current behaviour, as it's a
> > simple ABI that we can guarantee, and the dynamic linker will have to be
> > aware of BTI in order to do the right thing anyhow.
> 
> That's a valid point. If we have an old dynamic linker and the kernel
> enabled BTI automatically for the main executable, could things go wrong
> (e.g. does the PLT need to be BTI-aware)?

Also worth noting that an old dynamic linker won't have ARM64_ELF_BTI
set, so the kernel will not enable BTI for this.

Mark.
Mark Brown March 23, 2020, 3:32 p.m. UTC | #9
On Mon, Mar 23, 2020 at 02:55:46PM +0000, Mark Rutland wrote:
> On Mon, Mar 23, 2020 at 02:39:55PM +0000, Catalin Marinas wrote:

> > So this means that the interpreter will have to mprotect(PROT_BTI) the
> > text section of the primary executable.

> Yes, but after fixing up any relocations in that section it's going to
> have to call mprotect() on it anyhow (e.g. in order to make it
> read-only), and in doing so would throw away BTI unless it was BTI
> aware.

Ah, of course - I forgot that's not a read/modify/write cycle.  I'll
send the comment version.

> > That's a valid point. If we have an old dynamic linker and the kernel
> > enabled BTI automatically for the main executable, could things go wrong
> > (e.g. does the PLT need to be BTI-aware)?

> I believe that a PLT in an unguarded page needs no special treatment. A
> PLT within a guarded page needs to be built specially for BTI.

Unguarded stuff is unaffected.
Szabolcs Nagy March 24, 2020, 3:43 p.m. UTC | #10
The 03/23/2020 14:55, Mark Rutland wrote:
> On Mon, Mar 23, 2020 at 02:39:55PM +0000, Catalin Marinas wrote:
> > On Mon, Mar 23, 2020 at 01:57:22PM +0000, Mark Rutland wrote:
> > > On Mon, Mar 23, 2020 at 01:24:12PM +0000, Mark Brown wrote:
> > > > On Mon, Mar 23, 2020 at 12:21:44PM +0000, Catalin Marinas wrote:
> > > > > On Fri, Mar 20, 2020 at 05:39:46PM +0000, Szabolcs Nagy wrote:
> > > > 
> > > > > +int arch_elf_adjust_prot(int prot, const struct arch_elf_state *state,
> > > > > +                        bool has_interp, bool is_interp)
> > > > > +{
> > > > > +       if (is_interp != has_interp)
> > > > > +               return prot;
> > > > > +
> > > > > +       if (!(state->flags & ARM64_ELF_BTI))
> > > > > +               return prot;
> > > > > +
> > > > > +       if (prot & PROT_EXEC)
> > > > > +               prot |= PROT_BTI;
> > > > > +
> > > > > +       return prot;
> > > > > +}
> > > > 
> > > > > At a quick look, for dynamic binaries we have has_interp == true and
> > > > > is_interp == false. I don't know why but, either way, the above code
> > > > > needs a comment with some justification.
> > > > 
> > > > I don't really know for certain either, I inherited this code as is with
> > > > the understanding that this was all agreed with the toolchain and libc
> > > > people - the actual discussion that lead to the decisions being made
> > > > happened before I was involved.  My understanding is that the idea was
> > > > that the dynamic linker would be responsible for mapping everything in
> > > > dynamic applications other than itself but other than consistency I
> > > > don't know why.  I guess it defers more decision making to userspace but
> > > > I'm having a hard time thinking of sensible cases where one might wish
> > > > to make a decision other than enabling PROT_BTI.
> > > 
> > > My understanding was this had been agreed with the toolchain folk a
> > > while back -- anything static loaded by the kernel (i.e. a static
> > > executable or the dynamic linker) would get GP set. In other cases the
> > > linker will mess with the permissions on the pages anyhow, and needs to
> > > be aware of BTI in order to do the right thing, so it was better to
> > > leave it to userspace consistently (e.g. as that had the least risk of
> > > subtle changes in behaviour leading to ABI difficulties).
> > 
> > So this means that the interpreter will have to mprotect(PROT_BTI) the
> > text section of the primary executable.
> 
> Yes, but after fixing up any relocations in that section it's going to
> have to call mprotect() on it anyhow (e.g. in order to make it
> read-only), and in doing so would throw away BTI unless it was BTI
> aware.

note: on the main exe only one mprotect is used in case
there is PT_GNU_RELRO (or DF_BIND_NOW) to mark part of
the rw data segment read only. so if PROT_BTI on main
exe is ld.so responsibility that adds one more syscall
to the program startup (not a huge cost).

(currently executable segment can be mprotected by libc
in case of text relocations but those are not fully
supported and won't work under various kernel hardening
schemes that disallow exec+write mappings)
Mark Brown April 22, 2020, 3:44 p.m. UTC | #11
On Mon, Mar 16, 2020 at 04:50:42PM +0000, Mark Brown wrote:
> This patch series implements support for ARMv8.5-A Branch Target
> Identification (BTI), which is a control flow integrity protection
> feature introduced as part of the ARMv8.5-A extensions.

I've not resent this since the branch is still sitting in the arm64 tree
but it's also not in -next at the minute - is there anything you're
waiting for from my end here?
Catalin Marinas April 22, 2020, 4:29 p.m. UTC | #12
On Wed, Apr 22, 2020 at 04:44:36PM +0100, Mark Brown wrote:
> On Mon, Mar 16, 2020 at 04:50:42PM +0000, Mark Brown wrote:
> > This patch series implements support for ARMv8.5-A Branch Target
> > Identification (BTI), which is a control flow integrity protection
> > feature introduced as part of the ARMv8.5-A extensions.
> 
> I've not resent this since the branch is still sitting in the arm64 tree
> but it's also not in -next at the minute - is there anything you're
> waiting for from my end here?

It's up to Will whether he wants a new series posted. The for-next/bti
branch is complete AFAICT, only that normally we start queueing stuff
(and push to -next) around -rc3.
Will Deacon April 28, 2020, 1:28 p.m. UTC | #13
On Wed, Apr 22, 2020 at 05:29:54PM +0100, Catalin Marinas wrote:
> On Wed, Apr 22, 2020 at 04:44:36PM +0100, Mark Brown wrote:
> > On Mon, Mar 16, 2020 at 04:50:42PM +0000, Mark Brown wrote:
> > > This patch series implements support for ARMv8.5-A Branch Target
> > > Identification (BTI), which is a control flow integrity protection
> > > feature introduced as part of the ARMv8.5-A extensions.
> > 
> > I've not resent this since the branch is still sitting in the arm64 tree
> > but it's also not in -next at the minute - is there anything you're
> > waiting for from my end here?
> 
> It's up to Will whether he wants a new series posted. The for-next/bti
> branch is complete AFAICT, only that normally we start queueing stuff
> (and push to -next) around -rc3.

I'm happy either way, but it would be nice to base other BTI patches on
top of this branch. Mark -- is it easier for you to refresh the series
against v5.7-rc3, or leave it like it is? Please just let me know either
way.

Thanks,

Will
Mark Brown April 28, 2020, 3:12 p.m. UTC | #14
On Tue, Apr 28, 2020 at 02:28:05PM +0100, Will Deacon wrote:

> I'm happy either way, but it would be nice to base other BTI patches on
> top of this branch. Mark -- is it easier for you to refresh the series
> against v5.7-rc3, or leave it like it is? Please just let me know either
> way.

It's probably easier for me if you just use the existing branch, I've
already got a branch based on a merge down.
Will Deacon April 28, 2020, 3:18 p.m. UTC | #15
On Tue, Apr 28, 2020 at 04:12:05PM +0100, Mark Brown wrote:
> On Tue, Apr 28, 2020 at 02:28:05PM +0100, Will Deacon wrote:
> 
> > I'm happy either way, but it would be nice to base other BTI patches on
> > top of this branch. Mark -- is it easier for you to refresh the series
> > against v5.7-rc3, or leave it like it is? Please just let me know either
> > way.
> 
> It's probably easier for me if you just use the existing branch, I've
> already got a branch based on a merge down.

Okey doke, I'll funnel that in the direction of linux-next then. It does
mean that any subsequent patches for 5.8 that depend on BTI will need to
be based on this branch, so as long as you're ok with that then it's fine
by me (since I won't be able to apply patches if they refer to changes
introduced in the recent merge window).

Will
Mark Brown April 28, 2020, 3:58 p.m. UTC | #16
On Tue, Apr 28, 2020 at 04:18:16PM +0100, Will Deacon wrote:
> On Tue, Apr 28, 2020 at 04:12:05PM +0100, Mark Brown wrote:

> > It's probably easier for me if you just use the existing branch, I've
> > already got a branch based on a merge down.

> Okey doke, I'll funnel that in the direction of linux-next then. It does
> mean that any subsequent patches for 5.8 that depend on BTI will need to
> be based on this branch, so as long as you're ok with that then it's fine
> by me (since I won't be able to apply patches if they refer to changes
> introduced in the recent merge window).

That's not a problem, that's what I've got already and if I try to send
everything based off -rc3 directly the series would get unmanagably
large.  Actually unless you think it's a bad idea I think what I'll do
is go and send out a couple of the preparatory changes (the insn updates
and the last bit of annotation conversions) separately for that branch
while I finalize the revisions of the main BTI kernel bit, hopefully
that'll make the review a bit more approachable.
Will Deacon April 28, 2020, 4:01 p.m. UTC | #17
On Tue, Apr 28, 2020 at 04:58:12PM +0100, Mark Brown wrote:
> On Tue, Apr 28, 2020 at 04:18:16PM +0100, Will Deacon wrote:
> > On Tue, Apr 28, 2020 at 04:12:05PM +0100, Mark Brown wrote:
> 
> > > It's probably easier for me if you just use the existing branch, I've
> > > already got a branch based on a merge down.
> 
> > Okey doke, I'll funnel that in the direction of linux-next then. It does
> > mean that any subsequent patches for 5.8 that depend on BTI will need to
> > be based on this branch, so as long as you're ok with that then it's fine
> > by me (since I won't be able to apply patches if they refer to changes
> > introduced in the recent merge window).
> 
> That's not a problem, that's what I've got already and if I try to send
> everything based off -rc3 directly the series would get unmanagably
> large.  Actually unless you think it's a bad idea I think what I'll do
> is go and send out a couple of the preparatory changes (the insn updates
> and the last bit of annotation conversions) separately for that branch
> while I finalize the revisions of the main BTI kernel bit, hopefully
> that'll make the review a bit more approachable.

Okey doke, sounds good to me. I'm queuing stuff atm, so as long you tell
me what I need to apply things against then we should be good.

Will
Will Deacon April 30, 2020, 9:26 p.m. UTC | #18
On Tue, Apr 28, 2020 at 05:01:43PM +0100, Will Deacon wrote:
> On Tue, Apr 28, 2020 at 04:58:12PM +0100, Mark Brown wrote:
> > On Tue, Apr 28, 2020 at 04:18:16PM +0100, Will Deacon wrote:
> > > On Tue, Apr 28, 2020 at 04:12:05PM +0100, Mark Brown wrote:
> > 
> > > > It's probably easier for me if you just use the existing branch, I've
> > > > already got a branch based on a merge down.
> > 
> > > Okey doke, I'll funnel that in the direction of linux-next then. It does
> > > mean that any subsequent patches for 5.8 that depend on BTI will need to
> > > be based on this branch, so as long as you're ok with that then it's fine
> > > by me (since I won't be able to apply patches if they refer to changes
> > > introduced in the recent merge window).
> > 
> > That's not a problem, that's what I've got already and if I try to send
> > everything based off -rc3 directly the series would get unmanagably
> > large.  Actually unless you think it's a bad idea I think what I'll do
> > is go and send out a couple of the preparatory changes (the insn updates
> > and the last bit of annotation conversions) separately for that branch
> > while I finalize the revisions of the main BTI kernel bit, hopefully
> > that'll make the review a bit more approachable.
> 
> Okey doke, sounds good to me. I'm queuing stuff atm, so as long you tell
> me what I need to apply things against then we should be good.

Just a heads up: I've renamed for-next/bti to for-next/bti-user, so it
doesn't get confusing with the pending in-kernel BTI patches. All the commit
SHAs remain unchanged.

Will