Re: [RFC PATCH v2 12/24] unwind_user/eh_frame: Remove .eh_frame[_hdr] section on detected corruption

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