From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 47084C4345F for ; Thu, 2 May 2024 20:04:08 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id AD20C6B0085; Thu, 2 May 2024 16:04:07 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id A82536B0088; Thu, 2 May 2024 16:04:07 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 949E46B0089; Thu, 2 May 2024 16:04:07 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id 7365C6B0085 for ; Thu, 2 May 2024 16:04:07 -0400 (EDT) Received: from smtpin02.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id E97761A0E97 for ; Thu, 2 May 2024 20:04:06 +0000 (UTC) X-FDA: 82074532092.02.41DCDA5 Received: from mail-oi1-f171.google.com (mail-oi1-f171.google.com [209.85.167.171]) by imf15.hostedemail.com (Postfix) with ESMTP id 1B3EDA002C for ; Thu, 2 May 2024 20:04:03 +0000 (UTC) Authentication-Results: imf15.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b="MZ/i2jg9"; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf15.hostedemail.com: domain of allen.lkml@gmail.com designates 209.85.167.171 as permitted sender) smtp.mailfrom=allen.lkml@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1714680244; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=OjQQjpH8NhWGNJDJapsehrY5QM3o3eHZRU+UWPn4Acs=; b=UXiV5sbQAKnYqckXPr0ZQIRDbstfCPvleZmKLjPhi84qZRVg76AChFSRtPlm18v6DNDDiL 7qVgzGkXFv4byPNNtdOH4eCQXnjLwLnZ3mkWMPTwnGf/JzwdPLrPL5FfdvNNrZXGow+WEq +nNRB8L5dUradQbSJVImyDaKeLiXtNU= ARC-Authentication-Results: i=1; imf15.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b="MZ/i2jg9"; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf15.hostedemail.com: domain of allen.lkml@gmail.com designates 209.85.167.171 as permitted sender) smtp.mailfrom=allen.lkml@gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1714680244; a=rsa-sha256; cv=none; b=rXlgejDeWa5NfZ+e993rAErN1omJtd0VaBz11ZcpjwafTdkbFKBE5Zfo92CQ2imc3Qae0c Qv0q76HipMJ7G4yx7sqyz+kQxmoKQPrJ/DN4jrkE+pK0oYznwbain5wO7YPjHVXhta7Z9n 0JzN9DimrjvrzuVn8Vxu9Y3HqF2dki4= Received: by mail-oi1-f171.google.com with SMTP id 5614622812f47-3c7498041cfso4786544b6e.2 for ; Thu, 02 May 2024 13:04:03 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1714680243; x=1715285043; darn=kvack.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=OjQQjpH8NhWGNJDJapsehrY5QM3o3eHZRU+UWPn4Acs=; b=MZ/i2jg9jTPFw5miSnf7JiBXNcWv9Go5Gm6R7b2E8Fk3Me1Z9awrxdmwXBkp5ugJCc nywcqDmH4+Q/cJhbL4FwwAIwYN3o7H1hEZrtz4tkE5KO8ttMBEuSOrj4LowHV6nJCZFu ScL9OcgUWueaev/hUuFkT70rB8wnjyEi2FenysAdeOveqhB29WHR+vNxlrTqrm49CVE2 ixt9gwOYujbII+bTcycssAX0+tUbSCvWCYekm1AP2GJbnx6fppMsJYwIbjZ4gE5um8hk WWW/sQzfIOa6ecFNt6pxoauvUSPQ8mi2OLgqGmVtkLI5ujL1eSYKtwS7onLQFlgGLqFj Eilw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1714680243; x=1715285043; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=OjQQjpH8NhWGNJDJapsehrY5QM3o3eHZRU+UWPn4Acs=; b=Ef0B1CevMnKhokPqR/+iE2l2klxmS2RGKT/jcFwQ5IDBxDqdm/PGTs/Sr0alwvXSsU i6Te57tiLnrgJt4qNkJuEmklDusVxjk4L3wV84Hir1xb5BJw5R/UmOC+ZROxb93K/DJ6 awKgl77VasuaTQu9uSb4nawXy8g9shLhB1RUtHBc1VN+0Xgz/brEdbfmcfgjDBL3/qJS qyifSLR57EbTou6BYiARs5BFKAYDO8RF6bO9lEcQkix6xQzxRzJeI34o56aounB7ld25 kAgdm6quF6h376MhonKKDsAKv3c/H0cEgiatv5ofRzdjyAEloq//+GG5rhxHLd2FKYI0 T/7g== X-Forwarded-Encrypted: i=1; AJvYcCWVSjh/CdUV/tI3uwc/MXMT/0ef4Bas1Mm5oRbKzPC6lXmx7VfMOkYVaQqsnYjKg5oPQD5UDEfwhA+rpNm0rXzNduA= X-Gm-Message-State: AOJu0YyOUmKpwQisLvFS3vEKtKPECu3WPP7ndFSqabGZWgp/RGw1plV5 Q6FZT0d41kVRkSprsk1kVbEfWYgVn3GCL9pKf6uCbAalSQudkhovYwFeaxUGeT0fyqZnbCZcB1V ce7oqohHuea0EOyyCWmkTty1WbCU= X-Google-Smtp-Source: AGHT+IF5LRau2j9XR/7D0bXq9AYjJs5NDRapLGLlplnxUEZH7AF5vTaffEWNENripCd5tT/bMf3GYCkhnbdiAwDX+Ek= X-Received: by 2002:a05:6808:1898:b0:3c8:7599:d6a3 with SMTP id bi24-20020a056808189800b003c87599d6a3mr1174647oib.11.1714680243011; Thu, 02 May 2024 13:04:03 -0700 (PDT) MIME-Version: 1.0 References: <20240502145920.5011-1-apais@linux.microsoft.com> <202405021045.360F5313EA@keescook> In-Reply-To: <202405021045.360F5313EA@keescook> From: Allen Date: Thu, 2 May 2024 13:03:52 -0700 Message-ID: Subject: Re: [PATCH v2] fs/coredump: Enable dynamic configuration of max file note size To: Kees Cook Cc: Allen Pais , 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 X-Stat-Signature: xyx3d917roqtw8xmejxoihhoxh1dde33 X-Rspamd-Queue-Id: 1B3EDA002C X-Rspam-User: X-Rspamd-Server: rspam12 X-HE-Tag: 1714680243-543817 X-HE-Meta: U2FsdGVkX1/mVr9HdeSIkMr9Km8HnGM4HcpOlm/IDW/X0oCp6GO9LoBhhp7AGZPN8TYmUTkXmYN9fJ6/LXTuiseDFSScjxM01Imog9Km+wRPWGpMksr/6Ol7ywxJM/js+mK2RxNvp1yvAlmQmZ3wV6Dc3RwUDN8t8cv7dw+vwsAF/l5UBYH2tpwVwRZ8FvO93OGDEvaxu/GiZ+72BoZUFBKWo/b4eZ3ZYfpii2ti08QcfOZgnkSRe1KC3Wm7PK/p45XhA19/SdZ3FdvKyDJbVPoJG6sV4gD21kK6O6z303PEcBTClZaKuN+Qey2NNlrEypS0DKfMYxrSyJEt2zkdpojSuXLCwV1ru7E19Um3t+iZYDZzsn0k6gaO5XS+r3drBpvm3mYzLrvSbannHrXSVqM44xKbC8lnQg8czUDrHS9LAPkbfWhJSJvy5lZso3p9t+6Qofj47NtiEPFqogUXMjklKD/fo46MxgRRuDvmeMcwlaBLsk/uvXEso+bR+1t2em+K6XMM3cnQutvTQe7ZehBCy8FOHmtWuhpexDegjRXdFUCSKKmy1sGLNEtX69ePe+3/rS3RoU57UTzMQPcUs6RkHwuT1apwAn6LaCN5pumDxJNExx+LCUcZEytDe+q1LFzPRFNe+7kNl+AKcDqr8lhblGFbsHhZe9XPxTuETM8NK7gA2mOLgCNqF/n2x+CKPuCfppJ5g3e6BHa5iqDvHTsCw8CJe/mq6V6z1pcVFT9dpu3RenlOvtN87P0cVI7CGzJTGrWAXKwDqTmbl1Ya2MJORhroi0rnp8B7Yqt91KvUNwYcK+JQWKwT0IMTiivHN4hDCj2k4gbHnUXamvAcllaVsblSdyYOfkzlcx/tpXlAfcgT6rge+VeomBC3yBXvTaFmLeyAGsf/zhQ7uy/UYWasUnd7lYIrbb3/TlnGhniSR7gFA0lE33CDajEuYRyTAmJoBFZRUg3HbFfkE2R SOI0/NuQ vHG3iYYJZ3VSyAUaDMzB2GaKOuga8sL3WzKY3gROOmtbhfM5AZs/8hK5wn1HZNM2OamTdv3Eq36XRf7L7kkLgnyxY2l7/jPpvW+mI7PtN7eysT/MdbMYkjaQZTChuYjBJbSWYf8coh0vteZv1l7kJX9pxDyC7OPw2pzXhEquOmXOUHa4zMPooaqihJFFlnvRUn3JuMCgZAbRH2s+1Dk4d64quPy16moBXezPi/Wg7COW6aE0yK0qTzdQX9j4a9JumVGSTfglsFrg0snyEBGUn95jURjfuYLgRbc7hIBHTY8XqBlloLsQ016SZDmdaC5IKX7HxrsPGBdKhC9wZIOdVK7LLUHZnKoQKhBFve39e58yTBy9w67UNcJYr73TK65WtH+ebeYamTp8eeuueZUjRcNlT0Ba0hyNnqMwJD95I4iYsTuwzVKo/QcywoIDMGuC2oL7nXhvtwyrfZ/VRCd588nR1mw== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On Thu, May 2, 2024 at 10:50=E2=80=AFAM Kees Cook w= rote: > > 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 =3D 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 sectio= n > > becomes a bottleneck. The notes section quickly reaches its capacity, > > leading to incomplete memory segment information in the resulting cored= ump. > > 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 > > Signed-off-by: Allen Pais > > > > --- > > 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 & Ke= es] > > --- > > 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 *not= e, 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; > > 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 >=3D core_file_note_size_max) { > pr_warn_once("coredump Note size too large: %zu (does ker= nel.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 ca= p ..... + { + .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_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 =3D 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 =3D 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)? Thanks, Allen > > > size =3D round_up(size, PAGE_SIZE); > > /* > > diff --git a/fs/coredump.c b/fs/coredump.c > > index be6403b4b14b..a312be48030f 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) {} > > -- > > 2.17.1 > > Otherwise, yes, this looks good to me. > > -- > Kees Cook --=20 - Allen