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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Integer overflow in binary search table size calculation allows bypassing `.eh_frame_hdr` bounds check.
- [Medium] The `read_encoded_pointer` function incorrectly overwrites the computed base address when handling the `DW_EH_PE_absptr` format combined with relative application flags.
--

--- Patch [7]: [RFC PATCH v1 07/25] unwind_user/eh_frame: Add support for reading .eh_frame_hdr section ---
commit 0ca4af16fa2ddbd53ffc639d7ba44b189726700c
Author: Jens Remus <[email protected]>

unwind_user/eh_frame: Add support for reading .eh_frame_hdr section

This commit adds basic infrastructure for reading the .eh_frame_hdr
section from userspace in preparation for .eh_frame based stack unwinding.

> diff --git a/kernel/unwind/eh_frame.c b/kernel/unwind/eh_frame.c
> new file mode 100644
> index 0000000000000..8d2b638145bd4
> --- /dev/null
> +++ b/kernel/unwind/eh_frame.c
[ ... ]
> +static __always_inline int read_encoded_pointer(struct eh_frame_section *sec,
> +						unsigned long *addr,
> +						unsigned long end,
> +						u8 encoding,
> +						unsigned long *value)
> +{
> +	unsigned long cur = *addr;
> +	u8 format = DW_EH_PE_format(encoding);
> +	u8 application = DW_EH_PE_application(encoding);
> +	unsigned long result = 0;
> +	int ret;
> +
> +	if (encoding == DW_EH_PE_omit)
> +		return -EINVAL;
> +
> +	/* Determine base address based on application */
> +	switch (application) {
> +	case 0:
> +		/* Absolute */
> +		break;
> +	case DW_EH_PE_pcrel:
> +		result = *addr;
> +		break;
> +	case DW_EH_PE_datarel:
> +		result = sec->eh_frame_hdr_start;
> +		break;
> +	case DW_EH_PE_textrel:
> +		result = sec->text_start;
> +		break;
> +	case DW_EH_PE_funcrel:
> +	case DW_EH_PE_aligned:
> +		return -EOPNOTSUPP;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	/* Read value based on format */
> +	switch (format) {
> +	case DW_EH_PE_absptr:
> +		UNSAFE_GET_USER_INC(result, cur, end, Efault);
> +		break;

[Severity: Medium]
Does this overwrite the base address that was just computed for relative
encodings?

When handling DW_EH_PE_absptr in combination with relative application flags
like DW_EH_PE_pcrel, the base address is stored in the result variable above.
However, UNSAFE_GET_USER_INC directly assigns the read value to result
instead of adding it, which will discard the base address.

Should this read into a temporary variable and add it to result, similar to
how the other formats are handled?

> +	case DW_EH_PE_uleb128: {
> +		unsigned long tmp;
> +		ret = read_uleb128(&cur, end, &tmp);
> +		if (ret)
> +			return ret;
> +		result += tmp;
> +		break;
> +	}
[ ... ]
> +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;
> +	unsigned long eh_frame_start, eh_frame_vma_end, table_start, table_end;
> +	u8 version, eh_frame_ptr_enc, fde_count_enc, table_enc;
> +	unsigned long fde_count;
> +	int entry_size;
> +	int ret;
[ ... ]
> +		/* Read FDE count */
> +		ret = read_encoded_pointer(sec, &cur, end,
> +					   fde_count_enc, &fde_count);
> +		if (ret)
> +			return ret;
> +
> +		/* Determine binary search table start and end */
> +		table_start = cur;
> +		entry_size = 2 * encoded_pointer_size(table_enc);
> +		if (!entry_size)
> +			return -EINVAL;
> +		table_end = table_start + fde_count * entry_size;
> +		if (table_end > sec->eh_frame_hdr_end)
> +			return -EINVAL;

[Severity: Medium]
Can a maliciously large fde_count cause an integer overflow here?

Since fde_count is parsed from the untrusted user eh_frame_hdr section,
the multiplication of fde_count * entry_size could wrap around to a small
positive value.

This would bypass the bounds check against sec->eh_frame_hdr_end and
potentially allow the kernel to trust an arbitrarily large fde_count,
leading to out-of-bounds kernel memory reads when performing binary searches.

Would it be safer to use check_mul_overflow() and check_add_overflow() for
these bounds calculations?

> +	}
> +
> +end:

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