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

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] DW_CFA_restore and DW_CFA_restore_extended fail to restore the complete register rule state (offset and register number).
- [High] Bypass of pointer masking mitigation inside scoped_user_read_access_size blocks leading to a Spectre V1 vulnerability.
- [Medium] Integer overflow during instruction pointer advancement in DW_CFA_advance_loc4.
--

--- Patch [9]: [RFC PATCH v2 09/24] unwind_user/eh_frame: Add support for reading .eh_frame section ---
commit cd03be3d9017de830eb9301ff0da6dd6d08e69be
Author: Jens Remus <[email protected]>

unwind_user/eh_frame: Add support for reading .eh_frame section
    
In preparation for using .eh_frame to unwind user space stacks, add an
eh_frame_find() interface for finding the .eh_frame[_hdr] information
associated with a given text address.

> diff --git a/kernel/unwind/eh_frame.c b/kernel/unwind/eh_frame.c
> index 8cf19c5298fde..21f40805b417f 100644
> --- a/kernel/unwind/eh_frame.c
> +++ b/kernel/unwind/eh_frame.c

[ ... ]

> +static __always_inline int __read_cie(struct eh_frame_section *sec,
> +				      unsigned long cie_addr,
> +				      struct eh_frame_cie *cie)
> +{
> +	void __user *cie_ptr = (void __user *)cie_addr;
> +	unsigned long cur = cie_addr, end;

[ ... ]

> +	scoped_user_read_access_size(cie_ptr, 4 + length, Efault) {
> +		/* Read CIE_ID (must be 0 for CIE; FDE otherwise) */
> +		UNSAFE_GET_USER_INC(cie_id, cur, end, Efault);

[Severity: High]
Are we bypassing the pointer masking mitigation here? 

The scoped_user_read_access_size() macro shadows the passed pointer (cie_ptr)
with a masked version to prevent speculative kernel accesses. However, the
memory read via UNSAFE_GET_USER_INC() uses the separate unmasked cur variable.

Could this allow speculative out-of-bounds reads to access kernel memory?

> +		if (cie_id != EH_FRAME_CIE_ID)
> +			return -EINVAL;

[ ... ]

> +static __always_inline int __read_fde(struct eh_frame_section *sec,
> +				      unsigned long fde_addr,
> +				      struct eh_frame_fde *fde)
> +{
> +	void __user *fde_ptr = (void __user *)fde_addr;
> +	unsigned long cur = fde_addr, end;

[ ... ]

> +	scoped_user_read_access_size(fde_ptr, 4 + length, Efault) {
> +		/* Read CIE pointer (offset from current position) */
> +		UNSAFE_GET_USER_INC(cie_offset, cur, end, Efault);

[Severity: High]
Is this susceptible to the same pointer masking bypass as __read_cie()?

The unmasked cur variable is being used here instead of the masked fde_ptr.

> +		cie_addr = cur - 4 - cie_offset;
> +		if (cie_addr + EH_FRAME_CIE_MIN_LENGTH > fde_addr)
> +			return -EINVAL;
> +		if (cie_addr < sec->eh_frame_start)
> +			return -EINVAL;
> +	}

[ ... ]

> +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_restore: {
> +		u8 reg = DW_CFA_operand(opcode);
> +		int idx;
> +
> +		if (ctx->cie)
> +			return -EINVAL;
> +
> +		idx = reg_to_index(reg);
> +		if (idx >= 0)
> +			ctx->state.reg_rule[idx] = ctx->cie_state.reg_rule[idx];
> +		break;
> +	}

[Severity: High]
Does this properly restore the complete register state?

It appears only the rule type is restored from ctx->cie_state, but the
offset (reg_offset) and register number (reg_regnum) are not restored and
will remain at their previously modified values.

[ ... ]

> +		case DW_CFA_advance_loc4: {
> +			unsigned long offset;
> +			u32 delta;
> +
> +			UNSAFE_GET_USER_INC(delta, cur, end, Efault);
> +			offset = delta * fde->cie.code_align;

[Severity: Medium]
Can this calculation overflow?

The multiplication of delta (a u32) and fde->cie.code_align (an unsigned int)
evaluates as a 32-bit unsigned integer before being assigned to the 64-bit
unsigned long offset.

If the product exceeds 32 bits, the high bits would be truncated, which might
cause the unwinder to desync from the actual PC.

> +			ip += offset;
> +			break;
> +		}

[ ... ]

> +		case DW_CFA_restore_extended: {
> +			unsigned long reg;
> +			int idx;
> +
> +			if (ctx->cie)
> +				return -EINVAL;
> +
> +			ret = read_uleb128(&cur, end, &reg);
> +			if (ret)
> +				return ret;
> +
> +			idx = reg_to_index(reg);
> +			if (idx >= 0)
> +				ctx->state.reg_rule[idx] = ctx->cie_state.reg_rule[idx];
> +			break;
> +		}

[Severity: High]
This seems to have the same register state restoration issue as DW_CFA_restore.
Should reg_offset and reg_regnum also be restored here alongside the rule type?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=9