Re: [PATCH v4 19/39] unwind_user/sframe: Add support for reading .sframe contents

Jens Remus
2025-02-05T09:49:00Z

On 04.02.2025 19:51, Josh Poimboeuf wrote:
> On Thu, Jan 30, 2025 at 04:47:00PM +0100, Jens Remus wrote:
>> On 22.01.2025 03:31, Josh Poimboeuf wrote:

>>> +#define __UNSAFE_GET_USER_INC(to, from, type, label) \
>>> +({ \
>>> + type __to; \
>>> + unsafe_get_user(__to, (type __user *)from, label); \
>>> + from += sizeof(__to); \
>>> + to = (typeof(to))__to; \
>>> +})
>>> +
>>> +#define UNSAFE_GET_USER_INC(to, from, size, label) \
>>> +({ \
>>> + switch (size) { \
>>> + case 1: \
>>> + __UNSAFE_GET_USER_INC(to, from, u8, label); \
>>> + break; \
>>> + case 2: \
>>> + __UNSAFE_GET_USER_INC(to, from, u16, label); \
>>> + break; \
>>> + case 4: \
>>> + __UNSAFE_GET_USER_INC(to, from, u32, label); \
>>> + break; \
>>> + default: \
>>> + return -EFAULT; \
>>> + } \
>>> +})
>>
>> This does not work for the signed SFrame fields, such as the FRE CFA,
>> RA, and FP offsets, as it does not perform the required sign extension.
>> One option would be to rename to UNSAFE_GET_USER_UNSIGNED_INC() and
>> re-introduce UNSAFE_GET_USER_SIGNED_INC() using s8, s16, and s32.
>
> See the following line in __UNSAFE_GET_USER_INC():
>
> to = (typeof(to))__to;
>
> Does that not do the sign extension?

No. In practice with my proposed changes reverted and the following
debugging code added:

@@ -293,6 +293,10 @@ static __always_inline int __find_fre(struct sframe_section *sec,
return -EINVAL;
fre = prev_fre;

+ dbg_sec_uaccess("fre: ip_off=%u, cfa_off=%d, ra_off=%d, fp_off=%d, use_fp=%s, sp_val_off=%d\n",
+ fre->ip_off, fre->cfa_off, fre->ra_off, fre->fp_off,
+ SFRAME_FRE_CFA_BASE_REG_ID(fre->info) == SFRAME_BASE_REG_FP ? "y" : "n",
+ sframe_sp_val_off());

Excerpt from dmesg:

sframe: /usr/lib/ld64.so.1: fre: ip_off=16, cfa_off=440, ra_off=208, fp_off=184, use_fp=n, sp_val_off=-160
sframe: /usr/lib/ld64.so.1: fre: ip_off=2600, cfa_off=672, ra_off=208, fp_off=184, use_fp=y, sp_val_off=-160
sframe: /usr/lib/ld64.so.1: fre: ip_off=10, cfa_off=368, ra_off=0, fp_off=0, use_fp=n, sp_val_off=-160
sframe: /usr/lib/ld64.so.1: fre: ip_off=722, cfa_off=672, ra_off=208, fp_off=184, use_fp=y, sp_val_off=-160

On s390 the register save slots have negative offsets from CFA (due to
the CFA to be defined as SP at call site + 160). The RA, if saved,
would be saved at CFA-48 on the stack. I.e. ra_off=-48 instead of
ra_off=208 would have been correct.

208 = 0xd0 (unsigned) = -48 (signed)

Looking at the code:

UNSAFE_GET_USER_INC(ra_off, cur, offset_size, Efault);

With offset_size=1 expands into:

__UNSAFE_GET_USER_INC(/*to=*/ra_off, /*from=*cur, /*type=*/u8, /*label=*/Efault);

Expands into:

{
u8 __to;
unsafe_get_user(__to, (u8 __user *)cur, Efault);
cur += sizeof(__to);
ra_off = (typeof(ra_off))__to;
}

The issue is that on the last line __to is u8 instead of s8 and thus
u8 gets casted to s32, which is performed without sign extension. __to
would need to be s8 or get casted to s8 for sign extension to take
place.

Regards,
Jens
--
Jens Remus
Linux on Z Development (D3303)
+49-7031-16-1128 Office
jremus@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/