Message-ID: <202502191134.CC80931AC9@keescook>
Date: Wed, 19 Feb 2025 11:52:11 -0800
From: Kees Cook <kees@...nel.org>
To: Jan Kara <jack@...e.cz>, Michael Stapelberg <michael@...pelberg.ch>,
	Brian Mak <makb@...iper.net>
Cc: Christian Brauner <brauner@...nel.org>,
	"Eric W. Biederman" <ebiederm@...ssion.com>,
	"linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-mm@...ck.org" <linux-mm@...ck.org>,
	Oleg Nesterov <oleg@...hat.com>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Alexander Viro <viro@...iv.linux.org.uk>
Subject: Re: [PATCH v3] binfmt_elf: Dump smaller VMAs first in ELF cores

On Wed, Feb 19, 2025 at 05:20:17PM +0100, Jan Kara wrote:
> On Tue 18-02-25 19:53:51, Brian Mak wrote:
> > On Feb 18, 2025, at 12:54 AM, Michael Stapelberg <michael@...pelberg.ch> wrote:
> > 
> > > I think in your testing, you probably did not try the eu-stack tool
> > > from the elfutils package, because I think I found a bug:
> > 
> > Hi Michael,
> > 
> > Thanks for the report. I can confirm that this issue does seem to be
> > from this commit. I tested it with Juniper's Linux kernel with and
> > without the changes.
> > 
> > You're correct that the original testing done did not include the
> > eu-stack tool.
> > 
> > > Current elfutils cannot symbolize core dumps created by Linux 6.12+.
> > > I noticed this because systemd-coredump(8) uses elfutils, and when
> > > a program crashed on my machine, syslog did not show function names.
> > > 
> > > I reported this issue with elfutils at:
> > > https://urldefense.com/v3/__https://sourceware.org/bugzilla/show_bug.cgi?id=32713__;!!NEt6yMaO-gk!DbttKuHxkBdrV4Cj9axM3ED6mlBHXeQGY3NVzvfDlthl-K39e9QIrZcwT8iCXLRu0OivWRGgficcD-aCuus$
> > > …but figured it would be good to give a heads-up here, too.
> > > 
> > > Is this breakage sufficient reason to revert the commit?
> > > Or are we saying userspace just needs to be updated to cope?
> > 
> > The way I see it is that, as long as we're in compliance with the
> > applicable ELF specifications, then the issue lies with userspace apps
> > to ensure that they are not making additional erroneous assumptions.
> > 
> > However, Eric mentioned a while ago in v1 of this patch that he believes
> > that the ELF specification requires program headers be written in memory
> > order. Digging through the ELF specifications, I found that any loadable
> > segment entries in the program header table must be sorted on the
> > virtual address of the first byte of which the segment resides in
> > memory.
> > 
> > This indicates that we have deviated from the ELF specification with
> > this commit. One thing we can do to remedy this is to have program
> > headers sorted according to the specification, but then continue dumping
> > in VMA size ordering. This would make the dumping logic significantly
> > more complex though.
> > 
> > Seeing how most popular userspace apps, with the exception of eu-stack,
> > seem to work, we could also just leave it, and tell userspace apps to
> > fix it on their end.
> 
> Well, it does not seem eu-stack is that unpopular and we really try hard to
> avoid user visible regressions. So I think we should revert the change. Also
> the fact that the patch breaks ELF spec is an indication there may be other
> tools that would get confused by this and another reason for a revert...

Yeah, I think we need to make this a tunable. Updating the kernel breaks
elftools, which isn't some weird custom corner case. :P

So, while it took a few months, here is a report of breakage that I said
we'd need to watch for[1]. :)

Is anyone able to test this patch? And Brian will setting a sysctl be
okay for your use-case?


diff --git a/Documentation/admin-guide/sysctl/kernel.rst b/Documentation/admin-guide/sysctl/kernel.rst
index a43b78b4b646..35d5d86cff69 100644
--- a/Documentation/admin-guide/sysctl/kernel.rst
+++ b/Documentation/admin-guide/sysctl/kernel.rst
@@ -222,6 +222,17 @@ and ``core_uses_pid`` is set, then .PID will be appended to
 the filename.
 
 
+core_sort_vma
+=============
+
+The default coredump writes VMAs in address order. By setting
+``core_sort_vma`` to 1, VMAs will be written from smallest size
+to largest size. This is known to break at least elfutils, but
+can be handy when dealing with very large (and truncated)
+coredumps where the more useful debugging details are included
+in the smaller VMAs.
+
+
 ctrl-alt-del
 ============
 
diff --git a/fs/coredump.c b/fs/coredump.c
index 591700e1b2ce..4375c70144d0 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -63,6 +63,7 @@ static void free_vma_snapshot(struct coredump_params *cprm);
 
 static int core_uses_pid;
 static unsigned int core_pipe_limit;
+static unsigned int core_sort_vma;
 static char core_pattern[CORENAME_MAX_SIZE] = "core";
 static int core_name_size = CORENAME_MAX_SIZE;
 unsigned int core_file_note_size_limit = CORE_FILE_NOTE_SIZE_DEFAULT;
@@ -1026,6 +1027,15 @@ static const struct ctl_table coredump_sysctls[] = {
 		.extra1		= (unsigned int *)&core_file_note_size_min,
 		.extra2		= (unsigned int *)&core_file_note_size_max,
 	},
+	{
+		.procname	= "core_sort_vma",
+		.data		= &core_sort_vma,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_douintvec_minmax,
+		.extra1		= SYSCTL_ZERO,
+		.extra2		= SYSCTL_ONE,
+	},
 };
 
 static int __init init_fs_coredump_sysctls(void)
@@ -1256,8 +1266,9 @@ static bool dump_vma_snapshot(struct coredump_params *cprm)
 		cprm->vma_data_size += m->dump_size;
 	}
 
-	sort(cprm->vma_meta, cprm->vma_count, sizeof(*cprm->vma_meta),
-		cmp_vma_size, NULL);
+	if (core_sort_vma)
+		sort(cprm->vma_meta, cprm->vma_count, sizeof(*cprm->vma_meta),
+		     cmp_vma_size, NULL);
 
 	return true;
 }



-Kees

[1] https://lore.kernel.org/all/202408092104.FCE51021@keescook/

-- 
Kees Cook

Powered by blists - more mailing lists