Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] DW_CFA_restore and DW_CFA_restore_extended fail to restore the complete register rule state (offset and register number). - [High] Bypass of pointer masking mitigation inside scoped_user_read_access_size blocks leading to a Spectre V1 vulnerability. - [Medium] Integer overflow during instruction pointer advancement in DW_CFA_advance_loc4. --
--- Patch [9]: [RFC PATCH v2 09/24] unwind_user/eh_frame: Add support for reading .eh_frame section --- commit cd03be3d9017de830eb9301ff0da6dd6d08e69be Author: Jens Remus <[email protected]> unwind_user/eh_frame: Add support for reading .eh_frame section In preparation for using .eh_frame to unwind user space stacks, add an eh_frame_find() interface for finding the .eh_frame[_hdr] information associated with a given text address. > diff --git a/kernel/unwind/eh_frame.c b/kernel/unwind/eh_frame.c > index 8cf19c5298fde..21f40805b417f 100644 > --- a/kernel/unwind/eh_frame.c > +++ b/kernel/unwind/eh_frame.c [ ... ] > +static __always_inline int __read_cie(struct eh_frame_section *sec, > + unsigned long cie_addr, > + struct eh_frame_cie *cie) > +{ > + void __user *cie_ptr = (void __user *)cie_addr; > + unsigned long cur = cie_addr, end; [ ... ] > + scoped_user_read_access_size(cie_ptr, 4 + length, Efault) { > + /* Read CIE_ID (must be 0 for CIE; FDE otherwise) */ > + UNSAFE_GET_USER_INC(cie_id, cur, end, Efault); [Severity: High] Are we bypassing the pointer masking mitigation here? The scoped_user_read_access_size() macro shadows the passed pointer (cie_ptr) with a masked version to prevent speculative kernel accesses. However, the memory read via UNSAFE_GET_USER_INC() uses the separate unmasked cur variable. Could this allow speculative out-of-bounds reads to access kernel memory? > + if (cie_id != EH_FRAME_CIE_ID) > + return -EINVAL; [ ... ] > +static __always_inline int __read_fde(struct eh_frame_section *sec, > + unsigned long fde_addr, > + struct eh_frame_fde *fde) > +{ > + void __user *fde_ptr = (void __user *)fde_addr; > + unsigned long cur = fde_addr, end; [ ... ] > + scoped_user_read_access_size(fde_ptr, 4 + length, Efault) { > + /* Read CIE pointer (offset from current position) */ > + UNSAFE_GET_USER_INC(cie_offset, cur, end, Efault); [Severity: High] Is this susceptible to the same pointer masking bypass as __read_cie()? The unmasked cur variable is being used here instead of the masked fde_ptr. > + cie_addr = cur - 4 - cie_offset; > + if (cie_addr + EH_FRAME_CIE_MIN_LENGTH > fde_addr) > + return -EINVAL; > + if (cie_addr < sec->eh_frame_start) > + return -EINVAL; > + } [ ... ] > +static __always_inline int __do_cfi_insn(struct eh_frame_section *sec, > + struct eh_frame_fde *fde, > + unsigned long *cur_ptr, > + unsigned long end, > + unsigned long *ip_ptr, > + unsigned long target_ip, > + struct eh_frame_cfi_context *ctx) > +{ [ ... ] > + case DW_CFA_restore: { > + u8 reg = DW_CFA_operand(opcode); > + int idx; > + > + if (ctx->cie) > + return -EINVAL; > + > + idx = reg_to_index(reg); > + if (idx >= 0) > + ctx->state.reg_rule[idx] = ctx->cie_state.reg_rule[idx]; > + break; > + } [Severity: High] Does this properly restore the complete register state? It appears only the rule type is restored from ctx->cie_state, but the offset (reg_offset) and register number (reg_regnum) are not restored and will remain at their previously modified values. [ ... ] > + case DW_CFA_advance_loc4: { > + unsigned long offset; > + u32 delta; > + > + UNSAFE_GET_USER_INC(delta, cur, end, Efault); > + offset = delta * fde->cie.code_align; [Severity: Medium] Can this calculation overflow? The multiplication of delta (a u32) and fde->cie.code_align (an unsigned int) evaluates as a 32-bit unsigned integer before being assigned to the 64-bit unsigned long offset. If the product exceeds 32 bits, the high bits would be truncated, which might cause the unwinder to desync from the actual PC. > + ip += offset; > + break; > + } [ ... ] > + case DW_CFA_restore_extended: { > + unsigned long reg; > + int idx; > + > + if (ctx->cie) > + return -EINVAL; > + > + ret = read_uleb128(&cur, end, ®); > + if (ret) > + return ret; > + > + idx = reg_to_index(reg); > + if (idx >= 0) > + ctx->state.reg_rule[idx] = > ctx->cie_state.reg_rule[idx]; > + break; > + } [Severity: High] This seems to have the same register state restoration issue as DW_CFA_restore. Should reg_offset and reg_regnum also be restored here alongside the rule type? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=9
