| 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: <20250708123120.7862c458@gandalf.local.home>
Date: Tue, 8 Jul 2025 12:31:20 -0400
From: Steven Rostedt <rostedt@...dmis.org>
To: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: Josh Poimboeuf <jpoimboe@...nel.org>, Steven Rostedt
<rostedt@...nel.org>, linux-kernel@...r.kernel.org,
linux-trace-kernel@...r.kernel.org, bpf@...r.kernel.org, x86@...nel.org,
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>,
Jens Remus <jremus@...ux.ibm.com>, Andrew Morton
<akpm@...ux-foundation.org>, Jens Axboe <axboe@...nel.dk>, Florian Weimer
<fweimer@...hat.com>, Sam James <sam@...too.org>
Subject: Re: [PATCH v8 10/12] unwind_user/sframe: Enable debugging in
uaccess regions
On Tue, 8 Jul 2025 08:53:56 -0700
Linus Torvalds <torvalds@...ux-foundation.org> wrote:
> On Tue, 8 Jul 2025 at 07:41, Steven Rostedt <rostedt@...dmis.org> wrote:
> >
> > Would something like this work? If someone enables the config to enable the
> > validation, I don't think we need dynamic printk to do it (as that requires
> > passing in the format directly and not via a pointer).
>
> I really think you should just not use 'user_access_begin()" AT ALL if
> you need to play these kinds of games.
>
Looking at the code a bit deeper, I don't think we need to play these games
and still keep the user_read_access_begin().
The places that are more performance critical (where it reads the sframe
during normal stack walking during profiling) has no debug output, and
there's nothing there that needs to take it out of the user_read_access
area.
It's the validator that adds these hacks. I don't think it needs to. It can
just wrap the calls to the code that requires user_read_access and then
check the return value. The validator is just a debugging feature and
performance should not be an issue.
But I do think performance is something to care about during normal
operations where the one big user_read_access_begin() can help.
What about something like this? It adds "safe" versions of the user space
access functions and uses them only in the slow (we don't care about
performance) validator:
-- Steve
diff --git a/kernel/unwind/sframe.c b/kernel/unwind/sframe.c
index 0060cc576776..79ff3c0fc11f 100644
--- a/kernel/unwind/sframe.c
+++ b/kernel/unwind/sframe.c
@@ -321,7 +321,34 @@ int sframe_find(unsigned long ip, struct unwind_user_frame *frame)
#ifdef CONFIG_SFRAME_VALIDATION
-static __always_inline int __sframe_validate_section(struct sframe_section *sec)
+static int safe_read_fde(struct sframe_section *sec,
+ unsigned int fde_num, struct sframe_fde *fde)
+{
+ int ret;
+
+ if (!user_read_access_begin((void __user *)sec->sframe_start,
+ sec->sframe_end - sec->sframe_start))
+ return -EFAULT;
+ ret = __read_fde(sec, fde_num, fde);
+ user_read_access_end();
+ return ret;
+}
+
+static int safe_read_fre(struct sframe_section *sec,
+ struct sframe_fde *fde, unsigned long fre_addr,
+ struct sframe_fre *fre)
+{
+ int ret;
+
+ if (!user_read_access_begin((void __user *)sec->sframe_start,
+ sec->sframe_end - sec->sframe_start))
+ return -EFAULT;
+ ret = __read_fre(sec, fde, fre_addr, fre);
+ user_read_access_end();
+ return ret;
+}
+
+static int sframe_validate_section(struct sframe_section *sec)
{
unsigned long prev_ip = 0;
unsigned int i;
@@ -335,13 +362,13 @@ static __always_inline int __sframe_validate_section(struct sframe_section *sec)
unsigned int j;
int ret;
- ret = __read_fde(sec, i, &fde);
+ ret = safe_read_fde(sec, i, &fde);
if (ret)
return ret;
ip = sec->sframe_start + fde.start_addr;
if (ip <= prev_ip) {
- dbg_sec_uaccess("fde %u not sorted\n", i);
+ dbg_sec("fde %u not sorted\n", i);
return -EFAULT;
}
prev_ip = ip;
@@ -353,17 +380,20 @@ static __always_inline int __sframe_validate_section(struct sframe_section *sec)
fre = which ? fres : fres + 1;
which = !which;
- ret = __read_fre(sec, &fde, fre_addr, fre);
+ ret = safe_read_fre(sec, &fde, fre_addr, fre);
if (ret) {
- dbg_sec_uaccess("fde %u: __read_fre(%u) failed\n", i, j);
- dbg_print_fde_uaccess(sec, &fde);
+ dbg_sec("fde %u: __read_fre(%u) failed\n", i, j);
+ dbg_sec("FDE: start_addr:0x%x func_size:0x%x fres_off:0x%x fres_num:%d info:%u rep_size:%u\n",
+ fde.start_addr, fde.func_size,
+ fde.fres_off, fde.fres_num,
+ fde.info, fde.rep_size);
return ret;
}
fre_addr += fre->size;
if (prev_fre && fre->ip_off <= prev_fre->ip_off) {
- dbg_sec_uaccess("fde %u: fre %u not sorted\n", i, j);
+ dbg_sec("fde %u: fre %u not sorted\n", i, j);
return -EFAULT;
}
@@ -374,21 +404,6 @@ static __always_inline int __sframe_validate_section(struct sframe_section *sec)
return 0;
}
-static int sframe_validate_section(struct sframe_section *sec)
-{
- int ret;
-
- if (!user_read_access_begin((void __user *)sec->sframe_start,
- sec->sframe_end - sec->sframe_start)) {
- dbg_sec("section usercopy failed\n");
- return -EFAULT;
- }
-
- ret = __sframe_validate_section(sec);
- user_read_access_end();
- return ret;
-}
-
#else /* !CONFIG_SFRAME_VALIDATION */
static int sframe_validate_section(struct sframe_section *sec) { return 0; }
Powered by blists - more mailing lists