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

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
[email protected]

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/