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
