From c114e9948c2b6a0b400266e59cc656b59e795bca Mon Sep 17 00:00:00 2001 From: Roman Kisel Date: Thu, 18 Jul 2024 11:27:24 -0700 Subject: [PATCH 1/4] coredump: Standartize and fix logging The coredump code does not log the process ID and the comm consistently, logs unescaped comm when it does log it, and does not always use the ratelimited logging. That makes it harder to analyze logs and puts the system at the risk of spamming the system log incase something crashes many times over and over again. Fix that by logging TGID and comm (escaped) consistently and using the ratelimited logging always. Signed-off-by: Roman Kisel Tested-by: Allen Pais Link: https://lore.kernel.org/r/20240718182743.1959160-2-romank@linux.microsoft.com Signed-off-by: Kees Cook --- fs/coredump.c | 43 +++++++++++++++------------------------- include/linux/coredump.h | 22 ++++++++++++++++++++ 2 files changed, 38 insertions(+), 27 deletions(-) diff --git a/fs/coredump.c b/fs/coredump.c index 7f12ff6ad1d3..87ff71a59fbe 100644 --- a/fs/coredump.c +++ b/fs/coredump.c @@ -586,8 +586,7 @@ void do_coredump(const kernel_siginfo_t *siginfo) struct subprocess_info *sub_info; if (ispipe < 0) { - printk(KERN_WARNING "format_corename failed\n"); - printk(KERN_WARNING "Aborting core\n"); + coredump_report_failure("format_corename failed, aborting core"); goto fail_unlock; } @@ -607,27 +606,21 @@ void do_coredump(const kernel_siginfo_t *siginfo) * right pid if a thread in a multi-threaded * core_pattern process dies. */ - printk(KERN_WARNING - "Process %d(%s) has RLIMIT_CORE set to 1\n", - task_tgid_vnr(current), current->comm); - printk(KERN_WARNING "Aborting core\n"); + coredump_report_failure("RLIMIT_CORE is set to 1, aborting core"); goto fail_unlock; } cprm.limit = RLIM_INFINITY; dump_count = atomic_inc_return(&core_dump_count); if (core_pipe_limit && (core_pipe_limit < dump_count)) { - printk(KERN_WARNING "Pid %d(%s) over core_pipe_limit\n", - task_tgid_vnr(current), current->comm); - printk(KERN_WARNING "Skipping core dump\n"); + coredump_report_failure("over core_pipe_limit, skipping core dump"); goto fail_dropcount; } helper_argv = kmalloc_array(argc + 1, sizeof(*helper_argv), GFP_KERNEL); if (!helper_argv) { - printk(KERN_WARNING "%s failed to allocate memory\n", - __func__); + coredump_report_failure("%s failed to allocate memory", __func__); goto fail_dropcount; } for (argi = 0; argi < argc; argi++) @@ -644,8 +637,7 @@ void do_coredump(const kernel_siginfo_t *siginfo) kfree(helper_argv); if (retval) { - printk(KERN_INFO "Core dump to |%s pipe failed\n", - cn.corename); + coredump_report_failure("|%s pipe failed", cn.corename); goto close_fail; } } else { @@ -658,10 +650,8 @@ void do_coredump(const kernel_siginfo_t *siginfo) goto fail_unlock; if (need_suid_safe && cn.corename[0] != '/') { - printk(KERN_WARNING "Pid %d(%s) can only dump core "\ - "to fully qualified path!\n", - task_tgid_vnr(current), current->comm); - printk(KERN_WARNING "Skipping core dump\n"); + coredump_report_failure( + "this process can only dump core to a fully qualified path, skipping core dump"); goto fail_unlock; } @@ -730,13 +720,13 @@ void do_coredump(const kernel_siginfo_t *siginfo) idmap = file_mnt_idmap(cprm.file); if (!vfsuid_eq_kuid(i_uid_into_vfsuid(idmap, inode), current_fsuid())) { - pr_info_ratelimited("Core dump to %s aborted: cannot preserve file owner\n", - cn.corename); + coredump_report_failure("Core dump to %s aborted: " + "cannot preserve file owner", cn.corename); goto close_fail; } if ((inode->i_mode & 0677) != 0600) { - pr_info_ratelimited("Core dump to %s aborted: cannot preserve file permissions\n", - cn.corename); + coredump_report_failure("Core dump to %s aborted: " + "cannot preserve file permissions", cn.corename); goto close_fail; } if (!(cprm.file->f_mode & FMODE_CAN_WRITE)) @@ -757,7 +747,7 @@ void do_coredump(const kernel_siginfo_t *siginfo) * have this set to NULL. */ if (!cprm.file) { - pr_info("Core dump to |%s disabled\n", cn.corename); + coredump_report_failure("Core dump to |%s disabled", cn.corename); goto close_fail; } if (!dump_vma_snapshot(&cprm)) @@ -983,11 +973,10 @@ void validate_coredump_safety(void) { if (suid_dumpable == SUID_DUMP_ROOT && core_pattern[0] != '/' && core_pattern[0] != '|') { - pr_warn( -"Unsafe core_pattern used with fs.suid_dumpable=2.\n" -"Pipe handler or fully qualified core dump path required.\n" -"Set kernel.core_pattern before fs.suid_dumpable.\n" - ); + + coredump_report_failure("Unsafe core_pattern used with fs.suid_dumpable=2: " + "pipe handler or fully qualified core dump path required. " + "Set kernel.core_pattern before fs.suid_dumpable."); } } diff --git a/include/linux/coredump.h b/include/linux/coredump.h index 0904ba010341..45e598fe3476 100644 --- a/include/linux/coredump.h +++ b/include/linux/coredump.h @@ -43,8 +43,30 @@ extern int dump_align(struct coredump_params *cprm, int align); int dump_user_range(struct coredump_params *cprm, unsigned long start, unsigned long len); extern void do_coredump(const kernel_siginfo_t *siginfo); + +/* + * Logging for the coredump code, ratelimited. + * The TGID and comm fields are added to the message. + */ + +#define __COREDUMP_PRINTK(Level, Format, ...) \ + do { \ + char comm[TASK_COMM_LEN]; \ + \ + get_task_comm(comm, current); \ + printk_ratelimited(Level "coredump: %d(%*pE): " Format "\n", \ + task_tgid_vnr(current), (int)strlen(comm), comm, ##__VA_ARGS__); \ + } while (0) \ + +#define coredump_report(fmt, ...) __COREDUMP_PRINTK(KERN_INFO, fmt, ##__VA_ARGS__) +#define coredump_report_failure(fmt, ...) __COREDUMP_PRINTK(KERN_WARNING, fmt, ##__VA_ARGS__) + #else static inline void do_coredump(const kernel_siginfo_t *siginfo) {} + +#define coredump_report(...) +#define coredump_report_failure(...) + #endif #if defined(CONFIG_COREDUMP) && defined(CONFIG_SYSCTL) From fb97d2eb542faf19a8725afbd75cbc2518903210 Mon Sep 17 00:00:00 2001 From: Roman Kisel Date: Thu, 18 Jul 2024 11:27:25 -0700 Subject: [PATCH 2/4] binfmt_elf, coredump: Log the reason of the failed core dumps Missing, failed, or corrupted core dumps might impede crash investigations. To improve reliability of that process and consequently the programs themselves, one needs to trace the path from producing a core dumpfile to analyzing it. That path starts from the core dump file written to the disk by the kernel or to the standard input of a user mode helper program to which the kernel streams the coredump contents. There are cases where the kernel will interrupt writing the core out or produce a truncated/not-well-formed core dump without leaving a note. Add logging for the core dump collection failure paths to be able to reason what has gone wrong when the core dump is malformed or missing. Report the size of the data written to aid in diagnosing the user mode helper. Signed-off-by: Roman Kisel Link: https://lore.kernel.org/r/20240718182743.1959160-3-romank@linux.microsoft.com Signed-off-by: Kees Cook --- fs/binfmt_elf.c | 48 +++++++++++++----- fs/coredump.c | 107 ++++++++++++++++++++++++++++++++------- include/linux/coredump.h | 8 ++- kernel/signal.c | 21 +++++++- 4 files changed, 150 insertions(+), 34 deletions(-) diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c index 19fa49cd9907..bf9bfd1a0007 100644 --- a/fs/binfmt_elf.c +++ b/fs/binfmt_elf.c @@ -2027,8 +2027,10 @@ static int elf_core_dump(struct coredump_params *cprm) * Collect all the non-memory information about the process for the * notes. This also sets up the file header. */ - if (!fill_note_info(&elf, e_phnum, &info, cprm)) + if (!fill_note_info(&elf, e_phnum, &info, cprm)) { + coredump_report_failure("Error collecting note info"); goto end_coredump; + } has_dumped = 1; @@ -2043,8 +2045,10 @@ static int elf_core_dump(struct coredump_params *cprm) sz += elf_coredump_extra_notes_size(); phdr4note = kmalloc(sizeof(*phdr4note), GFP_KERNEL); - if (!phdr4note) + if (!phdr4note) { + coredump_report_failure("Error allocating program headers note entry"); goto end_coredump; + } fill_elf_note_phdr(phdr4note, sz, offset); offset += sz; @@ -2058,18 +2062,24 @@ static int elf_core_dump(struct coredump_params *cprm) if (e_phnum == PN_XNUM) { shdr4extnum = kmalloc(sizeof(*shdr4extnum), GFP_KERNEL); - if (!shdr4extnum) + if (!shdr4extnum) { + coredump_report_failure("Error allocating extra program headers"); goto end_coredump; + } fill_extnum_info(&elf, shdr4extnum, e_shoff, segs); } offset = dataoff; - if (!dump_emit(cprm, &elf, sizeof(elf))) + if (!dump_emit(cprm, &elf, sizeof(elf))) { + coredump_report_failure("Error emitting the ELF headers"); goto end_coredump; + } - if (!dump_emit(cprm, phdr4note, sizeof(*phdr4note))) + if (!dump_emit(cprm, phdr4note, sizeof(*phdr4note))) { + coredump_report_failure("Error emitting the program header for notes"); goto end_coredump; + } /* Write program headers for segments dump */ for (i = 0; i < cprm->vma_count; i++) { @@ -2092,20 +2102,28 @@ static int elf_core_dump(struct coredump_params *cprm) phdr.p_flags |= PF_X; phdr.p_align = ELF_EXEC_PAGESIZE; - if (!dump_emit(cprm, &phdr, sizeof(phdr))) + if (!dump_emit(cprm, &phdr, sizeof(phdr))) { + coredump_report_failure("Error emitting program headers"); goto end_coredump; + } } - if (!elf_core_write_extra_phdrs(cprm, offset)) + if (!elf_core_write_extra_phdrs(cprm, offset)) { + coredump_report_failure("Error writing out extra program headers"); goto end_coredump; + } /* write out the notes section */ - if (!write_note_info(&info, cprm)) + if (!write_note_info(&info, cprm)) { + coredump_report_failure("Error writing out notes"); goto end_coredump; + } /* For cell spufs */ - if (elf_coredump_extra_notes_write(cprm)) + if (elf_coredump_extra_notes_write(cprm)) { + coredump_report_failure("Error writing out extra notes"); goto end_coredump; + } /* Align to page */ dump_skip_to(cprm, dataoff); @@ -2113,16 +2131,22 @@ static int elf_core_dump(struct coredump_params *cprm) for (i = 0; i < cprm->vma_count; i++) { struct core_vma_metadata *meta = cprm->vma_meta + i; - if (!dump_user_range(cprm, meta->start, meta->dump_size)) + if (!dump_user_range(cprm, meta->start, meta->dump_size)) { + coredump_report_failure("Error writing out the process memory"); goto end_coredump; + } } - if (!elf_core_write_extra_data(cprm)) + if (!elf_core_write_extra_data(cprm)) { + coredump_report_failure("Error writing out extra data"); goto end_coredump; + } if (e_phnum == PN_XNUM) { - if (!dump_emit(cprm, shdr4extnum, sizeof(*shdr4extnum))) + if (!dump_emit(cprm, shdr4extnum, sizeof(*shdr4extnum))) { + coredump_report_failure("Error emitting extra program headers"); goto end_coredump; + } } end_coredump: diff --git a/fs/coredump.c b/fs/coredump.c index 87ff71a59fbe..5814a6d781ce 100644 --- a/fs/coredump.c +++ b/fs/coredump.c @@ -464,7 +464,17 @@ static bool dump_interrupted(void) * but then we need to teach dump_write() to restart and clear * TIF_SIGPENDING. */ - return fatal_signal_pending(current) || freezing(current); + if (fatal_signal_pending(current)) { + coredump_report_failure("interrupted: fatal signal pending"); + return true; + } + + if (freezing(current)) { + coredump_report_failure("interrupted: freezing"); + return true; + } + + return false; } static void wait_for_dump_helpers(struct file *file) @@ -519,7 +529,7 @@ static int umh_pipe_setup(struct subprocess_info *info, struct cred *new) return err; } -void do_coredump(const kernel_siginfo_t *siginfo) +int do_coredump(const kernel_siginfo_t *siginfo) { struct core_state core_state; struct core_name cn; @@ -527,7 +537,7 @@ void do_coredump(const kernel_siginfo_t *siginfo) struct linux_binfmt * binfmt; const struct cred *old_cred; struct cred *cred; - int retval = 0; + int retval; int ispipe; size_t *argv = NULL; int argc = 0; @@ -551,14 +561,20 @@ void do_coredump(const kernel_siginfo_t *siginfo) audit_core_dumps(siginfo->si_signo); binfmt = mm->binfmt; - if (!binfmt || !binfmt->core_dump) + if (!binfmt || !binfmt->core_dump) { + retval = -ENOEXEC; goto fail; - if (!__get_dumpable(cprm.mm_flags)) + } + if (!__get_dumpable(cprm.mm_flags)) { + retval = -EACCES; goto fail; + } cred = prepare_creds(); - if (!cred) + if (!cred) { + retval = -EPERM; goto fail; + } /* * We cannot trust fsuid as being the "true" uid of the process * nor do we know its entire history. We only know it was tainted @@ -587,6 +603,7 @@ void do_coredump(const kernel_siginfo_t *siginfo) if (ispipe < 0) { coredump_report_failure("format_corename failed, aborting core"); + retval = ispipe; goto fail_unlock; } @@ -607,6 +624,7 @@ void do_coredump(const kernel_siginfo_t *siginfo) * core_pattern process dies. */ coredump_report_failure("RLIMIT_CORE is set to 1, aborting core"); + retval = -EPERM; goto fail_unlock; } cprm.limit = RLIM_INFINITY; @@ -614,6 +632,7 @@ void do_coredump(const kernel_siginfo_t *siginfo) dump_count = atomic_inc_return(&core_dump_count); if (core_pipe_limit && (core_pipe_limit < dump_count)) { coredump_report_failure("over core_pipe_limit, skipping core dump"); + retval = -E2BIG; goto fail_dropcount; } @@ -621,6 +640,7 @@ void do_coredump(const kernel_siginfo_t *siginfo) GFP_KERNEL); if (!helper_argv) { coredump_report_failure("%s failed to allocate memory", __func__); + retval = -ENOMEM; goto fail_dropcount; } for (argi = 0; argi < argc; argi++) @@ -646,12 +666,16 @@ void do_coredump(const kernel_siginfo_t *siginfo) int open_flags = O_CREAT | O_WRONLY | O_NOFOLLOW | O_LARGEFILE | O_EXCL; - if (cprm.limit < binfmt->min_coredump) + if (cprm.limit < binfmt->min_coredump) { + coredump_report_failure("over coredump resource limit, skipping core dump"); + retval = -E2BIG; goto fail_unlock; + } if (need_suid_safe && cn.corename[0] != '/') { coredump_report_failure( "this process can only dump core to a fully qualified path, skipping core dump"); + retval = -EPERM; goto fail_unlock; } @@ -697,20 +721,28 @@ void do_coredump(const kernel_siginfo_t *siginfo) } else { cprm.file = filp_open(cn.corename, open_flags, 0600); } - if (IS_ERR(cprm.file)) + if (IS_ERR(cprm.file)) { + retval = PTR_ERR(cprm.file); goto fail_unlock; + } inode = file_inode(cprm.file); - if (inode->i_nlink > 1) + if (inode->i_nlink > 1) { + retval = -EMLINK; goto close_fail; - if (d_unhashed(cprm.file->f_path.dentry)) + } + if (d_unhashed(cprm.file->f_path.dentry)) { + retval = -EEXIST; goto close_fail; + } /* * AK: actually i see no reason to not allow this for named * pipes etc, but keep the previous behaviour for now. */ - if (!S_ISREG(inode->i_mode)) + if (!S_ISREG(inode->i_mode)) { + retval = -EISDIR; goto close_fail; + } /* * Don't dump core if the filesystem changed owner or mode * of the file during file creation. This is an issue when @@ -722,17 +754,22 @@ void do_coredump(const kernel_siginfo_t *siginfo) current_fsuid())) { coredump_report_failure("Core dump to %s aborted: " "cannot preserve file owner", cn.corename); + retval = -EPERM; goto close_fail; } if ((inode->i_mode & 0677) != 0600) { coredump_report_failure("Core dump to %s aborted: " "cannot preserve file permissions", cn.corename); + retval = -EPERM; goto close_fail; } - if (!(cprm.file->f_mode & FMODE_CAN_WRITE)) + if (!(cprm.file->f_mode & FMODE_CAN_WRITE)) { + retval = -EACCES; goto close_fail; - if (do_truncate(idmap, cprm.file->f_path.dentry, - 0, 0, cprm.file)) + } + retval = do_truncate(idmap, cprm.file->f_path.dentry, + 0, 0, cprm.file); + if (retval) goto close_fail; } @@ -748,10 +785,15 @@ void do_coredump(const kernel_siginfo_t *siginfo) */ if (!cprm.file) { coredump_report_failure("Core dump to |%s disabled", cn.corename); + retval = -EPERM; goto close_fail; } - if (!dump_vma_snapshot(&cprm)) + if (!dump_vma_snapshot(&cprm)) { + coredump_report_failure("Can't get VMA snapshot for core dump |%s", + cn.corename); + retval = -EACCES; goto close_fail; + } file_start_write(cprm.file); core_dumped = binfmt->core_dump(&cprm); @@ -767,9 +809,21 @@ void do_coredump(const kernel_siginfo_t *siginfo) } file_end_write(cprm.file); free_vma_snapshot(&cprm); + } else { + coredump_report_failure("Core dump to %s%s has been interrupted", + ispipe ? "|" : "", cn.corename); + retval = -EAGAIN; + goto fail; } + coredump_report( + "written to %s%s: VMAs: %d, size %zu; core: %lld bytes, pos %lld", + ispipe ? "|" : "", cn.corename, + cprm.vma_count, cprm.vma_data_size, cprm.written, cprm.pos); if (ispipe && core_pipe_limit) wait_for_dump_helpers(cprm.file); + + retval = 0; + close_fail: if (cprm.file) filp_close(cprm.file, NULL); @@ -784,7 +838,7 @@ fail_unlock: fail_creds: put_cred(cred); fail: - return; + return retval; } /* @@ -804,8 +858,16 @@ static int __dump_emit(struct coredump_params *cprm, const void *addr, int nr) if (dump_interrupted()) return 0; n = __kernel_write(file, addr, nr, &pos); - if (n != nr) + if (n != nr) { + if (n < 0) + coredump_report_failure("failed when writing out, error %zd", n); + else + coredump_report_failure( + "partially written out, only %zd(of %d) bytes written", + n, nr); + return 0; + } file->f_pos = pos; cprm->written += n; cprm->pos += n; @@ -818,9 +880,16 @@ static int __dump_skip(struct coredump_params *cprm, size_t nr) static char zeroes[PAGE_SIZE]; struct file *file = cprm->file; if (file->f_mode & FMODE_LSEEK) { - if (dump_interrupted() || - vfs_llseek(file, nr, SEEK_CUR) < 0) + int ret; + + if (dump_interrupted()) return 0; + + ret = vfs_llseek(file, nr, SEEK_CUR); + if (ret < 0) { + coredump_report_failure("failed when seeking, error %d", ret); + return 0; + } cprm->pos += nr; return 1; } else { diff --git a/include/linux/coredump.h b/include/linux/coredump.h index 45e598fe3476..edeb8532ce0f 100644 --- a/include/linux/coredump.h +++ b/include/linux/coredump.h @@ -42,7 +42,7 @@ extern int dump_emit(struct coredump_params *cprm, const void *addr, int nr); extern int dump_align(struct coredump_params *cprm, int align); int dump_user_range(struct coredump_params *cprm, unsigned long start, unsigned long len); -extern void do_coredump(const kernel_siginfo_t *siginfo); +extern int do_coredump(const kernel_siginfo_t *siginfo); /* * Logging for the coredump code, ratelimited. @@ -62,7 +62,11 @@ extern void do_coredump(const kernel_siginfo_t *siginfo); #define coredump_report_failure(fmt, ...) __COREDUMP_PRINTK(KERN_WARNING, fmt, ##__VA_ARGS__) #else -static inline void do_coredump(const kernel_siginfo_t *siginfo) {} +static inline int do_coredump(const kernel_siginfo_t *siginfo) +{ + /* Coredump support is not available, can't fail. */ + return 0; +} #define coredump_report(...) #define coredump_report_failure(...) diff --git a/kernel/signal.c b/kernel/signal.c index 60c737e423a1..8c3417550d71 100644 --- a/kernel/signal.c +++ b/kernel/signal.c @@ -2888,6 +2888,8 @@ relock: current->flags |= PF_SIGNALED; if (sig_kernel_coredump(signr)) { + int ret; + if (print_fatal_signals) print_fatal_signal(signr); proc_coredump_connector(current); @@ -2899,7 +2901,24 @@ relock: * first and our do_group_exit call below will use * that value and ignore the one we pass it. */ - do_coredump(&ksig->info); + ret = do_coredump(&ksig->info); + if (ret) + coredump_report_failure("coredump has not been created, error %d", + ret); + else if (!IS_ENABLED(CONFIG_COREDUMP)) { + /* + * Coredumps are not available, can't fail collecting + * the coredump. + * + * Leave a note though that the coredump is going to be + * not created. This is not an error or a warning as disabling + * support in the kernel for coredumps isn't commonplace, and + * the user must've built the kernel with the custom config so + * let them know all works as desired. + */ + coredump_report("no coredump collected as " + "that is disabled in the kernel configuration"); + } } /* From 7d442a33bfe817ab2a735f3d2e430e36305354ea Mon Sep 17 00:00:00 2001 From: Brian Mak Date: Tue, 6 Aug 2024 18:16:02 +0000 Subject: [PATCH 3/4] binfmt_elf: Dump smaller VMAs first in ELF cores Large cores may be truncated in some scenarios, such as with daemons with stop timeouts that are not large enough or lack of disk space. This impacts debuggability with large core dumps since critical information necessary to form a usable backtrace, such as stacks and shared library information, are omitted. We attempted to figure out which VMAs are needed to create a useful backtrace, and it turned out to be a non-trivial problem. Instead, we try simply sorting the VMAs by size, which has the intended effect. By sorting VMAs by dump size and dumping in that order, we have a simple, yet effective heuristic. Signed-off-by: Brian Mak Link: https://lore.kernel.org/r/036CD6AE-C560-4FC7-9B02-ADD08E380DC9@juniper.net Acked-by: "Eric W. Biederman" Signed-off-by: Kees Cook --- fs/coredump.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/fs/coredump.c b/fs/coredump.c index 5814a6d781ce..53a78b6bbb5b 100644 --- a/fs/coredump.c +++ b/fs/coredump.c @@ -18,6 +18,7 @@ #include #include #include +#include #include #include #include @@ -1249,6 +1250,18 @@ static void free_vma_snapshot(struct coredump_params *cprm) } } +static int cmp_vma_size(const void *vma_meta_lhs_ptr, const void *vma_meta_rhs_ptr) +{ + const struct core_vma_metadata *vma_meta_lhs = vma_meta_lhs_ptr; + const struct core_vma_metadata *vma_meta_rhs = vma_meta_rhs_ptr; + + if (vma_meta_lhs->dump_size < vma_meta_rhs->dump_size) + return -1; + if (vma_meta_lhs->dump_size > vma_meta_rhs->dump_size) + return 1; + return 0; +} + /* * Under the mmap_lock, take a snapshot of relevant information about the task's * VMAs. @@ -1311,5 +1324,8 @@ static bool dump_vma_snapshot(struct coredump_params *cprm) cprm->vma_data_size += m->dump_size; } + sort(cprm->vma_meta, cprm->vma_count, sizeof(*cprm->vma_meta), + cmp_vma_size, NULL); + return true; } From 44f65d900698278a8451988abe0d5ca37fd46882 Mon Sep 17 00:00:00 2001 From: Jeff Xu Date: Tue, 6 Aug 2024 21:49:27 +0000 Subject: [PATCH 4/4] binfmt_elf: mseal address zero MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In load_elf_binary as part of the execve(), when the current task’s personality has MMAP_PAGE_ZERO set, the kernel allocates one page at address 0. According to the comment: /* Why this, you ask??? Well SVr4 maps page 0 as read-only, and some applications "depend" upon this behavior. Since we do not have the power to recompile these, we emulate the SVr4 behavior. Sigh. */ At one point, Linus suggested removing this [1]. Code search in debian didn't see much use of MMAP_PAGE_ZERO [2], it exists in util and test (rr). Sealing this is probably safe, the comment doesn't say the app ever wanting to change the mapping to rwx. Sealing also ensures that never happens. If there is a complaint, we can make this configurable. Link: https://lore.kernel.org/lkml/CAHk-=whVa=nm_GW=NVfPHqcxDbWt4JjjK1YWb0cLjO4ZSGyiDA@mail.gmail.com/ [1] Link: https://codesearch.debian.net/search?q=MMAP_PAGE_ZERO&literal=1&perpkg=1&page=1 [2] Signed-off-by: Jeff Xu Link: https://lore.kernel.org/r/20240806214931.2198172-2-jeffxu@google.com Signed-off-by: Kees Cook --- fs/binfmt_elf.c | 5 +++++ include/linux/mm.h | 10 ++++++++++ mm/mseal.c | 2 +- 3 files changed, 16 insertions(+), 1 deletion(-) diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c index bf9bfd1a0007..c3aa700aeb3c 100644 --- a/fs/binfmt_elf.c +++ b/fs/binfmt_elf.c @@ -1314,6 +1314,11 @@ out_free_interp: emulate the SVr4 behavior. Sigh. */ error = vm_mmap(NULL, 0, PAGE_SIZE, PROT_READ | PROT_EXEC, MAP_FIXED | MAP_PRIVATE, 0); + + retval = do_mseal(0, PAGE_SIZE, 0); + if (retval) + pr_warn_ratelimited("pid=%d, couldn't seal address 0, ret=%d.\n", + task_pid_nr(current), retval); } regs = current_pt_regs(); diff --git a/include/linux/mm.h b/include/linux/mm.h index c4b238a20b76..a178c15812eb 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -4201,4 +4201,14 @@ void vma_pgtable_walk_end(struct vm_area_struct *vma); int reserve_mem_find_by_name(const char *name, phys_addr_t *start, phys_addr_t *size); +#ifdef CONFIG_64BIT +int do_mseal(unsigned long start, size_t len_in, unsigned long flags); +#else +static inline int do_mseal(unsigned long start, size_t len_in, unsigned long flags) +{ + /* noop on 32 bit */ + return 0; +} +#endif + #endif /* _LINUX_MM_H */ diff --git a/mm/mseal.c b/mm/mseal.c index bf783bba8ed0..7a40a84569c8 100644 --- a/mm/mseal.c +++ b/mm/mseal.c @@ -248,7 +248,7 @@ static int apply_mm_seal(unsigned long start, unsigned long end) * * unseal() is not supported. */ -static int do_mseal(unsigned long start, size_t len_in, unsigned long flags) +int do_mseal(unsigned long start, size_t len_in, unsigned long flags) { size_t len; int ret = 0;