[PATCH 0/3] arm64: Dynamic shadow call stack fixes

[PATCH 0/3] arm64: Dynamic shadow call stack fixes

[Ard Biesheuvel]

From: Ard Biesheuvel <ardb@kernel.org>

When building modules using the large code model, the emitted DWARF
metadata uses large fields for the code references too, and this is
currently not being handled. 

The CIE header frame contains metadata that describes the size of these
fields, and so this should be parsed and taken into account when
processing the FDE frames (patch #2)

But first, the handling of this CIE frame header needs to be fixed, as
it currently [inadvertently] used hardcoded defaults for some values
that happen to always match what Clang emits in practice (patch #1) 

Cc: Sami Tolvanen <samitolvanen@google.com>
Cc: Kees Cook <kees@kernel.org>
Cc: Nathan Chancellor <nathan@kernel.org>

Ard Biesheuvel (3):
  arm64/scs: Fix handling of DWARF augmentation data in CIE/FDE frames
  arm64/scs: Deal with 64-bit relative offsets in FDE frames
  arm64/scs: Drop unused prototype __pi_scs_patch_vmlinux()

 arch/arm64/include/asm/scs.h     |  8 +-
 arch/arm64/kernel/module.c       | 10 ++-
 arch/arm64/kernel/pi/patch-scs.c | 93 ++++++++++++++------
 3 files changed, 80 insertions(+), 31 deletions(-)

-- 
2.47.0.277.g8800431eea-goog

[PATCH 1/3] arm64/scs: Fix handling of DWARF augmentation data in CIE/FDE frames

[Ard Biesheuvel]

From: Ard Biesheuvel <ardb@kernel.org>

The dynamic SCS patching code pretends to parse the DWARF augmentation
data in the CIE (header) frame, and handle accordingly when processing
the individual FDE frames based on this CIE frame. However, the boolean
variable is defined inside the loop, and so the parsed value is ignored.

The same applies to the code alignment field, which is also read from
the header but then discarded.

This was never spotted before because Clang is the only compiler that
supports dynamic SCS patching (which is essentially an Android feature),
and the unwind tables it produces are highly uniform, and match the
de facto defaults.

So instead of testing for the 'z' flag in the augmentation data field,
require a fixed augmentation data string of 'zR', and simplify the rest
of the code accordingly.

Also introduce some error codes to specify why the patching failed, and
log it to the kernel console on failure when this happens when loading a
module. (Doing so for vmlinux is infeasible, as the patching is done
extremely early in the boot.)

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/arm64/include/asm/scs.h     |  7 +++
 arch/arm64/kernel/module.c       | 10 +++-
 arch/arm64/kernel/pi/patch-scs.c | 63 +++++++++++---------
 3 files changed, 50 insertions(+), 30 deletions(-)

diff --git a/arch/arm64/include/asm/scs.h b/arch/arm64/include/asm/scs.h
index 2e010ea76be2..934e9217cd74 100644
--- a/arch/arm64/include/asm/scs.h
+++ b/arch/arm64/include/asm/scs.h
@@ -46,6 +46,13 @@ static inline void dynamic_scs_init(void)
 static inline void dynamic_scs_init(void) {}
 #endif
 
+enum {
+	EDYNSCS_INVALID_CIE_HEADER		= 1,
+	EDYNSCS_INVALID_CIE_SDATA_SIZE		= 2,
+	EDYNSCS_INVALID_FDE_AUGM_DATA_SIZE	= 3,
+	EDYNSCS_INVALID_CFA_OPCODE		= 4,
+};
+
 int __pi_scs_patch(const u8 eh_frame[], int size);
 asmlinkage void __pi_scs_patch_vmlinux(void);
 
diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
index 36b25af56324..06bb680bfe97 100644
--- a/arch/arm64/kernel/module.c
+++ b/arch/arm64/kernel/module.c
@@ -462,14 +462,20 @@ int module_finalize(const Elf_Ehdr *hdr,
 		    struct module *me)
 {
 	const Elf_Shdr *s;
+	int ret;
+
 	s = find_section(hdr, sechdrs, ".altinstructions");
 	if (s)
 		apply_alternatives_module((void *)s->sh_addr, s->sh_size);
 
 	if (scs_is_dynamic()) {
 		s = find_section(hdr, sechdrs, ".init.eh_frame");
-		if (s)
-			__pi_scs_patch((void *)s->sh_addr, s->sh_size);
+		if (s) {
+			ret = __pi_scs_patch((void *)s->sh_addr, s->sh_size);
+			if (ret)
+				pr_err("module %s: error occurred during dynamic SCS patching (%d)\n",
+				       me->name, ret);
+		}
 	}
 
 	return module_init_ftrace_plt(hdr, sechdrs, me);
diff --git a/arch/arm64/kernel/pi/patch-scs.c b/arch/arm64/kernel/pi/patch-scs.c
index 49d8b40e61bc..cec8f0a52bbc 100644
--- a/arch/arm64/kernel/pi/patch-scs.c
+++ b/arch/arm64/kernel/pi/patch-scs.c
@@ -120,7 +120,11 @@ struct eh_frame {
 	union {
 		struct { // CIE
 			u8	version;
-			u8	augmentation_string[];
+			u8	augmentation_string[3];
+			u8	code_alignment_factor;
+			u8	data_alignment_factor;
+			u8	return_address_register;
+			u8	augmentation_data_size;
 		};
 
 		struct { // FDE
@@ -132,25 +136,21 @@ struct eh_frame {
 };
 
 static int scs_handle_fde_frame(const struct eh_frame *frame,
-				bool fde_has_augmentation_data,
 				int code_alignment_factor,
 				bool dry_run)
 {
 	int size = frame->size - offsetof(struct eh_frame, opcodes) + 4;
 	u64 loc = (u64)offset_to_ptr(&frame->initial_loc);
 	const u8 *opcode = frame->opcodes;
+	int l;
 
-	if (fde_has_augmentation_data) {
-		int l;
+	// assume single byte uleb128_t for augmentation data size
+	if (*opcode & BIT(7))
+		return EDYNSCS_INVALID_FDE_AUGM_DATA_SIZE;
 
-		// assume single byte uleb128_t
-		if (WARN_ON(*opcode & BIT(7)))
-			return -ENOEXEC;
-
-		l = *opcode++;
-		opcode += l;
-		size -= l + 1;
-	}
+	l = *opcode++;
+	opcode += l;
+	size -= l + 1;
 
 	/*
 	 * Starting from 'loc', apply the CFA opcodes that advance the location
@@ -201,7 +201,7 @@ static int scs_handle_fde_frame(const struct eh_frame *frame,
 			break;
 
 		default:
-			return -ENOEXEC;
+			return EDYNSCS_INVALID_CFA_OPCODE;
 		}
 	}
 	return 0;
@@ -209,12 +209,11 @@ static int scs_handle_fde_frame(const struct eh_frame *frame,
 
 int scs_patch(const u8 eh_frame[], int size)
 {
+	int code_alignment_factor = 1;
 	const u8 *p = eh_frame;
 
 	while (size > 4) {
 		const struct eh_frame *frame = (const void *)p;
-		bool fde_has_augmentation_data = true;
-		int code_alignment_factor = 1;
 		int ret;
 
 		if (frame->size == 0 ||
@@ -223,28 +222,36 @@ int scs_patch(const u8 eh_frame[], int size)
 			break;
 
 		if (frame->cie_id_or_pointer == 0) {
-			const u8 *p = frame->augmentation_string;
-
-			/* a 'z' in the augmentation string must come first */
-			fde_has_augmentation_data = *p == 'z';
+			/*
+			 * Require presence of augmentation data (z) with a
+			 * specifier for the size of the FDE initial_loc and
+			 * range fields (R), and nothing else.
+			 */
+			if (strcmp(frame->augmentation_string, "zR"))
+				return EDYNSCS_INVALID_CIE_HEADER;
 
 			/*
 			 * The code alignment factor is a uleb128 encoded field
 			 * but given that the only sensible values are 1 or 4,
-			 * there is no point in decoding the whole thing.
+			 * there is no point in decoding the whole thing.  Also
+			 * sanity check the size of the data alignment factor
+			 * field, and the values of the return address register
+			 * and augmentation data size fields.
 			 */
-			p += strlen(p) + 1;
-			if (!WARN_ON(*p & BIT(7)))
-				code_alignment_factor = *p;
+			if ((frame->code_alignment_factor & BIT(7)) ||
+			    (frame->data_alignment_factor & BIT(7)) ||
+			    frame->return_address_register != 30 ||
+			    frame->augmentation_data_size != 1)
+				return EDYNSCS_INVALID_CIE_HEADER;
+
+			code_alignment_factor = frame->code_alignment_factor;
 		} else {
-			ret = scs_handle_fde_frame(frame,
-						   fde_has_augmentation_data,
-						   code_alignment_factor,
+			ret = scs_handle_fde_frame(frame, code_alignment_factor,
 						   true);
 			if (ret)
 				return ret;
-			scs_handle_fde_frame(frame, fde_has_augmentation_data,
-					     code_alignment_factor, false);
+			scs_handle_fde_frame(frame, code_alignment_factor,
+					     false);
 		}
 
 		p += sizeof(frame->size) + frame->size;
-- 
2.47.0.277.g8800431eea-goog

Re: [PATCH 1/3] arm64/scs: Fix handling of DWARF augmentation data in CIE/FDE frames

[Sami Tolvanen]

On Wed, Nov 6, 2024 at 6:55 PM Ard Biesheuvel <ardb+git@google.com> wrote:
>
> Also introduce some error codes to specify why the patching failed, and
> log it to the kernel console on failure when this happens when loading a
> module. (Doing so for vmlinux is infeasible, as the patching is done
> extremely early in the boot.)

It's a bit unfortunate that we might fail quietly for vmlinux. Could
we store scs_patch() return value somewhere and print out a warning in
dynamic_scs_init() if something went wrong, for example?

Sami

Re: [PATCH 1/3] arm64/scs: Fix handling of DWARF augmentation data in CIE/FDE frames

[Ard Biesheuvel]

On Wed, 6 Nov 2024 at 23:13, Sami Tolvanen <samitolvanen@google.com> wrote:
>
> On Wed, Nov 6, 2024 at 6:55 PM Ard Biesheuvel <ardb+git@google.com> wrote:
> >
> > Also introduce some error codes to specify why the patching failed, and
> > log it to the kernel console on failure when this happens when loading a
> > module. (Doing so for vmlinux is infeasible, as the patching is done
> > extremely early in the boot.)
>
> It's a bit unfortunate that we might fail quietly for vmlinux. Could
> we store scs_patch() return value somewhere and print out a warning in
> dynamic_scs_init() if something went wrong, for example?
>

Yeah good point.

[PATCH 2/3] arm64/scs: Deal with 64-bit relative offsets in FDE frames

[Ard Biesheuvel]

From: Ard Biesheuvel <ardb@kernel.org>

In some cases, the compiler may decide to emit DWARF FDE frames with
64-bit signed fields for the code offset and range fields. This may
happen when using the large code model, for instance, which permits
an executable to be spread out over more than 4 GiB of address space.

Whether this is the case can be inferred from the augmentation data in
the CIE frame, so decode this data before processing the FDE frames.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/arm64/kernel/pi/patch-scs.c | 34 ++++++++++++++++++--
 1 file changed, 32 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/pi/patch-scs.c b/arch/arm64/kernel/pi/patch-scs.c
index cec8f0a52bbc..55d0cd64ef71 100644
--- a/arch/arm64/kernel/pi/patch-scs.c
+++ b/arch/arm64/kernel/pi/patch-scs.c
@@ -50,6 +50,10 @@ bool dynamic_scs_is_enabled;
 #define DW_CFA_GNU_negative_offset_extended 0x2f
 #define DW_CFA_hi_user                      0x3f
 
+#define DW_EH_PE_sdata4                     0x0b
+#define DW_EH_PE_sdata8                     0x0c
+#define DW_EH_PE_pcrel                      0x10
+
 enum {
 	PACIASP		= 0xd503233f,
 	AUTIASP		= 0xd50323bf,
@@ -125,6 +129,7 @@ struct eh_frame {
 			u8	data_alignment_factor;
 			u8	return_address_register;
 			u8	augmentation_data_size;
+			u8	fde_pointer_format;
 		};
 
 		struct { // FDE
@@ -132,11 +137,18 @@ struct eh_frame {
 			s32	range;
 			u8	opcodes[];
 		};
+
+		struct { // FDE
+			s64	initial_loc64;
+			s64	range64;
+			u8	opcodes64[];
+		};
 	};
 };
 
 static int scs_handle_fde_frame(const struct eh_frame *frame,
 				int code_alignment_factor,
+				bool use_sdata8,
 				bool dry_run)
 {
 	int size = frame->size - offsetof(struct eh_frame, opcodes) + 4;
@@ -144,6 +156,12 @@ static int scs_handle_fde_frame(const struct eh_frame *frame,
 	const u8 *opcode = frame->opcodes;
 	int l;
 
+	if (use_sdata8) {
+		loc = (u64)&frame->initial_loc64 + frame->initial_loc64;
+		opcode = frame->opcodes64;
+		size -= 8;
+	}
+
 	// assume single byte uleb128_t for augmentation data size
 	if (*opcode & BIT(7))
 		return EDYNSCS_INVALID_FDE_AUGM_DATA_SIZE;
@@ -210,6 +228,7 @@ static int scs_handle_fde_frame(const struct eh_frame *frame,
 int scs_patch(const u8 eh_frame[], int size)
 {
 	int code_alignment_factor = 1;
+	bool fde_use_sdata8 = false;
 	const u8 *p = eh_frame;
 
 	while (size > 4) {
@@ -245,13 +264,24 @@ int scs_patch(const u8 eh_frame[], int size)
 				return EDYNSCS_INVALID_CIE_HEADER;
 
 			code_alignment_factor = frame->code_alignment_factor;
+
+			switch (frame->fde_pointer_format) {
+			case DW_EH_PE_pcrel | DW_EH_PE_sdata4:
+				fde_use_sdata8 = false;
+				break;
+			case DW_EH_PE_pcrel | DW_EH_PE_sdata8:
+				fde_use_sdata8 = true;
+				break;
+			default:
+				return EDYNSCS_INVALID_CIE_SDATA_SIZE;
+			}
 		} else {
 			ret = scs_handle_fde_frame(frame, code_alignment_factor,
-						   true);
+						   fde_use_sdata8, true);
 			if (ret)
 				return ret;
 			scs_handle_fde_frame(frame, code_alignment_factor,
-					     false);
+					     fde_use_sdata8, false);
 		}
 
 		p += sizeof(frame->size) + frame->size;
-- 
2.47.0.277.g8800431eea-goog

[PATCH 3/3] arm64/scs: Drop unused prototype __pi_scs_patch_vmlinux()

[Ard Biesheuvel]

From: Ard Biesheuvel <ardb@kernel.org>

The function scs_patch_vmlinux() was removed in the LPA2 boot code
refactoring so remove the declaration as well.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/arm64/include/asm/scs.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm64/include/asm/scs.h b/arch/arm64/include/asm/scs.h
index 934e9217cd74..a76f9b387a26 100644
--- a/arch/arm64/include/asm/scs.h
+++ b/arch/arm64/include/asm/scs.h
@@ -54,7 +54,6 @@ enum {
 };
 
 int __pi_scs_patch(const u8 eh_frame[], int size);
-asmlinkage void __pi_scs_patch_vmlinux(void);
 
 #endif /* __ASSEMBLY __ */
 
-- 
2.47.0.277.g8800431eea-goog

Re: [PATCH 0/3] arm64: Dynamic shadow call stack fixes

[Sami Tolvanen]

Hi Ard,

On Wed, Nov 6, 2024 at 6:55 PM Ard Biesheuvel <ardb+git@google.com> wrote:
>
> From: Ard Biesheuvel <ardb@kernel.org>
>
> When building modules using the large code model, the emitted DWARF
> metadata uses large fields for the code references too, and this is
> currently not being handled.
>
> The CIE header frame contains metadata that describes the size of these
> fields, and so this should be parsed and taken into account when
> processing the FDE frames (patch #2)
>
> But first, the handling of this CIE frame header needs to be fixed, as
> it currently [inadvertently] used hardcoded defaults for some values
> that happen to always match what Clang emits in practice (patch #1)
>
> Cc: Sami Tolvanen <samitolvanen@google.com>
> Cc: Kees Cook <kees@kernel.org>
> Cc: Nathan Chancellor <nathan@kernel.org>
>
> Ard Biesheuvel (3):
>   arm64/scs: Fix handling of DWARF augmentation data in CIE/FDE frames
>   arm64/scs: Deal with 64-bit relative offsets in FDE frames
>   arm64/scs: Drop unused prototype __pi_scs_patch_vmlinux()
>
>  arch/arm64/include/asm/scs.h     |  8 +-
>  arch/arm64/kernel/module.c       | 10 ++-
>  arch/arm64/kernel/pi/patch-scs.c | 93 ++++++++++++++------
>  3 files changed, 80 insertions(+), 31 deletions(-)

I was able to reproduce the issue by building modules with
-mcmodel=large, and I confirmed that this series fixes it. For the
series:

Reviewed-by: Sami Tolvanen <samitolvanen@google.com>
Tested-by: Sami Tolvanen <samitolvanen@google.com>

Sami

Re: [PATCH 0/3] arm64: Dynamic shadow call stack fixes

[Catalin Marinas]

On Wed, 06 Nov 2024 19:55:14 +0100, Ard Biesheuvel wrote:
> When building modules using the large code model, the emitted DWARF
> metadata uses large fields for the code references too, and this is
> currently not being handled.
> 
> The CIE header frame contains metadata that describes the size of these
> fields, and so this should be parsed and taken into account when
> processing the FDE frames (patch #2)
> 
> [...]

Applied to arm64 (for-next/scs), thanks!

[1/3] arm64/scs: Fix handling of DWARF augmentation data in CIE/FDE frames
      https://git.kernel.org/arm64/c/ccf54058f532
[2/3] arm64/scs: Deal with 64-bit relative offsets in FDE frames
      https://git.kernel.org/arm64/c/60de7a647fc5
[3/3] arm64/scs: Drop unused prototype __pi_scs_patch_vmlinux()
      https://git.kernel.org/arm64/c/47965a49a2c8

-- 
Catalin