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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d2345191-0b73-4860-89be-9a189078d85e@linux.ibm.com>
Date: Wed, 5 Feb 2025 10:47:58 +0100
From: Jens Remus <jremus@...ux.ibm.com>
To: Josh Poimboeuf <jpoimboe@...nel.org>
Cc: x86@...nel.org, Peter Zijlstra <peterz@...radead.org>,
        Steven Rostedt <rostedt@...dmis.org>, Ingo Molnar <mingo@...nel.org>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        linux-kernel@...r.kernel.org, Indu Bhagat <indu.bhagat@...cle.com>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...nel.org>, Namhyung Kim <namhyung@...nel.org>,
        Ian Rogers <irogers@...gle.com>,
        Adrian Hunter <adrian.hunter@...el.com>,
        linux-perf-users@...r.kernel.org, Mark Brown <broonie@...nel.org>,
        linux-toolchains@...r.kernel.org, Jordan Rome <jordalgo@...a.com>,
        Sam James <sam@...too.org>, linux-trace-kernel@...r.kernel.org,
        Andrii Nakryiko <andrii.nakryiko@...il.com>,
        Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
        Florian Weimer <fweimer@...hat.com>, Andy Lutomirski <luto@...nel.org>,
        Masami Hiramatsu <mhiramat@...nel.org>, Weinan Liu <wnliu@...gle.com>,
        Heiko Carstens <hca@...ux.ibm.com>, Vasily Gorbik <gor@...ux.ibm.com>
Subject: 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
jremus@...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/