| 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
| ||
|
Message-ID: <6285a2b1-eb9b-4315-b960-cfaa99513ac1@linux.ibm.com>
Date: Thu, 17 Jul 2025 13:28:25 +0200
From: Jens Remus <jremus@...ux.ibm.com>
To: Josh Poimboeuf <jpoimboe@...nel.org>
Cc: linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org,
bpf@...r.kernel.org, x86@...nel.org,
Steven Rostedt <rostedt@...nel.org>,
Heiko Carstens <hca@...ux.ibm.com>, Vasily Gorbik <gor@...ux.ibm.com>,
Ilya Leoshkevich <iii@...ux.ibm.com>,
Masami Hiramatsu
<mhiramat@...nel.org>,
Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...nel.org>,
Jiri Olsa <jolsa@...nel.org>, Namhyung Kim <namhyung@...nel.org>,
Thomas Gleixner <tglx@...utronix.de>,
Andrii Nakryiko <andrii@...nel.org>,
Indu Bhagat <indu.bhagat@...cle.com>,
"Jose E. Marchesi" <jemarch@....org>,
Beau Belgrave <beaub@...ux.microsoft.com>,
Linus Torvalds <torvalds@...ux-foundation.org>,
Andrew Morton <akpm@...ux-foundation.org>,
Jens Axboe <axboe@...nel.dk>, Florian Weimer <fweimer@...hat.com>,
Sam James <sam@...too.org>
Subject: Re: [RFC PATCH v1 08/16] unwind_user: Enable archs that save RA/FP in
other registers
On 17.07.2025 04:01, Josh Poimboeuf wrote:
> On Thu, Jul 10, 2025 at 06:35:14PM +0200, Jens Remus wrote:
>> +#ifndef unwind_user_get_reg
>> +
>> +/**
>> + * generic_unwind_user_get_reg - Get register value.
>> + * @val: Register value.
>> + * @regnum: DWARF register number to obtain the value from.
>> + *
>> + * Returns zero if successful. Otherwise -EINVAL.
>> + */
>> +static inline int generic_unwind_user_get_reg(unsigned long *val, int regnum)
>> +{
>> + return -EINVAL;
>> +}
>> +
>> +#define unwind_user_get_reg generic_unwind_user_get_reg
>> +
>> +#endif /* !unwind_user_get_reg */
>
> I believe the traditional way to do this is to give the function the
> same name as the define:
>
> #ifndef unwind_user_get_reg
> static inline int unwind_user_get_reg(unsigned long *val, int regnum)
> {
> return -EINVAL;
> }
> #define unwind_user_get_reg unwind_user_get_reg
> #endif
Thanks! I will use use suggestion.
>> +/**
>> + * generic_sframe_set_frame_reginfo - Populate info to unwind FP/RA register
>> + * from SFrame offset.
>> + * @reginfo: Unwind info for FP/RA register.
>> + * @offset: SFrame offset value.
>> + *
>> + * A non-zero offset value denotes a stack offset from CFA and indicates
>> + * that the register is saved on the stack. A zero offset value indicates
>> + * that the register is not saved.
>> + */
>> +static inline void generic_sframe_set_frame_reginfo(
>> + struct unwind_user_reginfo *reginfo,
>> + s32 offset)
>> +{
>> + if (offset) {
>> + reginfo->loc = UNWIND_USER_LOC_STACK;
>> + reginfo->frame_off = offset;
>> + } else {
>> + reginfo->loc = UNWIND_USER_LOC_NONE;
>> + }
>> +}
>
> This just inits the reginfo struct, can we call it sframe_init_reginfo()?
>
> Also the function comment seems completely superfluous as the function
> is completely obvious.
>
> Also the signature should match kernel style, something like:
>
> static inline void
> sframe_init_reginfo(struct unwind_user_reginfo *reginfo, s32 offset)
Ditto.
>> @@ -98,26 +98,57 @@ static int unwind_user_next(struct unwind_user_state *state)
>>
>>
>> /* Get the Return Address (RA) */
>> - if (frame->ra_off) {
>> + switch (frame->ra.loc) {
>> + case UNWIND_USER_LOC_NONE:
>> + if (!IS_ENABLED(CONFIG_HAVE_USER_RA_REG) || !topmost)
>> + goto done;
>> + ra = user_return_address(task_pt_regs(current));
>> + break;
>> + case UNWIND_USER_LOC_STACK:
>> + if (!frame->ra.frame_off)
>> + 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))
>> + if ((cfa + frame->ra.frame_off) & ((1 << shift) - 1))
>> goto done;
>> - if (unwind_get_user_long(ra, cfa + frame->ra_off, state))
>> + if (unwind_get_user_long(ra, cfa + frame->ra.frame_off, state))
>> goto done;
>> - } else {
>> - if (!IS_ENABLED(CONFIG_HAVE_USER_RA_REG) || !topmost)
>> + break;
>> + case UNWIND_USER_LOC_REG:
>> + if (!IS_ENABLED(CONFIG_HAVE_UNWIND_USER_LOC_REG) || !topmost)
>> goto done;
>> - ra = user_return_address(task_pt_regs(current));
>> + if (unwind_user_get_reg(&ra, frame->ra.regnum))
>> + goto done;
>> + break;
>> + default:
>> + WARN_ON_ONCE(1);
>> + goto done;
>
> The default case will never happen, can we make it a BUG()?
Whatever Steve and you agree on. I am new to Kernel development.
>> }
>>
>> /* Get the Frame Pointer (FP) */
>> - if (frame->fp_off && unwind_get_user_long(fp, cfa + frame->fp_off, state))
>> + switch (frame->fp.loc) {
>> + case UNWIND_USER_LOC_NONE:
>> + break;
>
> The UNWIND_USER_LOC_NONE behavior is different here compared to above.
See my comments below.
> Do we also need UNWIND_USER_LOC_PT_REGS?
Sorry, I cannot follow. Do you suggest to rename UNWIND_USER_LOC_REG to
UNWIND_USER_LOC_PT_REGS?
>> + case UNWIND_USER_LOC_STACK:
>> + if (!frame->fp.frame_off)
>> + goto done;
>> + if (unwind_get_user_long(fp, cfa + frame->fp.frame_off, state))
>> + goto done;
>> + break;
>> + case UNWIND_USER_LOC_REG:
>> + if (!IS_ENABLED(CONFIG_HAVE_UNWIND_USER_LOC_REG) || !topmost)
>> + goto done;
>
> The topmost checking is *really* getting cumbersome, I do hope we can
> get rid of that.
Restoring from arbitrary registers is only valid in the topmost frame,
as their values (i.e. task_pt_regs(current)) are only available there.
For other frames only SP, FP, and RA register values are available.
I think this test makes sense. Is this test really that expensive?
>> + if (unwind_user_get_reg(&fp, frame->fp.regnum))
>> + goto done;
>> + break;
>> + default:
>> + WARN_ON_ONCE(1);
>> goto done;
>> + }
>
> BUG(1) here as well.
Same as for other WARN_ON_ONCE() vs. BUG().
>> state->ip = ra;
>> state->sp = sp;
>> - if (frame->fp_off)
>> + if (frame->fp.loc != UNWIND_USER_LOC_NONE)
>> state->fp = fp;
>
> Instead of the extra conditional here, can fp be initialized to zero?
> Either at the top of the function or in the RA switch statement?
No. But fp could be initialized to state->fp, so that when it is not
saved and thus not restored it keeps it previous value.
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/
Powered by blists - more mailing lists