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 v2] fs/coredump: Enable dynamic configuration of max file note size
Date: Thu, 2 May 2024 15:47:28 -0700 [thread overview]
Message-ID: <202405021545.4A3ACDED0@keescook> (raw)
In-Reply-To: <CAOMdWSJzXiqB5tusdKaavJFTaKC-qyArT0ssRHVY-fvZVKJW+Q@mail.gmail.com>
On Thu, May 02, 2024 at 01:03:52PM -0700, Allen wrote:
> On Thu, May 2, 2024 at 10:50 AM Kees Cook <keescook@chromium.org> wrote:
> >
> > On Thu, May 02, 2024 at 02:59:20PM +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.
> > >
> > > $ sysctl -a | grep max_file_note_size
> > > kernel.max_file_note_size = 4194304
> > >
> > > $ sysctl -n kernel.max_file_note_size
> > > 4194304
> > >
> > > $echo 519304 > /proc/sys/kernel/max_file_note_size
> > >
> > > $sysctl -n kernel.max_file_note_size
> > > 519304
> >
> > The names and paths in the commit log need a refresh here, since they've
> > changed.
>
> Will fix it in v3.
> >
> > >
> > > 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.
> >
> > Thanks for adding this!
> >
> > >
> > > Signed-off-by: Vijay Nag <nagvijay@microsoft.com>
> > > Signed-off-by: Allen Pais <apais@linux.microsoft.com>
> > >
> > > ---
> > > 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 | 3 +--
> > > fs/coredump.c | 10 ++++++++++
> > > include/linux/coredump.h | 1 +
> > > 3 files changed, 12 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> > > index 5397b552fbeb..6aebd062b92b 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,7 +1591,7 @@ 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 */
> > > + if (size >= core_file_note_size_max) /* paranoia check */
> > > return -EINVAL;
> >
> > I wonder, given the purpose of this sysctl, if it would be a
> > discoverability improvement to include a pr_warn_once() before the
> > EINVAL? Like:
> >
> > /* paranoia check */
> > if (size >= core_file_note_size_max) {
> > pr_warn_once("coredump Note size too large: %zu (does kernel.core_file_note_size_max sysctl need adjustment?\n", size);
> > return -EINVAL;
> > }
> >
> > What do folks think? (I can't imagine tracking down this problem
> > originally was much fun, for example.)
>
> I think this would really be helpful. I will go ahead and add this if
> there's no objection from anyone.
>
> Also, I haven't received a reply from Luis, do you think we need to
> add a ceiling?
>
> +#define MAX_FILE_NOTE_SIZE (4*1024*1024)
> +#define MAX_ALLOWED_NOTE_SIZE (16*1024*1024) // Define a reasonable max cap
> .....
>
> + {
> + .procname = "core_file_note_size_max",
> + .data = &core_file_note_size_max,
> + .maxlen = sizeof(unsigned int),
> + .mode = 0644,
> + .proc_handler = proc_core_file_note_size_max,
> + },
> };
>
> +int proc_core_file_note_size_max(struct ctl_table *table, int write,
> void __user *buffer, size_t *lenp, loff_t *ppos) {
> + int error = proc_douintvec(table, write, buffer, lenp, ppos);
> + if (write && (core_file_note_size_max < MAX_FILE_NOTE_SIZE ||
> core_file_note_size_max > MAX_ALLOWED_NOTE_SIZE))
> + core_file_note_size_max = MAX_FILE_NOTE_SIZE; // Revert to
> default if out of bounds
> + return error;
> +}
>
>
> Or, should we go ahead with the current patch(with the warning added)?
Let's add a ceiling just to avoid really pathological behavior. We got
this far with 4M, so having a new ceiling seems reasonable. And for
implementing it, see proc_douintvec_minmax.
-Kees
--
Kees Cook
prev parent reply other threads:[~2024-05-02 22:47 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-02 14:59 [PATCH v2] fs/coredump: Enable dynamic configuration of max file note size Allen Pais
2024-05-02 17:50 ` Kees Cook
2024-05-02 20:03 ` Allen
2024-05-02 21:35 ` Luis Chamberlain
2024-05-02 22:47 ` Kees Cook [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=202405021545.4A3ACDED0@keescook \
--to=keescook@chromium.org \
--cc=allen.lkml@gmail.com \
--cc=apais@linux.microsoft.com \
--cc=brauner@kernel.org \
--cc=ebiederm@xmission.com \
--cc=j.granados@samsung.com \
--cc=jack@suse.cz \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mcgrof@kernel.org \
--cc=viro@zeniv.linux.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).