Re: [RFC PATCH v1 14/25] unwind_user/eh_frame: Add .eh_frame[_hdr] validation option

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:

New issues:
- [High] Calling printk() inside a scoped_user_read_access_size block causes a UACCESS rule violation.
- [High] Infinite loop leading to local Denial of Service due to a 32-bit loop counter overflowing against a 64-bit upper bound.
- [Medium] CPU soft lockup due to unbounded loop over potentially millions of user-controlled entries without yielding.
- [Low] Compilation failure due to referencing a non-existent struct member `has_binary_search_table`.
- [Low] Spelling error in Kconfig help text ('entrire' instead of 'entire').

Pre-existing issues:
- [High] Information leak of uninitialized kernel stack memory to userspace via the unwinder.
--

--- Patch [14]: [RFC PATCH v1 14/25] unwind_user/eh_frame: Add .eh_frame[_hdr] validation option ---
commit e7ce89dcff055122fac1a936bcef539a42d26943
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/arch/Kconfig b/arch/Kconfig
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -490,6 +490,28 @@ config HAVE_UNWIND_USER_EH_FRAME
>  	bool
>  	select UNWIND_USER
>  
> +config EH_FRAME_VALIDATION
> +	bool "Enable .eh_frame[_hdr] section debugging"
> +	depends on HAVE_UNWIND_USER_EH_FRAME
> +	depends on DYNAMIC_DEBUG
> +	help
> +	  When adding an .eh_frame_hdr section for a test, validate the
> +	  entire section and its referenced entrire .eh_frame section

[Severity: Low]
Is there a typo here? (entrire -> entire)

> +	  immediately rather than on demand.

[ ... ]

> diff --git a/kernel/unwind/eh_frame.c b/kernel/unwind/eh_frame.c
> --- a/kernel/unwind/eh_frame.c
> +++ b/kernel/unwind/eh_frame.c

[ ... ]

> @@ -1107,11 +1107,11 @@
>  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;

[Severity: High]
This is a pre-existing issue, but can this leak uninitialized kernel stack
memory to userspace via the unwinder?

In __find_frame_row(), ctx is allocated on the kernel stack without
initialization. If an attacker provides a crafted .eh_frame starting with
DW_CFA_def_cfa_register, __do_cfi_program() sets cfa_rule = CFA_REG_OFFSET but
leaves ctx.state.cfa_offset uninitialized. This uninitialized kernel stack
value is assigned to frame->cfa.offset and used by the unwinder to compute the
user stack pointer.

[ ... ]

> @@ -1163,6 +1163,95 @@ 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)
> +{
> +	void __user *table_start_ptr;
> +	unsigned long table_size;
> +	u8 table_enc;
> +	int entry_size;
> +	unsigned long prev_func_addr;
> +	unsigned int i;
> +
> +	if (!sec->has_binary_search_table)

[Severity: Low]
Does this cause a build failure? It appears has_binary_search_table was never
added to struct eh_frame_section.

> +		return 0;
> +
> +	if (!sec->fde_count) {
> +		dbg_sec(".eh_frame_hdr: invalid FDE count\n");
> +		return -EINVAL;
> +	}
> +
> +	table_enc = sec->binary_search_table_enc;
> +	entry_size = 2 * encoded_pointer_size(table_enc);
> +	if (!entry_size) {
> +		dbg_sec(".eh_frame_hdr: invalid binary search table entry size\n");
> +		return -EINVAL;
> +	}
> +	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]
Can this loop become infinite and cause a hard lockup?

The loop counter i is a 32-bit unsigned int, while sec->fde_count is a 64-bit
unsigned long. If sec->fde_count exceeds UINT_MAX, i will wrap around to 0, and
the condition i < sec->fde_count will always evaluate to true.

[Severity: Medium]
Additionally, can this unbounded loop over potentially millions of entries
cause a CPU soft lockup or RCU stall? 

This loop executes entirely without a cond_resched() call, allowing a local
user to load a moderately large ELF file and hold the CPU uninterrupted for
seconds.

> +		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, NULL, &cur,
> +						   sec->binary_search_table_end,
> +						   table_enc, &func_addr);
> +			if (ret) {
> +				dbg_sec_ehfh(cur, "table[%u]: failed to read function start address\n", i);

[Severity: High]
Does calling dbg_sec_ehfh() inside scoped_user_read_access_size violate UACCESS
rules?

The macro scoped_user_read_access_size enables UACCESS. dbg_sec_ehfh expands to
pr_debug, which calls printk. Calling complex or sleepable functions like
printk with UACCESS enabled can trigger page faults, take locks, or schedule,
potentially leading to kernel oopses or panics.

> +				return ret;
> +			}

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