Re: [RFC PATCH v1 07/16] unwind_user: Enable archs that do not necessarily save RA
From: Josh Poimboeuf <jpoimboe@kernel.org>
Date: 2025-07-16 23:01:52
Also in:
bpf, lkml
On Thu, Jul 10, 2025 at 06:35:13PM +0200, Jens Remus wrote:
quoted hunk ↗ jump to hunk
+++ b/arch/Kconfig@@ -450,6 +450,11 @@ config HAVE_UNWIND_USER_SFRAME bool select UNWIND_USER +config HAVE_USER_RA_REG + bool + help + The arch passes the return address (RA) in user space in a register.
How about "HAVE_UNWIND_USER_RA_REG" so it matches the existing namespace?
quoted hunk ↗ jump to hunk
@@ -310,6 +307,12 @@ static __always_inline int __find_fre(struct sframe_section *sec, return -EINVAL; fre = prev_fre; + if ((!IS_ENABLED(CONFIG_HAVE_USER_RA_REG) || !topmost) && !fre->ra_off) { + dbg_sec_uaccess("fde addr 0x%x: zero ra_off\n", + fde->start_addr); + return -EINVAL; + }
The topmost frame doesn't necessarily (or even likely) come from before the prologue, or from a leaf function, so this check would miss the case where a non-leaf function wrongly has !ra_off after its prologue. Which in reality is probably fine, as there are other guardrails in place to catch such bad sframe data. But then do we think the !ra_off check is still worth the effort? It would be simpler to just always assume !ra_off is valid for the CONFIG_HAVE_USER_RA_REG case. I think I prefer the simplicity of removing the check, as the error would be rare, and corrupt sframe would be caught in other ways.
quoted hunk ↗ jump to hunk
@@ -86,18 +88,28 @@ static int unwind_user_next(struct unwind_user_state *state) /* Get the Stack Pointer (SP) */ sp = cfa + frame->sp_val_off; - /* Make sure that stack is not going in wrong direction */ - if (sp <= state->sp) + /* + * Make sure that stack is not going in wrong direction. Allow SP + * to be unchanged for the topmost frame, by subtracting topmost, + * which is either 0 or 1. + */ + if (sp <= state->sp - topmost) goto done; - /* Make sure that the address is word aligned */ - shift = sizeof(long) == 4 || compat_fp_state(state) ? 2 : 3; - if ((cfa + frame->ra_off) & ((1 << shift) - 1)) - goto done; /* Get the Return Address (RA) */ - if (unwind_get_user_long(ra, cfa + frame->ra_off, state)) - goto done; + if (frame->ra_off) { + /* Make sure that the address is word aligned */ + shift = sizeof(long) == 4 || compat_fp_state(state) ? 2 : 3; + if ((cfa + frame->ra_off) & ((1 << shift) - 1)) + goto done; + if (unwind_get_user_long(ra, cfa + frame->ra_off, state)) + goto done; + } else { + if (!IS_ENABLED(CONFIG_HAVE_USER_RA_REG) || !topmost) + goto done;
I think this check is redundant with the one in __find_fre()? -- Josh