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

[Jens Remus]

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@xxxxxxxxxx

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/