Re: [RFC PATCH v2 24/24] unwind_user/eh_frame: Add prctl() interface for (un)registering .eh_frame_hdr sections

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

New issues:
- [High] The strict `size != sizeof(data)` check completely breaks UAPI backward and forward compatibility for extensible structs.
- [Medium] Silent truncation of the 64-bit `eh_frame_hdr_start` address to a 32-bit `unsigned long` during unregistration on 32-bit architectures.

Pre-existing issues:
- [High] If `mas_erase` fails due to an out-of-memory condition, it returns the target entry but leaves it in the maple tree. `__eh_frame_remove_section` fails to check `mas_is_err(mas)`, freeing the entry while it remains accessible in the tree.
- [High] The unregister prctl triggers an unbounded iteration over the maple tree while holding a spinlock, allowing an unprivileged user to cause a CPU soft lockup.
--

--- Patch [24]: [RFC PATCH v2 24/24] unwind_user/eh_frame: Add prctl() interface for (un)registering .eh_frame_hdr sections ---
commit 51adfd3b1e2795e1bc35c760119a8842b4e76983
Author: Jens Remus <[email protected]>

unwind_user/eh_frame: Add prctl() interface for (un)registering .eh_frame_hdr sections

This commit adds prctl interfaces to register and unregister .eh_frame_hdr
sections. It allows dynamic linkers to notify the kernel about the location
of these sections using a new eh_frame_setup structure.

Link: https://lore.kernel.org/all/[email protected]/

> 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

[ ... ]

> @@ -1609,3 +1610,48 @@ void eh_frame_free_mm(struct mm_struct *mm)
>  
>  	mtree_destroy(&mm->eh_frame_mt);
>  }
> +
> +int eh_frame_register(struct eh_frame_setup __user *user_data, __kernel_size_t size)
> +{
> +	struct eh_frame_setup data;
> +	unsigned long eh_frame_hdr_end, text_end;
> +
> +	if (!user_data || !size)
> +		return -EINVAL;
> +
> +	if (size != sizeof(data))
> +		return -EINVAL;

[Severity: High]
Does this strict size check prevent future UAPI extensions? Since this
is a new UAPI structure, rejecting sizes that do not exactly match the
kernel's structure size prevents extending the structure later without
breaking backward compatibility. Would it be better to use
copy_struct_from_user() here to handle size mismatches by verifying
trailing zeroes?

> +
> +	if (copy_from_user(&data, user_data, sizeof(data)))
> +		return -EFAULT;
> +
> +	if (check_add_overflow(data.eh_frame_hdr_start, data.eh_frame_hdr_size,
> +			       &eh_frame_hdr_end))
> +		return -EINVAL;
> +
> +	if (check_add_overflow(data.text_start, data.text_size, &text_end))
> +		return -EINVAL;
> +
> +	return eh_frame_add_section(data.eh_frame_hdr_start, eh_frame_hdr_end,
> +				    data.text_start, text_end);
> +}
> +
> +int eh_frame_unregister(struct eh_frame_setup __user *user_data, __kernel_size_t size)
> +{
> +	struct eh_frame_setup data;
> +
> +	if (!user_data || !size)
> +		return -EINVAL;
> +
> +	if (size != sizeof(data))
> +		return -EINVAL;

[Severity: High]
Is this strict size check similarly going to cause extensibility issues
for the unregister path? Like the register function, could this use
copy_struct_from_user() to safely allow future modifications?

> +
> +	if (copy_from_user(&data, user_data, sizeof(data)))
> +		return -EFAULT;
> +
> +	/* Unregister only uses eh_frame_hdr_start */
> +	if (data.eh_frame_hdr_size || data.text_start || data.text_size)
> +		return -EINVAL;
> +
> +	return eh_frame_remove_section(data.eh_frame_hdr_start);

[Severity: Medium]
Will data.eh_frame_hdr_start be silently truncated on 32-bit architectures?
The user provides a 64-bit __u64 address, but eh_frame_remove_section()
expects an unsigned long. If the upper 32 bits are set by userspace on a
32-bit architecture, this could truncate into a valid 32-bit address without
any bounds validation.


[Severity: High]
This isn't a bug introduced by this patch, but does exposing this function
to userspace via prctl allow an unprivileged user to cause a CPU soft lockup?

Looking at eh_frame_remove_section(), it performs an unbounded iteration over
the maple tree while holding a spinlock:

kernel/unwind/eh_frame.c:eh_frame_remove_section() {
    ...
    mtree_lock(&mm->eh_frame_mt);
    mas_for_each(&mas, sec, ULONG_MAX) {
        if (sec->eh_frame_hdr_start == eh_frame_hdr_start) {
            found = true;
            ret |= __eh_frame_remove_section(&mas, sec);
        }
    }
    mtree_unlock(&mm->eh_frame_mt);
    ...
}

Could an attacker register a massive number of non-overlapping sections and
then trigger a long unbounded loop with preemption disabled?


[Severity: High]
This is also a pre-existing issue, but the new prctl makes it trivially
reachable by unprivileged userspace, which could lead to a use-after-free.

When __eh_frame_remove_section() attempts to remove a section:

kernel/unwind/eh_frame.c:__eh_frame_remove_section() {
    ...
    if (mas_erase(mas) != sec) {
        dbg_sec("mas_erase failed: text=%lx\n", sec->text_start);
        return -EINVAL;
    }

    call_srcu(&eh_frame_srcu, &sec->rcu, eh_frame_free_srcu);
    ...
}

If mas_erase() fails due to an out-of-memory condition, it can set an error
state and return the target entry without removing it from the tree. Because
the code does not check mas_is_err(mas), it frees the entry while it remains
accessible in the maple tree. Should this verify mas_is_err() before freeing
the section?

> +}

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