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, ®); > + 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
