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

[Jens Remus <jremus@linux.ibm.com>]

On 8/18/2026 5:05 PM, sashiko-bot@kernel.org wrote:

>> diff --git 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?

Valid finding.  I'll remove support for DW_EH_PE_funcrel, as it cannot
be used for the PC begin (function start address) nor PC range (function
size).

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

		unsigned long _offset;

>> +		long offset;
>> +		int idx;
>> +
>> +		ret = read_uleb128(&cur, end, &offset);

		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;

		offset = _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.

I agree.  But libgcc's unwinder does handle DW_CFA_restore and
DW_CFA_restore_extended like that.  There is even a comment in
libgcc/unwind-dw2-execute_cfa.h:

	/* FIXME, this is wrong; the CIE might have said that the
	   register was saved somewhere.  */

I will implement and check whether saving the CIE state and restoring to
that (as specified) still produces correct results.  I will also implement
support for DW_CFA_restore_extended.

> 
> [ ... ]
>> +		case DW_CFA_def_cfa_register: {
>> +			unsigned long reg;
>> +

			if (ctx->state.cfa_rule != CFA_REG_OFFSET)
				return -EINVAL;

Likewise for DW_CFA_def_cfa_offset, as the DWARF specification states
for both:

"This operation is valid only if the current CFA rule is defined to use
a register and offset."

>> +			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.
> 

Valid.  Fixed by checking that the CFA rule is register with offset.

Regards,
Jens
-- 
Jens Remus
Linux on Z Development (D3303)
jremus@de.ibm.com / jremus@linux.ibm.com

IBM Deutschland Research & Development GmbH; Vorsitzender des Aufsichtsrats: Wolfgang Wendt; Geschäftsführung: David Faller; Sitz der Gesellschaft: Ehningen; Registergericht: Amtsgericht Stuttgart, HRB 243294
IBM Data Privacy Statement: https://www.ibm.com/privacy/