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

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Incorrect pointer size assumption for 32-bit compat tasks using DW_EH_PE_absptr encoding.
- [High] Bypassing speculative execution pointer masking by using an unmasked pointer within a `scoped_user_read_access_size` block.
- [Low] Mixing goto-based cleanup labels and scoped_guard declarations in the same function.
--

--- Patch [7]: [RFC PATCH v2 07/24] unwind_user/eh_frame: Add support for reading .eh_frame_hdr section ---
commit d29ad7450daa52f7e2ed07991186d7ccf95beadb
Author: Jens Remus <[email protected]>

unwind_user/eh_frame: Add support for reading .eh_frame_hdr section
    
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
> new file mode 100644
> index 0000000000000..f25a0923e315b
> --- /dev/null
> +++ b/kernel/unwind/eh_frame.c
> @@ -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:
> +		return sizeof(unsigned long);

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

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;
> +		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;
> +	unsigned long cur = sec->eh_frame_hdr_start, end = sec->eh_frame_hdr_end;

[ ... ]

> +	scoped_user_read_access_size(eh_frame_hdr, end - sec->eh_frame_hdr_start,
> +				     Efault) {
> +		/* Read version */
> +		UNSAFE_GET_USER_INC(version, cur, end, Efault);

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

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.

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