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
