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 [off-list ref]
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.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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)
+{
[ ... ]
quoted hunk ↗ jump to hunk
+ 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?
quoted hunk ↗ jump to hunk
+ 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.
quoted hunk ↗ jump to hunk
+ 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/20260821195259.2688377-1-jremus@linux.ibm.com?part=14