Re: [RFC PATCH v1 09/25] unwind_user/eh_frame: Add support for reading .eh_frame section

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] `DW_CFA_restore` incorrectly clears register rules instead of restoring the CIE's initial rule.
- [High] Uninitialized usage of `fde->func_addr` when decoding an FDE's start address via `DW_EH_PE_funcrel`.
- [High] The `eh_frame_cfi_context` struct is partially initialized, exposing uninitialized stack memory via `cfa_offset`.
- [Low] Incompatible pointer type passed to `read_uleb128()` in `DW_CFA_offset` instruction parsing.
--

--- Patch [9]: [RFC PATCH v1 09/25] unwind_user/eh_frame: Add support for reading .eh_frame section ---

commit 7603506904ca3b69840f3790ec49f48b9d3d54fb
Author: Jens Remus [off-list ref]

unwind_user/eh_frame: Add support for reading .eh_frame section

This patch adds an eh_frame_find interface to parse DWARF Call Frame
Information from the .eh_frame section for unwinding user space stacks. It
reads Common Information Entries, Frame Description Entries, and evaluates
DWARF CFI instructions to restore registers.

[ ... ]

 static __always_inline int read_encoded_pointer(struct eh_frame_section *sec,
+                                               struct eh_frame_fde *fde,
                                                unsigned long *addr,
                                                unsigned long end,
                                                u8 encoding,
                                                unsigned long *value)
 {

[ ... ]

+	case DW_EH_PE_funcrel:
+		if (!fde)
+			return -EINVAL;
+		result = fde->func_addr;
+		break;

[ ... ]

+static __always_inline int __read_fde(struct eh_frame_section *sec,
+				      unsigned long fde_addr,
+				      struct eh_frame_fde *fde)
+{

[ ... ]

+	scoped_user_read_access_size(fde_ptr, 4 + length, Efault) {
+		/* Read PC begin (function start address) */
+		ret = read_encoded_pointer(sec, fde, &cur, end, fde->cie.fde_addr_enc, &func_addr);

[Severity: High]
Does this code use an uninitialized value for the function address?

If the user-space eh_frame section specifies the DW_EH_PE_funcrel
encoding for fde_addr_enc, read_encoded_pointer() will attempt to evaluate
fde->func_addr.

However, fde->func_addr is not populated until the end of __read_fde(),
after the function address has already been parsed. Could this allow a
crafted eh_frame to use uninitialized kernel stack memory in its
address calculation?

+		if (ret)
+			return ret;
+		if (func_addr < sec->text_start || func_addr >= sec->text_end)
+			return -EINVAL;

[ ... ]

+	fde->func_addr		= func_addr;

[ ... ]

+static __always_inline int __do_cfi_insn(struct eh_frame_section *sec,
+					 struct eh_frame_fde *fde,
+					 unsigned long *cur_ptr,
+					 unsigned long end,
+					 unsigned long *ip_ptr,
+					 unsigned long target_ip,
+					 struct eh_frame_cfi_context *ctx)
+{

[ ... ]

+	case DW_CFA_offset: {
+		u8 reg = DW_CFA_operand(opcode);
+		long offset;
+		int idx;
+
+		ret = read_uleb128(&cur, end, &offset);

[Severity: Low]
Is the offset variable the correct type here?

The read_uleb128() function expects a pointer to an unsigned long, but
offset is defined as a signed long. This creates an incompatible pointer
type warning during the build.

+		if (ret)
+			return ret;
+		offset *= fde->cie.data_align;

[ ... ]

+	case DW_CFA_restore: {
+		u8 reg = DW_CFA_operand(opcode);
+		int idx;
+
+		idx = reg_to_index(reg);
+		if (idx >= 0)
+			ctx->state.reg_rule[idx] = REG_UNDEFINED_IMPLICIT;
+		break;
+	}

[Severity: High]
Does this instruction handler violate the DWARF specification?

The DWARF standard requires DW_CFA_restore to change the rule for the
indicated register back to the rule assigned by the initial instructions in
the CIE. By unconditionally setting it to REG_UNDEFINED_IMPLICIT instead
of restoring the CIE initial rule, this could corrupt the register recovery
state and cause unwinding to fail.

[ ... ]

+		case DW_CFA_def_cfa_register: {
+			unsigned long reg;
+
+			ret = read_uleb128(&cur, end, &reg);
+			if (ret)
+				return ret;
+
+			ctx->state.cfa_rule = CFA_REG_OFFSET;
+			ctx->state.cfa_regnum = reg;
+			break;
+		}

[ ... ]

+static __always_inline int __find_frame_row(struct eh_frame_section *sec,
+					    struct eh_frame_fde *fde,
+					    unsigned long ip,
+					    struct unwind_user_frame *frame)
+{
+	struct eh_frame_cfi_context ctx;
+	int ret;
+
+	/* TODO: Signal frame - not supported yet */
+	if (fde->cie.signal_frame)
+		return -EOPNOTSUPP;
+
+	ret = __do_cfi_program(sec, fde, ip, &ctx);

[Severity: High]
Can this leak kernel stack memory?

The eh_frame_cfi_context ctx structure is allocated on the stack without
being zero-initialized. In __do_cfi_program(), some fields are manually
initialized, but cfa_offset is omitted.

If an attacker provides a crafted eh_frame using DW_CFA_def_cfa_register
as the first state-modifying instruction, it changes cfa_rule to
CFA_REG_OFFSET without setting cfa_offset. The uninitialized kernel stack
garbage in ctx.state.cfa_offset is then subsequently copied into
frame->cfa.offset, allowing it to be used as an offset to read memory.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260818144954.2320378-1-jremus@linux.ibm.com?part=9