Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues: - [High] A Time-of-Check to Time-of-Use (TOCTOU) race condition allows the kernel to erroneously unregister a valid, newly loaded .eh_frame section due to address reuse. - [High] The unwinder conflates actual memory corruption with kernel-imposed safety limits, permanently disabling unwinding for valid libraries. Pre-existing issues: - [Critical] Unchecked error from mas_erase() leads to freeing a maple tree section while it remains active in the tree, causing a Use-After-Free (UAF). - [High] Lock drop and reacquire inside mas_erase() during mas_for_each() iteration causes a TOCTOU race and memory leak. -- --- Patch [12]: [RFC PATCH v2 12/24] unwind_user/eh_frame: Remove .eh_frame[_hdr] section on detected corruption --- commit 14251b7471659003e9517d814ad704b284ed18df Author: Jens Remus <[email protected]> unwind_user/eh_frame: Remove .eh_frame[_hdr] section on detected corruption This commit adds logic to unregister .eh_frame or .eh_frame_hdr sections on demand when corruption is detected during lookup. This prevents repeated attempts to parse bad unwind sections by removing them immediately upon an error return. > diff --git a/kernel/unwind/eh_frame.c b/kernel/unwind/eh_frame.c > index 21f40805b417f..327849ea4a692 100644 > --- a/kernel/unwind/eh_frame.c > +++ b/kernel/unwind/eh_frame.c > @@ -1202,6 +1202,15 @@ int eh_frame_find(unsigned long ip, struct > unwind_user_frame *frame) > if (!ret) > ret = __find_frame_row(sec, &fde, ip, frame); > > + /* > + * Unregister .eh_frame[_hdr] in case of an error, > + * e.g. EINVAL (corrupted) or EFAULT (inaccessible). > + * Keep if ENOENT (not found) or EOPNOTSUPP (unsupported CFI). > + */ > + if (ret && (ret != -ENOENT && ret != -EOPNOTSUPP)) > + if (eh_frame_remove_section(sec->eh_frame_hdr_start)) > + dbg("eh_frame_remove_section() failed\n"); > + [Severity: High] Does this introduce a race condition with memory unmapping and mapping? If a thread is delayed after retrieving sec in eh_frame_find(), another thread could unmap the library and map a new library at the exact same virtual address, registering it with the same eh_frame_hdr_start. When the first thread resumes and fails to parse against the new memory, passing the virtual address sec->eh_frame_hdr_start to eh_frame_remove_section() seems like it would unintentionally remove the newly loaded, valid library's section. [Severity: High] Is it safe to assume -EINVAL always means corruption? Looking at __do_cfi_program(), it also returns -EINVAL when kernel limits are hit, such as exceeding EH_FRAME_CFI_INSN_LIMIT or EH_FRAME_MAX_CODE_ALIGN. If a valid library simply has a complex function that hits these limits, wouldn't this code globally and permanently disable unwinding for it by removing its section? [Severity: High] This is a pre-existing issue, but looking at eh_frame_remove_section() which is called here, is there a memory leak if mas_erase() drops the lock? eh_frame_remove_section() holds mtree_lock and iterates via mas_for_each(). It calls __eh_frame_remove_section(), which calls mas_erase(). If mas_erase() needs to allocate memory via mas_nomem(mas, GFP_KERNEL), it can drop the tree lock. If a concurrent thread inserts a new section covering the same range during this unlocked window, mas_erase() retries, erases the new section, and returns it. __eh_frame_remove_section() then sees the returned entry doesn't match sec, returns -EINVAL, and skips call_srcu(), potentially leaking the new section. [Severity: Critical] This is a pre-existing issue, but looking at __eh_frame_remove_section() called by eh_frame_remove_section(), can a memory allocation failure lead to a use-after-free? If mas_nomem() fails to allocate memory during mas_erase(), it sets an error state and aborts without removing the node, but still returns the original sec: __eh_frame_remove_section() { ... if (mas_erase(mas) != sec) { return -EINVAL; } call_srcu(&eh_frame_srcu, &sec->rcu, eh_frame_free_srcu); ... } Because __eh_frame_remove_section() only checks if the return value does not equal sec, it misses the error state completely. It then calls call_srcu() to free the section while the node remains active in the maple tree. Could subsequent calls to eh_frame_find() load and access this freed memory? > return ret; > } > -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=12
