Re: [RFC PATCH v2 07/24] unwind_user/eh_frame: Add support for reading .eh_frame_hdr section

[Jens Remus <jremus@linux.ibm.com>]

On 8/21/2026 10:06 PM, sashiko-bot@kernel.org wrote:

> In preparation for unwinding user space stacks with .eh_frame, add
> basic unwind user eh_frame infrastructure and support for reading the
> .eh_frame_hdr section.

>> diff --git a/kernel/unwind/eh_frame.c b/kernel/unwind/eh_frame.c

#include <linux/sched/task_stack.h>
#include <linux/unwind_user.h>

>> @@ -0,0 +1,366 @@
> 
> [ ... ]
> 
>> +static __always_inline int encoded_pointer_size(u8 encoding)
>> +{
>> +	u8 format = DW_EH_PE_format(encoding);
>> +
>> +	switch (format) {
>> +	case DW_EH_PE_absptr:

		if (IS_ENABLED(CONFIG_COMPAT))
			return unwind_user_word_size(task_pt_regs(current));

>> +		return sizeof(unsigned long);
> 
> [Severity: High]
> Does hardcoding sizeof(unsigned long) here cause issues for 32-bit compat
> tasks?

The word size from struct unwind_user_state field word_size cannot be used
because it needs to be known at .eh_frame[_hdr] section registration time:

eh_frame_add_section() -> eh_frame_read_header() -> read_encoded_pointer()

Would it be ok to determine the word size each time the special case
DW_EH_PE_absptr is handled in encoded_pointer_size() and
read_encoded_pointer()?  See also below.

Also I need to do some testing whether those actually occur in .eh_frame.
If not it would be preferable to drop DW_EH_PE_absptr support.

> 
> If a 64-bit kernel is profiling a 32-bit application, DW_EH_PE_absptr is
> expected to match the application's native 4-byte pointer size. Using
> sizeof(unsigned long) means the kernel will read 8 bytes, which could
> consume too much data and misalign the current pointer for the remainder
> of the unwinding process.
> 
>> +	case DW_EH_PE_udata2:
>> +	case DW_EH_PE_sdata2:
>> +		return 2;
> 
> [ ... ]
> 
>> +static __always_inline int read_encoded_pointer(struct eh_frame_section *sec,
>> +						unsigned long *addr,
>> +						unsigned long end,
>> +						u8 encoding,
>> +						unsigned long *value)
>> +{
> 
> [ ... ]
> 
>> +	/* Read value based on format */
>> +	switch (format) {
>> +	case DW_EH_PE_absptr: {
>> +		unsigned long tmp;
>> +		UNSAFE_GET_USER_INC(tmp, cur, end, Efault);
>> +		result += tmp;

Replace above with:

		if (IS_ENABLED(CONFIG_COMPAT) &&
		    unwind_user_word_size(task_pt_regs(current)) == sizeof(int)) {
			unsigned int tmp32;
			UNSAFE_GET_USER_INC(tmp32, cur, end, Efault);
			result += tmp32;
		} else {
			unsigned long tmp;
			UNSAFE_GET_USER_INC(tmp, cur, end, Efault);
			result += tmp;
		}

>> +		break;
>> +	}
> 
> [Severity: High]
> Will this also read 8 bytes instead of 4 for a 32-bit task?
> 
> Similar to encoded_pointer_size(), this reads a full unsigned long for an
> absolute pointer. If this is a 64-bit kernel unwinding a 32-bit compat
> application, it seems this might read past the intended data boundaries
> and corrupt the address parsing.
> 
>> +	case DW_EH_PE_uleb128: {
>> +		unsigned long tmp;
> 
> [ ... ]
> 
>> +static int eh_frame_read_header(struct eh_frame_section *sec)
>> +{
>> +	struct mm_struct *mm = current->mm;
>> +	void __user *eh_frame_hdr = (void __user *)sec->eh_frame_hdr_start;

	size_t size = sec->eh_frame_hdr_end - sec->eh_frame_hdr_start;

>> +	unsigned long cur = sec->eh_frame_hdr_start, end = sec->eh_frame_hdr_end;

Move cur and end into scoped user access block below.

> 
> [ ... ]
> 
>> +	scoped_user_read_access_size(eh_frame_hdr, end - sec->eh_frame_hdr_start,
>> +				     Efault) {

	scoped_user_read_access_size(eh_frame_hdr, size, Efault) {
		unsigned long cur = (unsigned long)eh_frame_hdr;
		unsigned long end = (unsigned long)eh_frame_hdr + size;

>> +		/* Read version */
>> +		UNSAFE_GET_USER_INC(version, cur, end, Efault);

Then whenever cur or end are expected to be unmasked pointers they need
to be converted back:

-		table_start = cur;
+		table_start = sec->eh_frame_hdr_start + (cur - (unsigned long)eh_frame_hdr);

...

-		if (fde_count > (end - table_start) / entry_size)
+		if (fde_count > (sec->eh_frame_hdr_end - table_start) / entry_size)
			return -EINVAL;

> 
> [Severity: High]
> Does this bypass the speculative execution pointer masking provided by
> scoped_user_read_access_size()?

True.  I was not aware of that.  While above seems to resolve this at a
first glance there are some implications in read_encoded_pointer():

DW_EH_PE_datarel (relative to .eh_frame_hdr) and DW_EH_PE_textrel
(relative to .text) return an unmasked pointer, which is ok, as the
callers expect it to be an unmasked value.

DW_EH_PE_pcrel (relative to the field itself) also returns an unmasked
pointer and now requires the unmasked cursor pointer to be provided as
input as well.

> 
> The block masks the eh_frame_hdr pointer, but inside the block,
> UNSAFE_GET_USER_INC() reads from the unmasked unsigned long cur variable:
> 
> #define UNSAFE_GET_USER_INC(to, from, end, label)
> ...
> 	unsafe_get_user(__to, (typeof(to) __user *)from, label);
> 
> Since cur is passed to unsafe_get_user() without being masked, it appears
> this defeats the Spectre V1 mitigation for architectures relying on
> can_do_masked_user_access().
> 
>> +		if (version != 1)
>> +			return -EINVAL;

>> +int eh_frame_add_section(unsigned long eh_frame_hdr_start,
>> +			 unsigned long eh_frame_hdr_end,
>> +			 unsigned long text_start,
>> +			 unsigned long text_end)
>> +{
>> +	struct mm_struct *mm = current->mm;
>> +	struct eh_frame_section *sec;
>> +	int ret;
> 
> [ ... ]
> 
>> +	scoped_guard(mmap_read_lock, mm) {
>> +		struct vm_area_struct *eh_frame_hdr_vma, *text_vma;
>> +
>> +		eh_frame_hdr_vma = vma_lookup(mm, eh_frame_hdr_start);
> 
> [ ... ]
> 
>> +	ret = eh_frame_read_header(sec);
>> +	if (ret)
>> +		goto err_free;
>> +
>> +	/* TODO nowhere to store it yet - just free it and return an error */
>> +	ret = -ENOSYS;
>> +
>> +err_free:
>> +	free_section(sec);
>> +	return ret;
>> +}
> 
> [Severity: Low]
> This isn't a bug, but should this function avoid mixing scoped_guard() and
> goto-based error handling?
> 
> The kernel cleanup guidelines recommend either fully converting to
> scope-based cleanup (for example, using __free() for the sec allocation)
> or strictly using goto-based cleanups without scoped_guard(). Mixing them
> in the same function can create confusing ownership semantics.

Other than converting the code to use mmap_read_lock(mm) and
mmap_read_unlock(mm) I don't see how the section cleanup logic
could be converted to scope-based cleanup, as it must not be
freed if it gets inserted into the mm->eh_frame_mt (in a
subsequent patch).  Sashiko probably misses this as in this
incomplete state it looks as if it could be converted, as the
section is always freed.

Thanks and regards,
Jens
-- 
Jens Remus
Linux on Z Development (D3303)
jremus@de.ibm.com / jremus@linux.ibm.com

IBM Deutschland Research & Development GmbH; Vorsitzender des Aufsichtsrats: Wolfgang Wendt; Geschäftsführung: David Faller; Sitz der Gesellschaft: Ehningen; Registergericht: Amtsgericht Stuttgart, HRB 243294
IBM Data Privacy Statement: https://www.ibm.com/privacy/