Re: [RFC PATCH v2 14/24] unwind_user/eh_frame: Add .eh_frame[_hdr] validation option

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Calling printk (via dbg_sec and dbg_sec_ehfh) while user access (UACCESS) is enabled.
- [High] Missing cond_resched() in massive, unbounded .eh_frame validation loop.
- [High] Misuse of scoped_user_read_access_size completely bypasses pointer masking mitigations.
--

--- Patch [14]: [RFC PATCH v2 14/24] unwind_user/eh_frame: Add .eh_frame[_hdr] validation option ---
commit e134b713f5753b4c2aa710bb7ce4a784930e6ccd
Author: Jens Remus <[email protected]>

unwind_user/eh_frame: Add .eh_frame[_hdr] validation option

Add a debug feature to validate all .eh_frame[_hdr] sections when first
loading the file rather than on demand.

> diff --git a/kernel/unwind/eh_frame.c b/kernel/unwind/eh_frame.c
> index 96237a96d263e..b0be56e315080 100644
> --- a/kernel/unwind/eh_frame.c
> +++ b/kernel/unwind/eh_frame.c
[ ... ]
> @@ -1216,6 +1216,92 @@ int eh_frame_find(unsigned long ip, struct unwind_user_frame *frame)
>  	return ret;
>  }
>  
> +#ifdef CONFIG_EH_FRAME_VALIDATION
> +
> +static int eh_frame_validate_section(struct eh_frame_section *sec)
> +{
[ ... ]
> +	table_start_ptr = (void __user *)sec->binary_search_table_start;
> +	table_size = sec->binary_search_table_end - sec->binary_search_table_start;
> +
> +	for (i = 0; i < sec->fde_count; i++) {

[Severity: High]
Could this unbounded loop cause CPU stalls or trigger soft lockup warnings?

Since fde_count is controlled by the size of the mapped .eh_frame_hdr VMA,
it can be extremely large for maliciously crafted ELF files, potentially
stalling the CPU for seconds or minutes.

Because mmap_read_lock is released before this is called, would it be safer
to add a cond_resched() in this loop?

> +		struct eh_frame_fde fde;
> +		unsigned long cur;
> +		unsigned long func_addr, fde_addr;
> +		int ret;
> +
> +		cur = sec->binary_search_table_start + i * entry_size;
> +
> +		scoped_user_read_access_size(table_start_ptr, table_size, Efault) {
> +			/* Read function start address from table */
> +			ret = read_encoded_pointer(sec, &cur,
> +						   sec->binary_search_table_end,
> +						   table_enc, &func_addr);

[Severity: High]
Does this bypass the Spectre v1 mitigations provided by the scoped access
macro?

The scoped_user_read_access_size() macro shadows the table_start_ptr variable
to enforce usage of a masked pointer. However, the code ignores that masked
pointer and instead uses the unmasked cur pointer derived outside the scope:

    cur = sec->binary_search_table_start + i * entry_size;

When read_encoded_pointer() uses this unmasked pointer inside
unsafe_get_user(), the masking mitigations are bypassed, allowing potential
arbitrary speculative reads.

> +			if (ret) {
> +				dbg_sec_ehfh(cur, "table[%lu]: failed to read function start address\n", i);

[Severity: High]
Is it safe to call printk wrappers like dbg_sec_ehfh() or dbg_sec() while user
access (UACCESS) is enabled?

Calling complex functions like printk inside a scoped_user_read_access_size()
block can lead to deadlocks or kernel panics. It does internal locking, can
sleep, and may clobber architecture-specific user access state like the SMAP
AC flag.

> +				return ret;
> +			}
> +			if (i && func_addr <= prev_func_addr) {
> +				dbg_sec(".eh_frame_hdr: table[%lu]: not sorted\n", i);
> +				return -EINVAL;
> +			}

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