Received-SPF: pass (google.com: domain of linux-kernel+bounces-168413-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Fri, 3 May 2024 16:31:55 -0700
From: Kees Cook <keescook@chromium.org>
To: Allen <allen.lkml@gmail.com>
Cc: Allen Pais <apais@linux.microsoft.com>, linux-fsdevel@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-mm@kvack.org,
	viro@zeniv.linux.org.uk, brauner@kernel.org, jack@suse.cz,
	ebiederm@xmission.com, mcgrof@kernel.org, j.granados@samsung.com
Subject: Re: [PATCH v3] fs/coredump: Enable dynamic configuration of max file
 note size
Message-ID: <202405031629.D95BE0F@keescook>
References: <20240502235603.19290-1-apais@linux.microsoft.com>
 <202405021743.D06C96516@keescook>
 <CAOMdWSLa-dp34aq3RepQABpnGs-TnyQgUxFm--MHVHFuVYTgFg@mail.gmail.com>
 <CAOMdWSLh80OHx=som1WeK_L_=2LVK1rehXH88t3Ew--4dSE4ew@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAOMdWSLh80OHx=som1WeK_L_=2LVK1rehXH88t3Ew--4dSE4ew@mail.gmail.com>

On Thu, May 02, 2024 at 06:40:58PM -0700, Allen 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.
> >
> > 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@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...
> >
> >  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 strings that should be fine. You'll want the ", size);" part on the
next line though.

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

Yeah, that's a common pattern for sysctl. You can fix it with a cast.
For example:

static const unsigned long      nlm_grace_period_min = 0;
static const unsigned long      nlm_grace_period_max = 240;
..
                .proc_handler   = proc_doulongvec_minmax,
                .extra1         = (unsigned long *) &nlm_grace_period_min,
                .extra2         = (unsigned long *) &nlm_grace_period_max,


But yeah, looks good.

-- 
Kees Cook