Re: [RFC PATCH] fs/coredump: Enable dynamic configuration of max file note size

[Kees Cook <keescook@chromium.org>]

On Mon, Apr 29, 2024 at 03:35:01PM -0700, Allen wrote:
> On Mon, Apr 29, 2024 at 12:49 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > On Mon, Apr 29, 2024 at 05:21:28PM +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.
> >
> > Under what conditions is this actually needed?
> 
>  I addressed this in the email I sent out before this.
> 
> >
> > > [...]
> > > diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> > > index 81cc974913bb..80cdc37f2fa2 100644
> > > --- a/kernel/sysctl.c
> > > +++ b/kernel/sysctl.c
> > > @@ -63,6 +63,7 @@
> > >  #include <linux/mount.h>
> > >  #include <linux/userfaultfd_k.h>
> > >  #include <linux/pid.h>
> > > +#include <linux/coredump.h>
> > >
> > >  #include "../lib/kstrtox.h"
> > >
> > > @@ -1623,6 +1624,13 @@ static struct ctl_table kern_table[] = {
> > >               .mode           = 0644,
> > >               .proc_handler   = proc_dointvec,
> > >       },
> > > +     {
> > > +             .procname       = "max_file_note_size",
> > > +             .data           = &max_file_note_size,
> > > +             .maxlen         = sizeof(unsigned int),
> > > +             .mode           = 0644,
> > > +             .proc_handler   = proc_dointvec,
> > > +     },
> >
> > Please don't add new sysctls to kernel/sysctl.c. Put this in fs/coredump.c
> > instead, and name it "core_file_note_size_max". (A "max" suffix is more
> > common than prefixes, and I'd like it clarified that it relates to the
> > coredumper with the "core" prefix that match the other coredump sysctls.
> >
> > -Kees
> 
> Makes sense. Let me know if the below looks fine,
> 
> 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;
>         size = round_up(size, PAGE_SIZE);
>         /*
> diff --git a/fs/coredump.c b/fs/coredump.c
> index be6403b4b14b..2108eb93acb9 100644
> --- a/fs/coredump.c
> +++ b/fs/coredump.c
> @@ -56,10 +56,13 @@
>  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)
> +
>  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;
> 
>  struct core_name {
>         char *corename;
> @@ -1020,6 +1023,13 @@ 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,
> +       },
>  };
> 
>  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) {}

Yes; thank you! I like this naming (and sysctl location) now. :)

-Kees

-- 
Kees Cook