* [RFC PATCH] fs/coredump: Enable dynamic configuration of max file note size
@ 2024-04-29 17:21 Allen Pais
2024-04-29 18:38 ` Luis Chamberlain
2024-04-29 19:49 ` Kees Cook
0 siblings, 2 replies; 7+ messages in thread
From: Allen Pais @ 2024-04-29 17:21 UTC (permalink / raw
To: linux-fsdevel
Cc: linux-kernel, linux-mm, viro, brauner, jack, ebiederm, keescook,
mcgrof, j.granados
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
Signed-off-by: Vijay Nag <nagvijay@microsoft.com>
Signed-off-by: Allen Pais <apais@linux.microsoft.com>
---
fs/binfmt_elf.c | 3 +--
fs/coredump.c | 3 +++
include/linux/coredump.h | 1 +
kernel/sysctl.c | 8 ++++++++
4 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 5397b552fbeb..5fc7baa9ebf2 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 >= max_file_note_size) /* paranoia check */
return -EINVAL;
size = round_up(size, PAGE_SIZE);
/*
diff --git a/fs/coredump.c b/fs/coredump.c
index be6403b4b14b..a83c6cc893fc 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 max_file_note_size = MAX_FILE_NOTE_SIZE;
struct core_name {
char *corename;
diff --git a/include/linux/coredump.h b/include/linux/coredump.h
index d3eba4360150..e1ae7ab33d76 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 max_file_note_size;
extern void validate_coredump_safety(void);
#else
static inline void validate_coredump_safety(void) {}
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,
+ },
#ifdef CONFIG_PROC_SYSCTL
{
.procname = "tainted",
--
2.17.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [RFC PATCH] fs/coredump: Enable dynamic configuration of max file note size
2024-04-29 17:21 [RFC PATCH] fs/coredump: Enable dynamic configuration of max file note size Allen Pais
@ 2024-04-29 18:38 ` Luis Chamberlain
2024-04-29 22:32 ` Allen
2024-04-29 19:49 ` Kees Cook
1 sibling, 1 reply; 7+ messages in thread
From: Luis Chamberlain @ 2024-04-29 18:38 UTC (permalink / raw
To: Allen Pais
Cc: linux-fsdevel, linux-kernel, linux-mm, viro, brauner, jack,
ebiederm, keescook, j.granados
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.
>
> - 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
This doesn't highlight anything about *why*. So in practice you must've
hit a use case where ELF notes are huge, can you give an example of
that? The commit should also describe that this is only used in the path
of a coredump on ELF binaries via elf_core_dump().
More below.
> Signed-off-by: Vijay Nag <nagvijay@microsoft.com>
> Signed-off-by: Allen Pais <apais@linux.microsoft.com>
> ---
> fs/binfmt_elf.c | 3 +--
> fs/coredump.c | 3 +++
> include/linux/coredump.h | 1 +
> kernel/sysctl.c | 8 ++++++++
> 4 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index 5397b552fbeb..5fc7baa9ebf2 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 >= max_file_note_size) /* paranoia check */
> return -EINVAL;
> size = round_up(size, PAGE_SIZE);
> /*
> diff --git a/fs/coredump.c b/fs/coredump.c
> index be6403b4b14b..a83c6cc893fc 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 max_file_note_size = MAX_FILE_NOTE_SIZE;
>
> struct core_name {
> char *corename;
> diff --git a/include/linux/coredump.h b/include/linux/coredump.h
> index d3eba4360150..e1ae7ab33d76 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 max_file_note_size;
> extern void validate_coredump_safety(void);
> #else
> static inline void validate_coredump_safety(void) {}
> 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,
> + },
> #ifdef CONFIG_PROC_SYSCTL
No, please move this to coredump_sysctls in fs/coredump.c. And there is
no point in supporting int, this is unisgned int right? So use the right
proc handler for it.
If we're gonna do this, it makes sense to document the ELF note binary
limiations. Then, consider a defense too, what if a specially crafted
binary with a huge elf note are core dumped many times, what then?
Lifting to 4 MiB puts in a situation where abuse can lead to many silly
insane kvmalloc()s. Is that what we want? Why?
Luis
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH] fs/coredump: Enable dynamic configuration of max file note size
2024-04-29 17:21 [RFC PATCH] fs/coredump: Enable dynamic configuration of max file note size Allen Pais
2024-04-29 18:38 ` Luis Chamberlain
@ 2024-04-29 19:49 ` Kees Cook
2024-04-29 22:35 ` Allen
1 sibling, 1 reply; 7+ messages in thread
From: Kees Cook @ 2024-04-29 19:49 UTC (permalink / raw
To: Allen Pais
Cc: linux-fsdevel, linux-kernel, linux-mm, viro, brauner, jack,
ebiederm, mcgrof, j.granados
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?
> [...]
> 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
--
Kees Cook
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH] fs/coredump: Enable dynamic configuration of max file note size
2024-04-29 18:38 ` Luis Chamberlain
@ 2024-04-29 22:32 ` Allen
2024-04-30 17:51 ` Allen
0 siblings, 1 reply; 7+ messages in thread
From: Allen @ 2024-04-29 22:32 UTC (permalink / raw
To: Luis Chamberlain
Cc: Allen Pais, linux-fsdevel, linux-kernel, linux-mm, viro, brauner,
jack, ebiederm, keescook, j.granados
On Mon, Apr 29, 2024 at 11:38 AM Luis Chamberlain <mcgrof@kernel.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.
> >
> > - 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
>
> This doesn't highlight anything about *why*. So in practice you must've
> hit a use case where ELF notes are huge, can you give an example of
> that? The commit should also describe that this is only used in the path
> of a coredump on ELF binaries via elf_core_dump().
>
Yes, I should have captured it. 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 will add the above to the commit message. Hope that addresses your concern.
> More below.
>
> > Signed-off-by: Vijay Nag <nagvijay@microsoft.com>
> > Signed-off-by: Allen Pais <apais@linux.microsoft.com>
> > ---
> > fs/binfmt_elf.c | 3 +--
> > fs/coredump.c | 3 +++
> > include/linux/coredump.h | 1 +
> > kernel/sysctl.c | 8 ++++++++
> > 4 files changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> > index 5397b552fbeb..5fc7baa9ebf2 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 >= max_file_note_size) /* paranoia check */
> > return -EINVAL;
> > size = round_up(size, PAGE_SIZE);
> > /*
> > diff --git a/fs/coredump.c b/fs/coredump.c
> > index be6403b4b14b..a83c6cc893fc 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 max_file_note_size = MAX_FILE_NOTE_SIZE;
> >
> > struct core_name {
> > char *corename;
> > diff --git a/include/linux/coredump.h b/include/linux/coredump.h
> > index d3eba4360150..e1ae7ab33d76 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 max_file_note_size;
> > extern void validate_coredump_safety(void);
> > #else
> > static inline void validate_coredump_safety(void) {}
> > 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,
> > + },
> > #ifdef CONFIG_PROC_SYSCTL
>
> No, please move this to coredump_sysctls in fs/coredump.c. And there is
> no point in supporting int, this is unisgned int right? So use the right
> proc handler for it.
>
Will address it in v2.
> If we're gonna do this, it makes sense to document the ELF note binary
> limiations. Then, consider a defense too, what if a specially crafted
> binary with a huge elf note are core dumped many times, what then?
> Lifting to 4 MiB puts in a situation where abuse can lead to many silly
> insane kvmalloc()s. Is that what we want? Why?
>
You raise a good point. I need to see how we can safely handle this case.
Thanks,
Allen
> Luis
--
- Allen
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH] fs/coredump: Enable dynamic configuration of max file note size
2024-04-29 19:49 ` Kees Cook
@ 2024-04-29 22:35 ` Allen
2024-04-30 5:36 ` Kees Cook
0 siblings, 1 reply; 7+ messages in thread
From: Allen @ 2024-04-29 22:35 UTC (permalink / raw
To: Kees Cook
Cc: Allen Pais, linux-fsdevel, linux-kernel, linux-mm, viro, brauner,
jack, ebiederm, mcgrof, j.granados
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) {}
Thanks,
Allen
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [RFC PATCH] fs/coredump: Enable dynamic configuration of max file note size
2024-04-29 22:35 ` Allen
@ 2024-04-30 5:36 ` Kees Cook
0 siblings, 0 replies; 7+ messages in thread
From: Kees Cook @ 2024-04-30 5:36 UTC (permalink / raw
To: Allen
Cc: Allen Pais, linux-fsdevel, linux-kernel, linux-mm, viro, brauner,
jack, ebiederm, mcgrof, j.granados
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH] fs/coredump: Enable dynamic configuration of max file note size
2024-04-29 22:32 ` Allen
@ 2024-04-30 17:51 ` Allen
0 siblings, 0 replies; 7+ messages in thread
From: Allen @ 2024-04-30 17:51 UTC (permalink / raw
To: Luis Chamberlain
Cc: Allen Pais, linux-fsdevel, linux-kernel, linux-mm, viro, brauner,
jack, ebiederm, keescook, j.granados
> Will address it in v2.
>
> > If we're gonna do this, it makes sense to document the ELF note binary
> > limiations. Then, consider a defense too, what if a specially crafted
> > binary with a huge elf note are core dumped many times, what then?
> > Lifting to 4 MiB puts in a situation where abuse can lead to many silly
> > insane kvmalloc()s. Is that what we want? Why?
> >
> You raise a good point. I need to see how we can safely handle this case.
>
Luis,
Here's a rough idea that caps the max allowable size for the note section.
I am using 16MB as the max value.
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -56,10 +56,14 @@
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 MAX_ALLOWED_NOTE_SIZE (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;
+unsigned int core_file_note_size_max = MAX_FILE_NOTE_SIZE;
struct core_name {
char *corename;
@@ -1060,12 +1064,22 @@ 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_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))
+. /* Revert to default if out of bounds */
+ core_file_note_size_max = MAX_FILE_NOTE_SIZE;
+ return error;
+}
Let me know what you think.
Thanks,
- Allen
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-04-30 17:51 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-04-29 17:21 [RFC PATCH] fs/coredump: Enable dynamic configuration of max file note size Allen Pais
2024-04-29 18:38 ` Luis Chamberlain
2024-04-29 22:32 ` Allen
2024-04-30 17:51 ` Allen
2024-04-29 19:49 ` Kees Cook
2024-04-29 22:35 ` Allen
2024-04-30 5:36 ` Kees Cook
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).