Received-SPF: pass (google.com: domain of linux-kernel+bounces-163108-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240429172128.4246-1-apais@linux.microsoft.com> <202404291245.18281A6D@keescook>
In-Reply-To: <202404291245.18281A6D@keescook>
From: Allen <allen.lkml@gmail.com>
Date: Mon, 29 Apr 2024 15:35:01 -0700
Message-ID: <CAOMdWS+k63T9TQ=Zvev-+Q3Zw-wuEUv_f63=YiTx0nK1J9Jfwg@mail.gmail.com>
Subject: Re: [RFC PATCH] fs/coredump: Enable dynamic configuration of max file
 note size
To: Kees Cook <keescook@chromium.org>
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
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Mon, Apr 29, 2024 at 12:49=E2=80=AFPM 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[] =3D {
> >               .mode           =3D 0644,
> >               .proc_handler   =3D proc_dointvec,
> >       },
> > +     {
> > +             .procname       =3D "max_file_note_size",
> > +             .data           =3D &max_file_note_size,
> > +             .maxlen         =3D sizeof(unsigned int),
> > +             .mode           =3D 0644,
> > +             .proc_handler   =3D 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 =3D (2 + 3 * count) * sizeof(data[0]);
  alloc:
-       if (size >=3D MAX_FILE_NOTE_SIZE) /* paranoia check */
+       if (size >=3D core_file_note_size_max) /* paranoia check */
                return -EINVAL;
        size =3D 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] =3D "core";
 static int core_name_size =3D CORENAME_MAX_SIZE;
+unsigned int core_file_note_size_max =3D MAX_FILE_NOTE_SIZE;

 struct core_name {
        char *corename;
@@ -1020,6 +1023,13 @@ static struct ctl_table coredump_sysctls[] =3D {
                .mode           =3D 0644,
                .proc_handler   =3D proc_dointvec,
        },
+       {
+               .procname       =3D "core_file_note_size_max",
+               .data           =3D &core_file_note_size_max,
+               .maxlen         =3D sizeof(unsigned int),
+               .mode           =3D 0644,
+               .proc_handler   =3D 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) {}

Thanks,
Allen