Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [Critical] Using `unsafe_copy_from_user` inside a `scoped_user_read_access_size` block prematurely clears the AC flag on x86, causing subsequent user memory accesses to trigger a fatal Page Fault (kernel panic). - [High] Integer overflow in the DWARF expression bounds check allows an infinite parsing loop in kernel space. - [High] The `cfa_offset` field of the CFI context state is passed to the unwinder uninitialized if a malformed CFI program starts with a `DW_CFA_def_cfa_register` instruction. - [Medium] Calling an out-of-line function within a UACCESS region violates objtool's safe execution rules. --
--- Patch [18]: [RFC PATCH v1 18/25] unwind_user/eh_frame: Add support for DWARF expressions --- commit 8c411d17e9c2d4af2ed33eb236e117fc360ae4f2 Author: Jens Remus <[email protected]> Subject: unwind_user/eh_frame: Add support for DWARF expressions This commit enables architectures to handle DWARF expressions in DWARF CFI instructions. It limits the maximum expression length to a reasonable size and provides inline stubs for architectures to override. > diff --git a/include/linux/unwind_user_eh_frame_types.h > b/include/linux/unwind_user_eh_frame_types.h > new file mode 100644 > --- /dev/null > +++ b/include/linux/unwind_user_eh_frame_types.h > @@ -0,0 +1,37 @@ [ ... ] > +struct eh_frame_reg_state { > + /* CFA recovery rule */ > + enum eh_frame_cfa_rule cfa_rule; > + unsigned long cfa_regnum; > + long cfa_offset; [Severity: High] What ensures cfa_offset is initialized when processing CFI programs? When struct eh_frame_cfi_context ctx is allocated on the kernel stack in __do_cfi_program(), its state.cfa_rule is initialized, but state.cfa_offset is not. If a user-supplied .eh_frame begins with DW_CFA_def_cfa_register, it sets ctx.state.cfa_rule = CFA_REG_OFFSET but leaves cfa_offset untouched. When __find_frame_row() executes, it verifies cfa_rule == CFA_REG_OFFSET and unconditionally reads the uninitialized kernel stack memory. Could this cause the unwinder to use unpredictable values for the CFA offset? > + > + /* FP and RA recovery rules (SP uses implicit recovery) */ > + enum eh_frame_reg_rule reg_rule[NR_REGS]; > + unsigned long reg_regnum[NR_REGS]; > + long reg_offset[NR_REGS]; > +}; [ ... ] > diff --git a/kernel/unwind/eh_frame.c b/kernel/unwind/eh_frame.c > --- a/kernel/unwind/eh_frame.c > +++ b/kernel/unwind/eh_frame.c [ ... ] > @@ -839,6 +808,27 @@ static __always_inline int __do_cfi_insn(struct > eh_frame_section *sec, > break; > } > > + case DW_CFA_def_cfa_expression: { > + unsigned long expr_len; > + char expr[EH_FRAME_MAX_EXPRESSION_LENGTH]; > + > + ret = read_uleb128(&cur, end, &expr_len); > + if (ret) > + return ret; > + > + if (cur + expr_len > end) [Severity: High] Can the pointer arithmetic wrap around if expr_len is maliciously large? Since expr_len is parsed from untrusted user input as an unsigned long, a large value could overflow cur + expr_len and wrap to a small value, bypassing this check against end. If this occurs, the parsing loop would advance cur by the overflowed expr_len, moving cur backwards. Would this lead to an infinite parsing loop and a hard lockup? > + return -EINVAL; > + > + if (expr_len > sizeof(expr)) > + return -EOPNOTSUPP; > + unsafe_copy_from_user(&expr, (void __user *)cur, > expr_len, Efault); [Severity: Critical] Might unsafe_copy_from_user() improperly toggle SMAP during expression parsing? Because this parsing runs inside a scoped_user_read_access_size block, calling unsafe_copy_from_user() on x86 defaults to __copy_from_user(), which executes its own stac() and clac() instructions. The inner clac() instruction clears the AC flag prematurely, ending the UACCESS region while the outer block assumes it is still open. When the loop continues to parse the next instruction using UNSAFE_GET_USER_INC, it will attempt a direct user memory read with the AC flag cleared. Will this result in a fatal page fault? > + ret = eh_frame_do_def_cfa_expression(expr, expr_len, > target_ip, &ctx->state); [Severity: Medium] Is it safe to call an architecture hook inside a UACCESS region? If an architecture overrides eh_frame_do_def_cfa_expression() with an out-of-line external function, it will trigger objtool's 'call to XXX() with UACCESS enabled' warning. Executing arbitrary functions with the AC flag set is strictly forbidden on x86 to prevent SMAP bypasses. > + if (ret) > + return ret; > + cur += expr_len; > + break; > + } [ ... ] > @@ -1005,9 +995,19 @@ static __always_inline int __do_cfi_insn(struct > eh_frame_section *sec, > if (cur + expr_len > end) > return -EINVAL; > > - if (reg == EH_FRAME_REG_SP || reg == EH_FRAME_REG_FP || > reg == EH_FRAME_REG_RA) > - return -EOPNOTSUPP; > + if (reg == EH_FRAME_REG_SP || reg == EH_FRAME_REG_FP || > reg == EH_FRAME_REG_RA) { > + char expr[EH_FRAME_MAX_EXPRESSION_LENGTH]; > > + if (expr_len > sizeof(expr)) > + return -EOPNOTSUPP; > + unsafe_copy_from_user(&expr, (void __user > *)cur, expr_len, Efault); [Severity: Critical] Does this copy run into the same SMAP clearing issue as mentioned above? Calling unsafe_copy_from_user() here will also execute a clac() instruction and clear the AC flag, causing subsequent user memory accesses to trigger a fatal page fault. > + if (opcode == DW_CFA_expression) > + ret = eh_frame_do_expression(reg, expr, > expr_len, target_ip, &ctx->state); > + else > + ret = eh_frame_do_val_expression(reg, > expr, expr_len, target_ip, &ctx->state); [Severity: Medium] Could these hooks also cause objtool UACCESS warnings? Similar to eh_frame_do_def_cfa_expression(), overriding these with external functions could lead to executing arbitrary code with the AC flag set. > + if (ret) > + return ret; > + } > cur += expr_len; > break; > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=18
