On 17.07.2025 01:01, Josh Poimboeuf wrote:
> On Thu, Jul 10, 2025 at 06:35:13PM +0200, Jens Remus wrote:
>> +++ 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?
Ok. I am open to any improvements.
>> @@ -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.
On s390 the compiler may intermingle prologue instructions with function
body instructions. As a result !ra_off is valid as long as the RA
register value from function entry has not been destroyed. Furthermore
the compiler may decide to save RA only if it is actually about to
destroy the RA register value (e.g. due to a function call).
Note that it is valid to restore the RA register value anywhere in a
function and represent that in SFrame. Following is an example from
Glibc libc.so psignal() on s390x. The RA rule changes back to "u"
(= undefined; !ra_off) multiple times, whenever the RA register has
its value from function entry again.
func idx [589]: pc = 0x6c530, size = 250 bytes <psignal>
STARTPC CFA FP RA
000000000006c530 sp+160 u u
000000000006c536 sp+160 c-72 c-48
000000000006c53c sp+328 c-72 c-48
000000000006c5b4 sp+160 u u
000000000006c5b6 sp+328 c-72 c-48
000000000006c60c sp+160 u u
000000000006c60e sp+328 c-72 c-48
> 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.
It is only valid in the topmost frame. Otherwise something is wrong.
> 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.
But I am fine with removing it in user unwind sframe (above), as
user unwind performs the same check (see below).
>> @@ -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()?
Correct. This one needs to stay, as it is at the topmost level (user
unwind checks the unwind info obtained from user unwind sframe or any
other method).
Regards,
Jens
--
Jens Remus
Linux on Z Development (D3303)
+49-7031-16-1128 Office
jre...@de.ibm.com
IBM
IBM Deutschland Research & Development GmbH; Vorsitzender des Aufsichtsrats:
Wolfgang Wendt; Geschäftsführung: David Faller; Sitz der Gesellschaft:
Böblingen; Registergericht: Amtsgericht Stuttgart, HRB 243294
IBM Data Privacy Statement: https://www.ibm.com/privacy/
