Messages in this thread Patch in this message | | | Date | Thu, 2 May 2024 17:49:43 -0700 | From | Kees Cook <> | Subject | Re: [PATCH v3] fs/coredump: Enable dynamic configuration of max file note size |
| |
On Thu, May 02, 2024 at 11:56:03PM +0000, Allen Pais wrote: > Introduce the capability to dynamically configure the maximum file > note size for ELF core dumps via sysctl. This enhancement removes > the previous static limit of 4MB, allowing system administrators to > adjust the size based on system-specific requirements or constraints. > > - Remove hardcoded `MAX_FILE_NOTE_SIZE` from `fs/binfmt_elf.c`. > - Define `max_file_note_size` in `fs/coredump.c` with an initial value > set to 4MB. > - Declare `max_file_note_size` as an external variable in > `include/linux/coredump.h`. > - Add a new sysctl entry in `kernel/sysctl.c` to manage this setting > at runtime.
The above bullet points should be clear from the patch itself. The commit is really more about rationale and examples (which you have below). I'd remove the bullets.
> > $ sysctl -a | grep core_file_note_size_max > kernel.core_file_note_size_max = 4194304 > > $ sysctl -n kernel.core_file_note_size_max > 4194304 > > $echo 519304 > /proc/sys/kernel/core_file_note_size_max > > $sysctl -n kernel.core_file_note_size_max > 519304 > > Attempting to write beyond the ceiling value of 16MB > $echo 17194304 > /proc/sys/kernel/core_file_note_size_max > bash: echo: write error: Invalid argument > > Why is this being done? > We have observed that during a crash when there are more than 65k mmaps > in memory, the existing fixed limit on the size of the ELF notes section > becomes a bottleneck. The notes section quickly reaches its capacity, > leading to incomplete memory segment information in the resulting coredump. > This truncation compromises the utility of the coredumps, as crucial > information about the memory state at the time of the crash might be > omitted.
I'd make this the first paragraph of the commit log. "We have this problem" goes first, then "Here's what we did to deal with it", then you examples. :)
> > Signed-off-by: Vijay Nag <nagvijay@microsoft.com> > Signed-off-by: Allen Pais <apais@linux.microsoft.com> > > --- > Chagnes in v3: > - Fix commit message to reflect the correct sysctl knob [Kees] > - Add a ceiling for maximum pssible note size(16M) [Allen] > - Add a pr_warn_once() [Kees] > Changes in v2: > - Move new sysctl to fs/coredump.c [Luis & Kees] > - rename max_file_note_size to core_file_note_size_max [kees] > - Capture "why this is being done?" int he commit message [Luis & Kees] > --- > fs/binfmt_elf.c | 8 ++++++-- > fs/coredump.c | 15 +++++++++++++++ > include/linux/coredump.h | 1 + > 3 files changed, 22 insertions(+), 2 deletions(-) > > diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c > index 5397b552fbeb..5294f8f3a9a8 100644 > --- a/fs/binfmt_elf.c > +++ b/fs/binfmt_elf.c > @@ -1564,7 +1564,6 @@ static void fill_siginfo_note(struct memelfnote *note, user_siginfo_t *csigdata, > fill_note(note, "CORE", NT_SIGINFO, sizeof(*csigdata), csigdata); > } > > -#define MAX_FILE_NOTE_SIZE (4*1024*1024) > /* > * Format of NT_FILE note: > * > @@ -1592,8 +1591,13 @@ static int fill_files_note(struct memelfnote *note, struct coredump_params *cprm > > names_ofs = (2 + 3 * count) * sizeof(data[0]); > alloc: > - if (size >= MAX_FILE_NOTE_SIZE) /* paranoia check */ > + /* paranoia check */ > + if (size >= core_file_note_size_max) { > + pr_warn_once("coredump Note size too large: %u " > + "(does kernel.core_file_note_size_max sysctl need adjustment?)\n",
The string can be on a single line (I think scripts/check_patch.pl will warn about this, as well as the indentation of "size" below...
> + size); > return -EINVAL; > + } > size = round_up(size, PAGE_SIZE); > /* > * "size" can be 0 here legitimately. > diff --git a/fs/coredump.c b/fs/coredump.c > index be6403b4b14b..ffaed8c1b3b0 100644 > --- a/fs/coredump.c > +++ b/fs/coredump.c > @@ -56,10 +56,16 @@ > static bool dump_vma_snapshot(struct coredump_params *cprm); > static void free_vma_snapshot(struct coredump_params *cprm); > > +#define MAX_FILE_NOTE_SIZE (4*1024*1024) > +/* Define a reasonable max cap */ > +#define MAX_ALLOWED_NOTE_SIZE (16*1024*1024)
Let's call this CORE_FILE_NOTE_SIZE_DEFAULT and CORE_FILE_NOTE_SIZE_MAX to match the sysctl.
> + > static int core_uses_pid; > static unsigned int core_pipe_limit; > static char core_pattern[CORENAME_MAX_SIZE] = "core"; > static int core_name_size = CORENAME_MAX_SIZE; > +unsigned int core_file_note_size_max = MAX_FILE_NOTE_SIZE; > +unsigned int core_file_note_size_allowed = MAX_ALLOWED_NOTE_SIZE;
The latter can be static and const.
For the note below, perhaps add:
static const unsigned int core_file_note_size_min = CORE_FILE_NOTE_SIZE_DEFAULT;
> > struct core_name { > char *corename; > @@ -1020,6 +1026,15 @@ static struct ctl_table coredump_sysctls[] = { > .mode = 0644, > .proc_handler = proc_dointvec, > }, > + { > + .procname = "core_file_note_size_max", > + .data = &core_file_note_size_max, > + .maxlen = sizeof(unsigned int), > + .mode = 0644, > + .proc_handler = proc_douintvec_minmax, > + .extra1 = &core_file_note_size_max,
This means you can never shrink it if you raise it from the default. Let's use the core_file_note_size_min above.
> + .extra2 = &core_file_note_size_allowed, > + }, > }; > > static int __init init_fs_coredump_sysctls(void) > diff --git a/include/linux/coredump.h b/include/linux/coredump.h > index d3eba4360150..14c057643e7f 100644 > --- a/include/linux/coredump.h > +++ b/include/linux/coredump.h > @@ -46,6 +46,7 @@ static inline void do_coredump(const kernel_siginfo_t *siginfo) {} > #endif > > #if defined(CONFIG_COREDUMP) && defined(CONFIG_SYSCTL) > +extern unsigned int core_file_note_size_max; > extern void validate_coredump_safety(void); > #else > static inline void validate_coredump_safety(void) {} > -- > 2.17.1 >
I think v4 will be all good to go, assuming no one else pops up. :) Thanks for the changes!
-Kees
-- Kees Cook
| |