linux-kernel - Re: [PATCH v8 10/12] unwind_user/sframe: Enable debugging in uaccess regions

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: <20250708104130.25b76df9@gandalf.local.home>
Date: Tue, 8 Jul 2025 10:41:30 -0400
From: Steven Rostedt <rostedt@...dmis.org>
To: Josh Poimboeuf <jpoimboe@...nel.org>
Cc: Linus Torvalds <torvalds@...ux-foundation.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 07:34:36 -0700
Josh Poimboeuf <jpoimboe@...nel.org> wrote:

> I had found those debug printks really useful for debugging
> corrupt/missing .sframe data, but yeah, this patch is ridiculously bad.
> Sorry for putting that out into the world ;-)
> 
> And those are all error paths, so it's rather pointless to do that whole
> uaccess disable/enable/disable dance.
> 
> So yeah, drop it for now and I can replace it with something better
> later on.

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).

-- Steve

diff --git a/kernel/unwind/sframe.c b/kernel/unwind/sframe.c
index 0060cc576776..524738e2b823 100644
--- a/kernel/unwind/sframe.c
+++ b/kernel/unwind/sframe.c
@@ -321,11 +321,24 @@ 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)
+/* Used to save error messages in uaccess sections */
+struct err_msg {
+	const char		*fmt;
+	int			param1;
+	int			param2;
+	long			param3;
+	long			param4;
+};
+
+static __always_inline
+int __sframe_validate_section(struct sframe_section *sec, struct err_msg *err)
 {
 	unsigned long prev_ip = 0;
 	unsigned int i;
 
+/* current->comm, current->pid, sec->filename */
+#define ERR_HDR KERN_WARNING "%s (%d) %s: "
+
 	for (i = 0; i < sec->num_fdes; i++) {
 		struct sframe_fre *fre, *prev_fre = NULL;
 		unsigned long ip, fre_addr;
@@ -341,7 +354,8 @@ static __always_inline int __sframe_validate_section(struct sframe_section *sec)
 
 		ip = sec->sframe_start + fde.start_addr;
 		if (ip <= prev_ip) {
-			dbg_sec_uaccess("fde %u not sorted\n", i);
+			err->fmt = ERR_HDR "fde %u not sorted\n";
+			err->param1 = i;
 			return -EFAULT;
 		}
 		prev_ip = ip;
@@ -355,15 +369,23 @@ static __always_inline int __sframe_validate_section(struct sframe_section *sec)
 
 			ret = __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);
+				err->fmt = ERR_HDR
+					"fde %u: __read_fre(%u) failed\n"
+					"  frame_start=%lx frame_end=%lx\n";
+				err->param1 = i;
+				err->param2 = j;
+				err->param3 = (long)sec->sframe_start;
+				err->param4 = (long)sec->sframe_end;
 				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);
+				err->fmt = ERR_HDR
+					"fde %u: fre %u not sorted\n";
+				err->param1 = i;
+				err->param2 = j;
 				return -EFAULT;
 			}
 
@@ -376,16 +398,26 @@ static __always_inline int __sframe_validate_section(struct sframe_section *sec)
 
 static int sframe_validate_section(struct sframe_section *sec)
 {
+	struct err_msg err;
 	int ret;
 
+	memset(&err, 0, sizeof(err));
+
 	if (!user_read_access_begin((void __user *)sec->sframe_start,
 				    sec->sframe_end - sec->sframe_start)) {
-		dbg_sec("section usercopy failed\n");
+		pr_warn("%s (%d): section usercopy failed\n",
+			current->comm, current->pid);
 		return -EFAULT;
 	}
 
-	ret = __sframe_validate_section(sec);
+	ret = __sframe_validate_section(sec, &err);
 	user_read_access_end();
+
+	if (ret) {
+		printk(err.fmt, current->comm, current->pid,
+		       sec->filename, err.param1, err.param2,
+		       err.param3, err.param4);
+	}
 	return ret;
 }