Received-SPF: pass (google.com: domain of linux-kernel+bounces-167116-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Thu, 2 May 2024 17:49:43 -0700
From: Kees Cook <keescook@chromium.org>
To: Allen Pais <apais@linux.microsoft.com>
Cc: 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, allen.lkml@gmail.com
Subject: Re: [PATCH v3] fs/coredump: Enable dynamic configuration of max file
 note size
Message-ID: <202405021743.D06C96516@keescook>
References: <20240502235603.19290-1-apais@linux.microsoft.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240502235603.19290-1-apais@linux.microsoft.com>

On Thu, May 02, 2024 at 11:56:03PM +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.

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.

> 
> $ 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. :)

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

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

> +
>  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;

>  
>  struct core_name {
>  	char *corename;
> @@ -1020,6 +1026,15 @@ 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_minmax,
> +		.extra1		= &core_file_note_size_max,

This means you can never shrink it if you raise it from the default.
Let's use the core_file_note_size_min above.

> +		.extra2		= &core_file_note_size_allowed,
> +	},
>  };
>  
>  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) {}
> -- 
> 2.17.1
> 

I think v4 will be all good to go, assuming no one else pops up. :)
Thanks for the changes!

-Kees

-- 
Kees Cook