Re: [RFC PATCH v1 16/25] unwind_user/eh_frame: Add linear .eh_frame search fallback

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] The fallback stub __find_fde_lsearch() returns 0 (success) instead of an error when CONFIG_EH_FRAME_LINEAR_SEARCH is disabled, causing the unwinder to consume an uninitialized stack variable.
- [High] Enabling CONFIG_EH_FRAME_LINEAR_SEARCH unconditionally triggers linear validation and causes out-of-bounds reads, permanently breaking unwinding for valid binaries lacking a zero terminator.
--

--- Patch [16]: [RFC PATCH v1 16/25] unwind_user/eh_frame: Add linear .eh_frame search fallback ---
commit 4fc15aee544b13c1c7ff56efb01c804156c21a55
Author: Jens Remus <[email protected]>

unwind_user/eh_frame: Add linear .eh_frame search fallback

This commit adds a linear search fallback for .eh_frame sections when
the .eh_frame_hdr lacks a binary search table. It also introduces validation
logic for the referenced .eh_frame section, guarded by a new config
option EH_FRAME_LINEAR_SEARCH.

> diff --git a/kernel/unwind/eh_frame.c b/kernel/unwind/eh_frame.c
> index 7f572d1711d3f..ac288cec8021f 100644
> --- a/kernel/unwind/eh_frame.c
> +++ b/kernel/unwind/eh_frame.c
[ ... ]
> @@ -590,6 +589,82 @@ static __always_inline int __find_fde(struct eh_frame_section *sec,
>  	return -EFAULT;
>  }
>  
> +#ifdef CONFIG_EH_FRAME_LINEAR_SEARCH
> +
> +static __always_inline int __find_fde_lsearch(struct eh_frame_section *sec,
> +					      unsigned long ip,
> +					      struct eh_frame_fde *fde)
> +{
> +	unsigned long start = sec->eh_frame_start;
> +	unsigned long vma_end = sec->eh_frame_vma_end;
> +	unsigned long cur;
> +	int ret;
> +
> +	/* Linear search through .eh_frame */
> +	cur = start;
> +	while (cur >= start && cur < vma_end) {

[Severity: High]
Since the section header only provides the start address, this loop iterates
all the way to vma_end. As noted in the commit message, many binaries lack
a zero terminator.

If an unresolved IP is scanned, will this read past the actual section
boundary into padding or other sections, return -EINVAL on garbage data, and
subsequently cause the caller to permanently remove the section?

[ ... ]
> +		cur = entry_end;
> +	}
> +
> +	return -ENOENT;
> +}
> +
> +#else /* !CONFIG_EH_FRAME_LINEAR_SEARCH */
> +
> +static __always_inline int __find_fde_lsearch(struct eh_frame_section *sec,
> +					      unsigned long ip,
> +					      struct eh_frame_fde *fde)
> +{
> +	return 0;
> +}

[Severity: Critical]
Does returning 0 here falsely indicate success when linear search is
disabled?

If a binary lacks a search table, __find_fde() will call this stub.
Returning 0 causes the caller, eh_frame_find(), to proceed with an
uninitialized fde stack variable:

kernel/unwind/eh_frame.c:eh_frame_find() {
    struct eh_frame_fde fde;
    ...
    ret = __find_fde(sec, ip, &fde);
    if (!ret)
        ret = __find_frame_row(sec, &fde, ip, frame);
    ...
}

Could this lead to corrupted stack traces or arbitrary memory reads by
consuming the uninitialized fde context? Should this return -ENOENT
instead?

[ ... ]
> @@ -1246,6 +1321,90 @@ static int eh_frame_validate_section(struct eh_frame_section *sec)
>  	return -EFAULT;
>  }
>  
> +static int eh_frame_validate_eh_frame(struct eh_frame_section *sec)
> +{
[ ... ]
> +static int eh_frame_validate_section(struct eh_frame_section *sec)
> +{
> +	int ret;
> +
> +	/*
> +	 * Validate .eh_frame_hdr binary search table
> +	 * (incl. all referenced FDE and CIE in .eh_frame).
> +	 */
> +	ret = eh_frame_validate_eh_frame_hdr(sec);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Validate .eh_frame CIE and FDE.  Skip if linear search
> +	 * is disabled, as many .eh_frame sections lack a zero
> +	 * terminator and the section end if unknown.
> +	 */
> +	if (IS_ENABLED(CONFIG_EH_FRAME_LINEAR_SEARCH)) {
> +		ret = eh_frame_validate_eh_frame(sec);
> +		if (ret)
> +			return ret;
> +	}

[Severity: High]
Does this unconditionally apply linear validation to all binaries when
CONFIG_EH_FRAME_LINEAR_SEARCH is enabled, even if they already have a valid
binary search table?

If the binary lacks a zero terminator, eh_frame_validate_eh_frame() will
read out of bounds up to vma_end, fail with -EINVAL, and reject the entire
section.

Will this permanently break unwinding for valid binaries that were
previously working fine via the binary search table?

> +
> +	return 0;
> +}

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