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 <[email protected]>

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.

> diff --git a/kernel/unwind/eh_frame.c b/kernel/unwind/eh_frame.c
> --- a/kernel/unwind/eh_frame.c
> +++ b/kernel/unwind/eh_frame.c
[ ... ]
>  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/[email protected]?part=9