Message-ID: <CAOMdWSLh80OHx=som1WeK_L_=2LVK1rehXH88t3Ew--4dSE4ew@mail.gmail.com>
Date: Thu, 2 May 2024 18:40:58 -0700
From: Allen <allen.lkml@...il.com>
To: Kees Cook <keescook@...omium.org>
Cc: Allen Pais <apais@...ux.microsoft.com>, linux-fsdevel@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-mm@...ck.org, viro@...iv.linux.org.uk, 
	brauner@...nel.org, jack@...e.cz, ebiederm@...ssion.com, mcgrof@...nel.org, 
	j.granados@...sung.com
Subject: Re: [PATCH v3] fs/coredump: Enable dynamic configuration of max file
 note size

> > > 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.
>
> Sure, I have it modified to:
>
> fs/coredump: Enable dynamic configuration of max file note size
>
>     Introduce the capability to dynamically configure the maximum file
>     note size for ELF core dumps via sysctl.
>
>     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.
>
>     This enhancement removes the previous static limit of 4MB, allowing
>     system administrators to adjust the size based on system-specific
>     requirements or constraints.
>
>     Eg:
>     $ sysctl -a | grep core_file_note_size_max
>     kernel.core_file_note_size_max = 4194304
> .......
> >
> > >
> > > $ 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. :)
> >
>  Done.
>
> > >
> > > Signed-off-by: Vijay Nag <nagvijay@...rosoft.com>
> > > Signed-off-by: Allen Pais <apais@...ux.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...
>
>  It does warn, but if I leave it as a single line, there's still a warning:
> WARNING: line length of 135 exceeds 100 columns, which is why I
> split it into multiple lines.
>
> >
> > > +             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.
> >
>
>  Sure, will update it in v4.
>
> > > +
> > >  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;
> >
>
>  core_file_note_size_min will be used in fs/binfmt_elf.c at:
>
>     if (size >= core_file_note_size_min) ,
> did you mean
> static const unsigned int core_file_note_size_allowed =
> CORE_FILE_NOTE_SIZE_MAX;??
> > >

Kees,

 My bad, I misunderstood what you asked for. Here is the final diff,
if it looks fine,
i can send out a v4.

Note, there is a warning issued by checkpatch.pl (WARNING: line length
of 134 exceeds 100 columns)
for the pr_warn_once() and adding const trigger a build
warning(warning: initialization discards
 'const' qualifier from pointer target type), which is why i dropped it.

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 5397b552fbeb..19bd85d1e42e 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,11 @@ 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_allowed) {
+               pr_warn_once("coredump Note size too large: %u (does
kernel.core_file_note_size_min sysctl need adjustment?\n", 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..69085bb494dc 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 CORE_FILE_NOTE_SIZE_DEFAULT (4*1024*1024)
+/* Define a reasonable max cap */
+#define CORE_FILE_NOTE_SIZE_MAX (16*1024*1024)
+
 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;
+static unsigned int core_file_note_size_min = CORE_FILE_NOTE_SIZE_DEFAULT;
+unsigned int core_file_note_size_allowed = CORE_FILE_NOTE_SIZE_MAX;

 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_min",
+               .data           = &core_file_note_size_min,
+               .maxlen         = sizeof(unsigned int),
+               .mode           = 0644,
+               .proc_handler   = proc_douintvec_minmax,
+               .extra1         = &core_file_note_size_min,
+               .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..776bde5f9752 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_allowed;
 extern void validate_coredump_safety(void);
 #else
 static inline void validate_coredump_safety(void) {}


Thanks,
Allen
> > >  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.
>
> Sure, will fix it in v4.
> >
> > > +             .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!
>
> Thank you for the reviews. Will send out v4 soon.
>
> --
>        - Allen



-- 
       - Allen

Powered by blists - more mailing lists