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 [off-list ref]

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.

[ ... ]

+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/20260818144954.2320378-1-jremus@linux.ibm.com?part=7