Message ID | 20230929031716.it.155-kees@kernel.org |
---|---|
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:cae8:0:b0:403:3b70:6f57 with SMTP id r8csp3892696vqu; Fri, 29 Sep 2023 02:30:43 -0700 (PDT) X-Google-Smtp-Source: AGHT+IF6dMKlB4p8oy/Gh3iYnTolbRJoZLezrnOxK730jUDjA00O8wszHNLbLSRh7XYefJy4pXqg X-Received: by 2002:a05:6a21:3d8b:b0:163:4288:1c3d with SMTP id bj11-20020a056a213d8b00b0016342881c3dmr433141pzc.43.1695979841971; Fri, 29 Sep 2023 02:30:41 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1695979841; cv=none; d=google.com; s=arc-20160816; b=cP/9m1DgooY/7GHg2OV4QbazjjqgZCIXrgWCNFD2gG3NyykzUp9T19jUD2f6z1Xs81 uDXO1w+qbeWrgE3N3IwOMLsFNw1QbsCHiSWy3FOzFo6BZiVG1rbNSMWEzcJpshAJoLuJ etxC5xTTQlmWRKLMQQMghEhuCkuhygP8g3DV0hJyN5EHsnBbinvk7zDOUuW8hwt1mAPo khWpF5hAfLWuG8aXLc+YiRBz1BRPBQxpudTZ9iQpeSA3TbG798TzMmwqmwM8Sm/Wm+Ex wR0GCbMz50TIXS+ksDxSmRMtsCVymNp2Fu+2JcPkxer3Crc/Rbwmhe9WuFbS7rLhKB4y Wr0g== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=DZpy/gLfnuMsQ3zYwfMZFr7l/UudGqfvJfNYCKkC6BA=; fh=XX5IXYH8cr6m1JvqVhV2T0rYnPj2s5ee3zd7sxhMDe4=; b=r2SJOGEZ3sSae09JaL74rgS2yTgfHd6+qXQiadlJE4VXT29eOK+yZFGHolKAxaeCIT MwfFvkv/nw5Orrp1zUNnG87nc/J+LhCNcv49tQMVS15mVcfCvXJUY/F8Hzch3CVN0WZD 6iO3Ghu0BY7bKEvjGvfvpaDODtZtsliZaQUilbQfkXX3qnRB5uwgQraGkK+3hnidK7/Q t2xVAjLakg4P0+MvtusgGIzwI4JONThpuQeqPebKdRnky25FHwXtjo4+J4qzk/s68bFs x1IdFgMEIMFPDLpgfi3mnL8Qyg2zEyiTogs/A9TVvIlUEy4CNKYAZwEsJ1Hd71SJU4Oc SwHw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@chromium.org header.s=google header.b=OjoTLLKT; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=chromium.org Received: from snail.vger.email (snail.vger.email. [2620:137:e000::3:7]) by mx.google.com with ESMTPS id y6-20020a63e246000000b00570505c5267si21541438pgj.262.2023.09.29.02.30.41 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 29 Sep 2023 02:30:41 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7; Authentication-Results: mx.google.com; dkim=pass header.i=@chromium.org header.s=google header.b=OjoTLLKT; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=chromium.org Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by snail.vger.email (Postfix) with ESMTP id 8195B82FABD1; Thu, 28 Sep 2023 20:24:50 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at snail.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229912AbjI2DYl (ORCPT <rfc822;pwkd43@gmail.com> + 20 others); Thu, 28 Sep 2023 23:24:41 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:50996 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229541AbjI2DYk (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 28 Sep 2023 23:24:40 -0400 Received: from mail-pl1-x62d.google.com (mail-pl1-x62d.google.com [IPv6:2607:f8b0:4864:20::62d]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 99872199 for <linux-kernel@vger.kernel.org>; Thu, 28 Sep 2023 20:24:37 -0700 (PDT) Received: by mail-pl1-x62d.google.com with SMTP id d9443c01a7336-1c453379020so102778515ad.1 for <linux-kernel@vger.kernel.org>; Thu, 28 Sep 2023 20:24:37 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; t=1695957877; x=1696562677; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=DZpy/gLfnuMsQ3zYwfMZFr7l/UudGqfvJfNYCKkC6BA=; b=OjoTLLKT2BLmAAwOCFmylxZWmrd8SbraEqzMVMu3orKXWJYB8mjAuosRLOmD+TC7Bz gTfm+1xA0YRedGX5bo4NUJdIq4ndJ82qIhixDAUh/u86sYdd057zDna4w4uR8QNZAQy7 G4xgnn+D3W9igApDT5au+08XUNb1n1kMzAES0= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1695957877; x=1696562677; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=DZpy/gLfnuMsQ3zYwfMZFr7l/UudGqfvJfNYCKkC6BA=; b=aXpff0M4Rpphv9IIo6UiLs4z4h5X++5kAa17nL9WjfspSkFGVs5Xzh0xdHe7LsrC3T LN5TxmdbWYGetKMsoTdymOrxeGIO4FKU3kb+IA9GWGKhtoUAD1pzawd1q2vs9wEFmee0 WW2dvAzkv2uV7+vv7Oo8Dsp5AunIdSKkSZC5IKeAWIAVOJBXFih172yLt9LqFuG+SHTY wsWhHj0K9Cr+W+1Z8WqnDqp9rXq21+0zNawzAXmZwhqFGbDWoTOVJNdEVCnGyA/EjdzA B/4Q3+wDscGzgsrwLh6OxyMfxuxLJygyg8EpA34h67eyMwI0kvBJlGaC9+cFQ36xz0q8 TztA== X-Gm-Message-State: AOJu0YwQ2k0gf7zE1Xu88vNmDzyGcp+TIdrUZJbv/NCOpjTC1Mc7PeVF FxxdjtifsPN1wqaWKuBpRyVx0g== X-Received: by 2002:a17:903:1cf:b0:1b8:94e9:e7b0 with SMTP id e15-20020a17090301cf00b001b894e9e7b0mr3606862plh.9.1695957877031; Thu, 28 Sep 2023 20:24:37 -0700 (PDT) Received: from www.outflux.net (198-0-35-241-static.hfc.comcastbusiness.net. [198.0.35.241]) by smtp.gmail.com with ESMTPSA id iw12-20020a170903044c00b001bf574dd1fesm6250537plb.141.2023.09.28.20.24.36 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 28 Sep 2023 20:24:36 -0700 (PDT) From: Kees Cook <keescook@chromium.org> To: Eric Biederman <ebiederm@xmission.com> Cc: Kees Cook <keescook@chromium.org>, Sebastian Ott <sebott@redhat.com>, =?utf-8?q?Thomas_Wei=C3=9Fschuh?= <linux@weissschuh.net>, Pedro Falcato <pedro.falcato@gmail.com>, Al Viro <viro@zeniv.linux.org.uk>, Christian Brauner <brauner@kernel.org>, Andrew Morton <akpm@linux-foundation.org>, linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-mm@kvack.org, linux-hardening@vger.kernel.org Subject: [PATCH v4 0/6] binfmt_elf: Support segments with 0 filesz and misaligned starts Date: Thu, 28 Sep 2023 20:24:28 -0700 Message-Id: <20230929031716.it.155-kees@kernel.org> X-Mailer: git-send-email 2.34.1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 X-Developer-Signature: v=1; a=openpgp-sha256; l=1382; i=keescook@chromium.org; h=from:subject:message-id; bh=uV4AJsRv4n/gB4mSOC9KIOnGOAI8uNiU0zUGBP7t1x8=; b=owEBbQKS/ZANAwAKAYly9N/cbcAmAcsmYgBlFkNxLv+WtDGXS3EGA7QG2806nd9yzM662vbJz Z/QF9lP7jqJAjMEAAEKAB0WIQSlw/aPIp3WD3I+bhOJcvTf3G3AJgUCZRZDcQAKCRCJcvTf3G3A Jl1KD/4w77J7PmG0TdHjUB7lH4ZlBX43xi8xD755lb9vhHxLRTtI3hqQ+JvyAc97r4JGaph3Jk3 +kKf02DqkcBcEm0W7oeO0JKZtqIfM8XEHkHQ9/pE8mKfalwC5DCFnIJbNcsdxr9SSwhz0POUO3E QaUEJZjccKFkHTwDKLd3XRvNdDZ3jll5HxQfWEKXvyjNCHVTG5jgC+GQB8d+RSZPwp1CGvEWqN5 4GDF1tB2l1DHa4lX6yI8qPhawqRzZuJFTO3SSse+E2IPkgfkt6VfJ8cWyqDd6AMALfIOl3hIq7E u0hVjNyK8WcQSmSzMHbrAHi1hVxdRRZzfKJw49k9mbc3zUKx2iTEbJRitqjEQOXaE9xDtpENZ8z ACRF8mJ3ewqtPtv5N3XhiB9SiOUGIBaWR5qCcgL989zLDhLf7IdBgj6pCpz+35BH+8Jx3ioPlse tvWvLx1KjocOYMQWqde+FJQsJokrG/P2gt8jKDPp96b16uQlble+qU64f0zUbbTRQXjRaGP9JE6 lUxI28L4rZ/qPL2gFq6pMGTT2offlDs6i1y5OBptudXqWdphur4s1n/ctL4RlT89tTT81h/qPgm T2AB0n5jBX6DQm4Ji0DI78f0/MRN0aAbjeLYg7EiWO86oDL4JmXSqgzGwkWXgxhXTqngX8Gnzoc dw/DwpD vMJ3XKAQ== X-Developer-Key: i=keescook@chromium.org; a=openpgp; fpr=A5C3F68F229DD60F723E6E138972F4DFDC6DC026 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE, SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (snail.vger.email [0.0.0.0]); Thu, 28 Sep 2023 20:24:50 -0700 (PDT) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1778363758739368801 X-GMAIL-MSGID: 1778363758739368801 |
Series |
binfmt_elf: Support segments with 0 filesz and misaligned starts
|
|
Message
Kees Cook
Sept. 29, 2023, 3:24 a.m. UTC
Hi, This is the continuation of the work Eric started for handling "p_memsz > p_filesz" in arbitrary segments (rather than just the last, BSS, segment). I've added the suggested changes: - drop unused "elf_bss" variable - refactor load_elf_interp() to use elf_load() - refactor load_elf_library() to use elf_load() - report padzero() errors when PROT_WRITE is present - drop vm_brk() Thanks! -Kees v4: - refactor load_elf_library() too - don't refactor padzero(), just test in the only remaining caller - drop now-unused vm_brk() v3: https://lore.kernel.org/all/20230927033634.make.602-kees@kernel.org v2: https://lore.kernel.org/lkml/87sf71f123.fsf@email.froward.int.ebiederm.org v1: https://lore.kernel.org/lkml/87jzsemmsd.fsf_-_@email.froward.int.ebiederm.org Eric W. Biederman (1): binfmt_elf: Support segments with 0 filesz and misaligned starts Kees Cook (5): binfmt_elf: elf_bss no longer used by load_elf_binary() binfmt_elf: Use elf_load() for interpreter binfmt_elf: Use elf_load() for library binfmt_elf: Only report padzero() errors when PROT_WRITE mm: Remove unused vm_brk() fs/binfmt_elf.c | 214 ++++++++++++++++----------------------------- include/linux/mm.h | 3 +- mm/mmap.c | 6 -- mm/nommu.c | 5 -- 4 files changed, 76 insertions(+), 152 deletions(-)
Comments
Hello Kees, On Thu, 28 Sep 2023, Kees Cook wrote: > This is the continuation of the work Eric started for handling > "p_memsz > p_filesz" in arbitrary segments (rather than just the last, > BSS, segment). I've added the suggested changes: > > - drop unused "elf_bss" variable > - refactor load_elf_interp() to use elf_load() > - refactor load_elf_library() to use elf_load() > - report padzero() errors when PROT_WRITE is present > - drop vm_brk() While I was debugging the initial issue I stumbled over the following - care to take it as part of this series? ----->8 [PATCH] mm: vm_brk_flags don't bail out while holding lock Calling vm_brk_flags() with flags set other than VM_EXEC will exit the function without releasing the mmap_write_lock. Just do the sanity check before the lock is acquired. This doesn't fix an actual issue since no caller sets a flag other than VM_EXEC. Cc: Andrew Morton <akpm@linux-foundation.org> Cc: linux-mm@kvack.org Signed-off-by: Sebastian Ott <sebott@redhat.com> --- mm/mmap.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/mm/mmap.c b/mm/mmap.c index b56a7f0c9f85..7ed286662839 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -3143,13 +3143,13 @@ int vm_brk_flags(unsigned long addr, unsigned long request, unsigned long flags) if (!len) return 0; - if (mmap_write_lock_killable(mm)) - return -EINTR; - /* Until we need other flags, refuse anything except VM_EXEC. */ if ((flags & (~VM_EXEC)) != 0) return -EINVAL; + if (mmap_write_lock_killable(mm)) + return -EINTR; + ret = check_brk_limits(addr, len); if (ret) goto limits_failed;
On Fri, Sep 29, 2023 at 4:24 AM Kees Cook <keescook@chromium.org> wrote: > > Hi, > > This is the continuation of the work Eric started for handling > "p_memsz > p_filesz" in arbitrary segments (rather than just the last, > BSS, segment). I've added the suggested changes: > > - drop unused "elf_bss" variable > - refactor load_elf_interp() to use elf_load() > - refactor load_elf_library() to use elf_load() > - report padzero() errors when PROT_WRITE is present > - drop vm_brk() > > Thanks! > > -Kees > > v4: > - refactor load_elf_library() too > - don't refactor padzero(), just test in the only remaining caller > - drop now-unused vm_brk() > v3: https://lore.kernel.org/all/20230927033634.make.602-kees@kernel.org > v2: https://lore.kernel.org/lkml/87sf71f123.fsf@email.froward.int.ebiederm.org > v1: https://lore.kernel.org/lkml/87jzsemmsd.fsf_-_@email.froward.int.ebiederm.org > > Eric W. Biederman (1): > binfmt_elf: Support segments with 0 filesz and misaligned starts > > Kees Cook (5): > binfmt_elf: elf_bss no longer used by load_elf_binary() > binfmt_elf: Use elf_load() for interpreter > binfmt_elf: Use elf_load() for library > binfmt_elf: Only report padzero() errors when PROT_WRITE > mm: Remove unused vm_brk() > > fs/binfmt_elf.c | 214 ++++++++++++++++----------------------------- > include/linux/mm.h | 3 +- > mm/mmap.c | 6 -- > mm/nommu.c | 5 -- > 4 files changed, 76 insertions(+), 152 deletions(-) Sorry for taking so long to take a look at this. While I didn't test PPC64 (I don't own PPC64 hardware, and I wasn't the original reporter), I did manage to craft a reduced test case of: a.out: Program Headers: Type Offset VirtAddr PhysAddr FileSiz MemSiz Flags Align PHDR 0x0000000000000040 0x0000000000000040 0x0000000000000040 0x00000000000001f8 0x00000000000001f8 R 0x8 INTERP 0x0000000000000238 0x0000000000000238 0x0000000000000238 0x0000000000000020 0x0000000000000020 R 0x1 [Requesting program interpreter: /home/pfalcato/musl/lib/libc.so] LOAD 0x0000000000000000 0x0000000000000000 0x0000000000000000 0x0000000000000428 0x0000000000000428 R 0x1000 LOAD 0x0000000000001000 0x0000000000001000 0x0000000000001000 0x00000000000000cd 0x00000000000000cd R E 0x1000 LOAD 0x0000000000002000 0x0000000000002000 0x0000000000002000 0x0000000000000084 0x0000000000000084 R 0x1000 LOAD 0x0000000000002e50 0x0000000000003e50 0x0000000000003e50 0x00000000000001c8 0x00000000000001c8 RW 0x1000 DYNAMIC 0x0000000000002e50 0x0000000000003e50 0x0000000000003e50 0x0000000000000180 0x0000000000000180 RW 0x8 GNU_STACK 0x0000000000000000 0x0000000000000000 0x0000000000000000 0x0000000000000000 0x0000000000000000 RW 0x10 GNU_RELRO 0x0000000000002e50 0x0000000000003e50 0x0000000000003e50 0x00000000000001b0 0x00000000000001b0 R 0x1 /home/pfalcato/musl/lib/libc.so: Program Headers: Type Offset VirtAddr PhysAddr FileSiz MemSiz Flags Align PHDR 0x0000000000000040 0x0000000000000040 0x0000000000000040 0x0000000000000230 0x0000000000000230 R 0x8 LOAD 0x0000000000000000 0x0000000000000000 0x0000000000000000 0x0000000000049d9c 0x0000000000049d9c R 0x1000 LOAD 0x0000000000049da0 0x000000000004ada0 0x000000000004ada0 0x0000000000057d30 0x0000000000057d30 R E 0x1000 LOAD 0x00000000000a1ad0 0x00000000000a3ad0 0x00000000000a3ad0 0x00000000000005f0 0x00000000000015f0 RW 0x1000 LOAD 0x00000000000a20c0 0x00000000000a60c0 0x00000000000a60c0 0x0000000000000428 0x0000000000002f80 RW 0x1000 DYNAMIC 0x00000000000a1f38 0x00000000000a3f38 0x00000000000a3f38 0x0000000000000110 0x0000000000000110 RW 0x8 GNU_RELRO 0x00000000000a1ad0 0x00000000000a3ad0 0x00000000000a3ad0 0x00000000000005f0 0x0000000000002530 R 0x1 GNU_EH_FRAME 0x0000000000049d10 0x0000000000049d10 0x0000000000049d10 0x0000000000000024 0x0000000000000024 R 0x4 GNU_STACK 0x0000000000000000 0x0000000000000000 0x0000000000000000 0x0000000000000000 0x0000000000000000 RW 0x0 NOTE 0x0000000000000270 0x0000000000000270 0x0000000000000270 0x0000000000000018 0x0000000000000018 R 0x4 Section to Segment mapping: Segment Sections... 00 01 .note.gnu.build-id .dynsym .gnu.hash .hash .dynstr .rela.dyn .rela.plt .rodata .eh_frame_hdr .eh_frame 02 .text .plt 03 .data.rel.ro .dynamic .got .toc 04 .data .got.plt .bss 05 .dynamic 06 .data.rel.ro .dynamic .got .toc 07 .eh_frame_hdr 08 09 .note.gnu.build-id So on that end, you can take my Tested-by: Pedro Falcato <pedro.falcato@gmail.com> Although this still doesn't address the other bug I found (https://github.com/heatd/elf-bug-questionmark), where segments can accidentally overwrite cleared BSS if we end up in a situation where e.g we have the following segments: Program Headers: Type Offset VirtAddr PhysAddr FileSiz MemSiz Flags Align LOAD 0x0000000000001000 0x0000000000400000 0x0000000000400000 0x0000000000000045 0x0000000000000045 R E 0x1000 LOAD 0x0000000000002000 0x0000000000401000 0x0000000000401000 0x000000000000008c 0x000000000000008c R 0x1000 LOAD 0x0000000000000000 0x0000000000402000 0x0000000000402000 0x0000000000000000 0x0000000000000801 RW 0x1000 LOAD 0x0000000000002801 0x0000000000402801 0x0000000000402801 0x0000000000000007 0x0000000000000007 RW 0x1000 NOTE 0x0000000000002068 0x0000000000401068 0x0000000000401068 0x0000000000000024 0x0000000000000024 0x4 Section to Segment mapping: Segment Sections... 00 .text 01 .rodata .note.gnu.property .note.gnu.build-id 02 .bss 03 .data 04 .note.gnu.build-id since the mmap of .data will end up happening over .bss.
Pedro Falcato <pedro.falcato@gmail.com> writes: > On Fri, Sep 29, 2023 at 4:24 AM Kees Cook <keescook@chromium.org> wrote: >> >> While load_elf_library() is a libc5-ism, we can still replace most of >> its contents with elf_load() as well, further simplifying the code. > > While I understand you want to break as little as possible (as the ELF > loader maintainer), I'm wondering if we could axe CONFIG_USELIB > altogether? Since CONFIG_BINFMT_AOUT also got axed. Does this have > users anywhere? As I recall: - libc4 was a.out and used uselib. - libc5 was elf and used uselib. - libc6 is elf and has never used uselib. Anything using libc5 is extremely rare. It is an entire big process to see if there are any users in existence. In the meantime changing load_elf_library to use elf_load removes any maintenance burden. Eric
Pedro Falcato <pedro.falcato@gmail.com> writes: > On Fri, Sep 29, 2023 at 4:24 AM Kees Cook <keescook@chromium.org> wrote: >> >> Hi, >> >> This is the continuation of the work Eric started for handling >> "p_memsz > p_filesz" in arbitrary segments (rather than just the last, >> BSS, segment). I've added the suggested changes: >> >> - drop unused "elf_bss" variable >> - refactor load_elf_interp() to use elf_load() >> - refactor load_elf_library() to use elf_load() >> - report padzero() errors when PROT_WRITE is present >> - drop vm_brk() >> >> Thanks! >> >> -Kees >> >> v4: >> - refactor load_elf_library() too >> - don't refactor padzero(), just test in the only remaining caller >> - drop now-unused vm_brk() >> v3: https://lore.kernel.org/all/20230927033634.make.602-kees@kernel.org >> v2: https://lore.kernel.org/lkml/87sf71f123.fsf@email.froward.int.ebiederm.org >> v1: https://lore.kernel.org/lkml/87jzsemmsd.fsf_-_@email.froward.int.ebiederm.org >> >> Eric W. Biederman (1): >> binfmt_elf: Support segments with 0 filesz and misaligned starts >> >> Kees Cook (5): >> binfmt_elf: elf_bss no longer used by load_elf_binary() >> binfmt_elf: Use elf_load() for interpreter >> binfmt_elf: Use elf_load() for library >> binfmt_elf: Only report padzero() errors when PROT_WRITE >> mm: Remove unused vm_brk() >> >> fs/binfmt_elf.c | 214 ++++++++++++++++----------------------------- >> include/linux/mm.h | 3 +- >> mm/mmap.c | 6 -- >> mm/nommu.c | 5 -- >> 4 files changed, 76 insertions(+), 152 deletions(-) > > Sorry for taking so long to take a look at this. > While I didn't test PPC64 (I don't own PPC64 hardware, and I wasn't > the original reporter), I did manage to craft a reduced test case of: > > a.out: > > Program Headers: > Type Offset VirtAddr PhysAddr > FileSiz MemSiz Flags Align > PHDR 0x0000000000000040 0x0000000000000040 0x0000000000000040 > 0x00000000000001f8 0x00000000000001f8 R 0x8 > INTERP 0x0000000000000238 0x0000000000000238 0x0000000000000238 > 0x0000000000000020 0x0000000000000020 R 0x1 > [Requesting program interpreter: /home/pfalcato/musl/lib/libc.so] > LOAD 0x0000000000000000 0x0000000000000000 0x0000000000000000 > 0x0000000000000428 0x0000000000000428 R 0x1000 > LOAD 0x0000000000001000 0x0000000000001000 0x0000000000001000 > 0x00000000000000cd 0x00000000000000cd R E 0x1000 > LOAD 0x0000000000002000 0x0000000000002000 0x0000000000002000 > 0x0000000000000084 0x0000000000000084 R 0x1000 > LOAD 0x0000000000002e50 0x0000000000003e50 0x0000000000003e50 > 0x00000000000001c8 0x00000000000001c8 RW 0x1000 > DYNAMIC 0x0000000000002e50 0x0000000000003e50 0x0000000000003e50 > 0x0000000000000180 0x0000000000000180 RW 0x8 > GNU_STACK 0x0000000000000000 0x0000000000000000 0x0000000000000000 > 0x0000000000000000 0x0000000000000000 RW 0x10 > GNU_RELRO 0x0000000000002e50 0x0000000000003e50 0x0000000000003e50 > 0x00000000000001b0 0x00000000000001b0 R 0x1 > > /home/pfalcato/musl/lib/libc.so: > Program Headers: > Type Offset VirtAddr PhysAddr > FileSiz MemSiz Flags Align > PHDR 0x0000000000000040 0x0000000000000040 0x0000000000000040 > 0x0000000000000230 0x0000000000000230 R 0x8 > LOAD 0x0000000000000000 0x0000000000000000 0x0000000000000000 > 0x0000000000049d9c 0x0000000000049d9c R 0x1000 > LOAD 0x0000000000049da0 0x000000000004ada0 0x000000000004ada0 > 0x0000000000057d30 0x0000000000057d30 R E 0x1000 > LOAD 0x00000000000a1ad0 0x00000000000a3ad0 0x00000000000a3ad0 > 0x00000000000005f0 0x00000000000015f0 RW 0x1000 > LOAD 0x00000000000a20c0 0x00000000000a60c0 0x00000000000a60c0 > 0x0000000000000428 0x0000000000002f80 RW 0x1000 > DYNAMIC 0x00000000000a1f38 0x00000000000a3f38 0x00000000000a3f38 > 0x0000000000000110 0x0000000000000110 RW 0x8 > GNU_RELRO 0x00000000000a1ad0 0x00000000000a3ad0 0x00000000000a3ad0 > 0x00000000000005f0 0x0000000000002530 R 0x1 > GNU_EH_FRAME 0x0000000000049d10 0x0000000000049d10 0x0000000000049d10 > 0x0000000000000024 0x0000000000000024 R 0x4 > GNU_STACK 0x0000000000000000 0x0000000000000000 0x0000000000000000 > 0x0000000000000000 0x0000000000000000 RW 0x0 > NOTE 0x0000000000000270 0x0000000000000270 0x0000000000000270 > 0x0000000000000018 0x0000000000000018 R 0x4 > > Section to Segment mapping: > Segment Sections... > 00 > 01 .note.gnu.build-id .dynsym .gnu.hash .hash .dynstr .rela.dyn > .rela.plt .rodata .eh_frame_hdr .eh_frame > 02 .text .plt > 03 .data.rel.ro .dynamic .got .toc > 04 .data .got.plt .bss > 05 .dynamic > 06 .data.rel.ro .dynamic .got .toc > 07 .eh_frame_hdr > 08 > 09 .note.gnu.build-id > > > So on that end, you can take my > > Tested-by: Pedro Falcato <pedro.falcato@gmail.com> > > Although this still doesn't address the other bug I found > (https://github.com/heatd/elf-bug-questionmark), where segments can > accidentally overwrite cleared BSS if we end up in a situation where > e.g we have the following segments: > > Program Headers: > Type Offset VirtAddr PhysAddr > FileSiz MemSiz Flags Align > LOAD 0x0000000000001000 0x0000000000400000 0x0000000000400000 > 0x0000000000000045 0x0000000000000045 R E 0x1000 > LOAD 0x0000000000002000 0x0000000000401000 0x0000000000401000 > 0x000000000000008c 0x000000000000008c R 0x1000 > LOAD 0x0000000000000000 0x0000000000402000 0x0000000000402000 > 0x0000000000000000 0x0000000000000801 RW 0x1000 > LOAD 0x0000000000002801 0x0000000000402801 0x0000000000402801 > 0x0000000000000007 0x0000000000000007 RW 0x1000 > NOTE 0x0000000000002068 0x0000000000401068 0x0000000000401068 > 0x0000000000000024 0x0000000000000024 0x4 > > Section to Segment mapping: > Segment Sections... > 00 .text > 01 .rodata .note.gnu.property .note.gnu.build-id > 02 .bss > 03 .data > 04 .note.gnu.build-id > > since the mmap of .data will end up happening over .bss. This is simply invalid userspace, doubly so with the alignment set to 0x1000. The best the kernel can do is have a pre-pass through the elf program headers (before the point of no-return) and if they describe overlapping segments or out of order segments, cause execve syscall to fail. Eric
Sebastian Ott <sebott@redhat.com> writes: > Hello Kees, > > On Thu, 28 Sep 2023, Kees Cook wrote: >> This is the continuation of the work Eric started for handling >> "p_memsz > p_filesz" in arbitrary segments (rather than just the last, >> BSS, segment). I've added the suggested changes: >> >> - drop unused "elf_bss" variable >> - refactor load_elf_interp() to use elf_load() >> - refactor load_elf_library() to use elf_load() >> - report padzero() errors when PROT_WRITE is present >> - drop vm_brk() > > While I was debugging the initial issue I stumbled over the following > - care to take it as part of this series? > > ----->8 > [PATCH] mm: vm_brk_flags don't bail out while holding lock > > Calling vm_brk_flags() with flags set other than VM_EXEC > will exit the function without releasing the mmap_write_lock. > > Just do the sanity check before the lock is acquired. This > doesn't fix an actual issue since no caller sets a flag other > than VM_EXEC. That seems like a sensible patch. Have you by any chance read this code enough to understand what is gained by calling vm_brk_flags rather than vm_mmap without a file? Unless there is a real advantage it probably makes sense to replace the call of vm_brk_flags with vm_mmap(NULL, ...) as binfmt_elf_fdpic has already done. That would allow removing vm_brk_flags and sys_brk would be the last caller of do_brk_flags. Eric > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: linux-mm@kvack.org > Signed-off-by: Sebastian Ott <sebott@redhat.com> > --- > mm/mmap.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/mm/mmap.c b/mm/mmap.c > index b56a7f0c9f85..7ed286662839 100644 > --- a/mm/mmap.c > +++ b/mm/mmap.c > @@ -3143,13 +3143,13 @@ int vm_brk_flags(unsigned long addr, unsigned long request, unsigned long flags) > if (!len) > return 0; > > - if (mmap_write_lock_killable(mm)) > - return -EINTR; > - > /* Until we need other flags, refuse anything except VM_EXEC. */ > if ((flags & (~VM_EXEC)) != 0) > return -EINVAL; > > + if (mmap_write_lock_killable(mm)) > + return -EINTR; > + > ret = check_brk_limits(addr, len); > if (ret) > goto limits_failed;
On Fri, Sep 29, 2023 at 12:58:18PM +0100, Pedro Falcato wrote: > So on that end, you can take my > > Tested-by: Pedro Falcato <pedro.falcato@gmail.com> Thanks!
On Fri, Sep 29, 2023 at 01:33:50PM +0200, Sebastian Ott wrote: > Hello Kees, > > On Thu, 28 Sep 2023, Kees Cook wrote: > > This is the continuation of the work Eric started for handling > > "p_memsz > p_filesz" in arbitrary segments (rather than just the last, > > BSS, segment). I've added the suggested changes: > > > > - drop unused "elf_bss" variable > > - refactor load_elf_interp() to use elf_load() > > - refactor load_elf_library() to use elf_load() > > - report padzero() errors when PROT_WRITE is present > > - drop vm_brk() > > While I was debugging the initial issue I stumbled over the following > - care to take it as part of this series? > ----->8 > [PATCH] mm: vm_brk_flags don't bail out while holding lock > > Calling vm_brk_flags() with flags set other than VM_EXEC > will exit the function without releasing the mmap_write_lock. > > Just do the sanity check before the lock is acquired. This > doesn't fix an actual issue since no caller sets a flag other > than VM_EXEC. Oh, eek. Yeah, that seems like a good idea. :) Reviewed-by: Kees Cook <keescook@chromium.org> -Kees