Re: [RFC PATCH v2 17/24] unwind_user/eh_frame: Add support for DWARF expressions

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Unaligned stack buffer passed to unsafe_copy_from_user() causes undefined behavior
--

--- Patch [17]: [RFC PATCH v2 17/24] unwind_user/eh_frame: Add support for DWARF expressions ---
commit 38e0b8657cdb2bd1e95dd3bc74b1aa71e0063c9a
Author: Jens Remus <[email protected]>

    unwind_user/eh_frame: Add support for DWARF expressions
    
    Enable architectures to handle DWARF expressions in DWARF CFI
    instructions DW_CFA_def_cfa_expression, DW_CFA_expression, and
    DW_CFA_val_expression. Limit the maximum expression length to a
    reasonable size, while enabling architectures to override the
    limit.

> diff --git a/kernel/unwind/eh_frame.c b/kernel/unwind/eh_frame.c
> index d185187b0c34e..a98804dbe5886 100644
> --- a/kernel/unwind/eh_frame.c
> +++ b/kernel/unwind/eh_frame.c
> @@ -803,6 +772,27 @@ static __always_inline int __do_cfi_insn(struct eh_frame_section *sec,
>  			break;
>  		}
>  
> +		case DW_CFA_def_cfa_expression: {
> +			unsigned long expr_len;
> +			char expr[EH_FRAME_MAX_EXPRESSION_LENGTH];
> +
> +			ret = read_uleb128(&cur, end, &expr_len);
> +			if (ret)
> +				return ret;
> +
> +			if (cur + expr_len < cur || cur + expr_len > end)
> +				return -EINVAL;
> +
> +			if (expr_len > sizeof(expr))
> +				return -EOPNOTSUPP;
> +			unsafe_copy_from_user(&expr, (void __user *)cur, expr_len, Efault);

[Severity: Low]
Does this unaligned stack buffer passed to unsafe_copy_from_user() cause
undefined behavior?

Passing a 1-byte aligned char array to unsafe_copy_from_user() violates C
alignment rules. In architecture-specific implementations, the macro
explicitly casts the destination pointer to larger integer types (e.g.,
u64 *) to perform optimized word-sized memory assignments. Dereferencing
an unaligned pointer is Undefined Behavior in C.

While x86 hardware gracefully handles unaligned memory accesses, the
compiler's optimizer is legally allowed to assume u64 * is 8-byte aligned
and could emit trapping instructions. Furthermore, if this generic .eh_frame
code is later enabled on architectures with strict hardware alignment
enforcement, it will immediately trigger alignment exceptions.

Could this buffer be declared with __aligned(8) or as an array of u64?

> +			ret = eh_frame_do_def_cfa_expression(expr, expr_len, target_ip, &ctx->state);
> +			if (ret)
> +				return ret;
> +			cur += expr_len;
> +			break;
> +		}
> +

[ ... ]

> @@ -970,12 +960,22 @@ static __always_inline int __do_cfi_insn(struct eh_frame_section *sec,
>  			if (ret)
>  				return ret;
>  
> -			if (cur + expr_len > end)
> +			if (cur + expr_len < cur || cur + expr_len > end)
>  				return -EINVAL;
>  
> -			if (reg == EH_FRAME_REG_SP || reg == EH_FRAME_REG_FP || reg == EH_FRAME_REG_RA)
> -				return -EOPNOTSUPP;
> +			if (reg == EH_FRAME_REG_SP || reg == EH_FRAME_REG_FP || reg == EH_FRAME_REG_RA) {
> +				char expr[EH_FRAME_MAX_EXPRESSION_LENGTH];
>  
> +				if (expr_len > sizeof(expr))
> +					return -EOPNOTSUPP;
> +				unsafe_copy_from_user(&expr, (void __user *)cur, expr_len, Efault);

[Severity: Low]
Is this the same alignment issue as above?

Could this buffer also be declared with __aligned(8) or as an array of u64?

> +				if (opcode == DW_CFA_expression)
> +					ret = eh_frame_do_expression(reg, expr, expr_len, target_ip, &ctx->state);
> +				else
> +					ret = eh_frame_do_val_expression(reg, expr, expr_len, target_ip, &ctx->state);
> +				if (ret)
> +					return ret;
> +			}
>  			cur += expr_len;
>  			break;
>  		}

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