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 82DC7E7F151 for ; Wed, 27 Sep 2023 02:34:57 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id D60478D001C; Tue, 26 Sep 2023 22:34:56 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id D10DF8D0002; Tue, 26 Sep 2023 22:34:56 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id BB2018D001C; Tue, 26 Sep 2023 22:34:56 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id A5B978D0002 for ; Tue, 26 Sep 2023 22:34:56 -0400 (EDT) Received: from smtpin11.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id 74612140DE6 for ; Wed, 27 Sep 2023 02:34:56 +0000 (UTC) X-FDA: 81280809792.11.36987F2 Received: from mail-pf1-f173.google.com (mail-pf1-f173.google.com [209.85.210.173]) by imf27.hostedemail.com (Postfix) with ESMTP id 93D5240008 for ; Wed, 27 Sep 2023 02:34:53 +0000 (UTC) Authentication-Results: imf27.hostedemail.com; dkim=pass header.d=chromium.org header.s=google header.b=HoEcC1hw; spf=pass (imf27.hostedemail.com: domain of keescook@chromium.org designates 209.85.210.173 as permitted sender) smtp.mailfrom=keescook@chromium.org; dmarc=pass (policy=none) header.from=chromium.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1695782093; 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=MEoyNhEirsDID+iSINOkm/zSxakfDUSa2NzmIBoMrtU=; b=8sAFnnt7NGiHFVIlDGIuithSf7DeBM585hPZFUhHrq+/pJJK1bc2r2TIVq0OimkC+b3PEm 5Y3X0OcRcvDDYzbrhCSEcuLp0kvsCn3lwm1lorH6+R0OAIsUClLEsn/M286oAf2YhwfuVf iljM1FRPKJckGSCY5jyxAw56vCw6ghs= ARC-Authentication-Results: i=1; imf27.hostedemail.com; dkim=pass header.d=chromium.org header.s=google header.b=HoEcC1hw; spf=pass (imf27.hostedemail.com: domain of keescook@chromium.org designates 209.85.210.173 as permitted sender) smtp.mailfrom=keescook@chromium.org; dmarc=pass (policy=none) header.from=chromium.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1695782093; a=rsa-sha256; cv=none; b=2Bt/c9TSf0ezohwzmFFhWbwqvCDx505JeVHFFVvrW0JzXzE+TodB9v/UKkF0IB8bFnBe3/ nZpgopCr0fRVFvYwNOCN5zTYz/Y8h2B+EtWtuL7kdSu45ZDGA1yQdjt7nuH/YDQ7w2xvqj Icci1RbpdFFkc1LM57aPUZf7XwVAP3E= Received: by mail-pf1-f173.google.com with SMTP id d2e1a72fcca58-6910ea9cca1so7840630b3a.1 for ; Tue, 26 Sep 2023 19:34:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; t=1695782092; x=1696386892; darn=kvack.org; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:from:to :cc:subject:date:message-id:reply-to; bh=MEoyNhEirsDID+iSINOkm/zSxakfDUSa2NzmIBoMrtU=; b=HoEcC1hwuiyNmV9ZDNuvDvstl0bdTj1nxctMf0VAWc4WobaoeUXACmlRE7qnfuszYp PJaHnZumlPVMsnGNS2uxEfAFEHpqQDlv+MVtJi7hNB39Dspdz2tn9RCpRatfuBBAyIB2 oSnGSkzEs7viaY1a5ElgC1UVS2ZGrZ1CzNnao= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1695782092; x=1696386892; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=MEoyNhEirsDID+iSINOkm/zSxakfDUSa2NzmIBoMrtU=; b=rpQl7jPgLVfOErTy5TVPjMUoEek8sED8adB03/h5YlVZTi0ZqLxrwm7YiRsN8MCKpP fmBIHmxFcZVrwd4a7Dgpbl6Ubbhl/cTNR1rI3DMRxB9JZvX+Fwn7cGVYWTvCNSp5mIm2 ib4Yd4zfFukSSevK+1KrBLaUi3hjB5Bg0Cru30nybpMLVVpSR00B8/fWxdz+1Gd+sjx1 +nsPII/yPG3dQFxzVXxtXCxi142B4l7ebZYxdm7EwCVd99lDVTCCMPVPqPgTb7JRgF21 tttOoDSsKwNeUjMyJYFAkj82CIlxxFFGE4broxIm2fK3jOh4f/ytrMA2k6qLnhP/G4ja 0eFQ== X-Gm-Message-State: AOJu0YyJl8jS0G3Q4JPjfhcw57eaCQWwhTJD3Zj+2RooO6R5jXzhcrpN JtOYNxIz/p/mvif3rG7Qy47R/w== X-Google-Smtp-Source: AGHT+IH3mtSkhCwY4l5NpnakMGAz2fqDSfIVRDAs1r22pVm2l9sPYt/EIE/sGjn2HyWVPFLAH2dqvA== X-Received: by 2002:a05:6a00:139d:b0:692:780a:de89 with SMTP id t29-20020a056a00139d00b00692780ade89mr882056pfg.33.1695782092187; Tue, 26 Sep 2023 19:34:52 -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 z26-20020aa785da000000b006883561b421sm10697260pfn.162.2023.09.26.19.34.51 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 26 Sep 2023 19:34:51 -0700 (PDT) Date: Tue, 26 Sep 2023 19:34:50 -0700 From: Kees Cook To: "Eric W. Biederman" Cc: Sebastian Ott , Pedro Falcato , Thomas =?iso-8859-1?Q?Wei=DFschuh?= , Alexander Viro , Christian Brauner , Mark Brown , Willy Tarreau , sam@gentoo.org, Rich Felker , linux-fsdevel@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, stable@vger.kernel.org Subject: Re: [PATCH] binfmt_elf: Support segments with 0 filesz and misaligned starts Message-ID: <202309261929.BE87B8B7@keescook> References: <20230914-bss-alloc-v1-1-78de67d2c6dd@weissschuh.net> <36e93c8e-4384-b269-be78-479ccc7817b1@redhat.com> <87zg1bm5xo.fsf@email.froward.int.ebiederm.org> <37d3392c-cf33-20a6-b5c9-8b3fb8142658@redhat.com> <87jzsemmsd.fsf_-_@email.froward.int.ebiederm.org> <84e974d3-ae0d-9eb5-49b2-3348b7dcd336@redhat.com> <202309251001.C050864@keescook> <87v8bxiph5.fsf@email.froward.int.ebiederm.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <87v8bxiph5.fsf@email.froward.int.ebiederm.org> X-Rspamd-Queue-Id: 93D5240008 X-Rspam-User: X-Stat-Signature: 5xg8tcbtnhhe4y8qxd1k6eykjqgg1utb X-Rspamd-Server: rspam01 X-HE-Tag: 1695782093-119619 X-HE-Meta: U2FsdGVkX1+57U4zwM+REoq930iOCJx8wsqROzNdKID0LQPKL8dpZeh9GnRvNK7catvO0QFbMOeDdGBz4mJrU6EYDhoOAtG7lnjbq/k6YQ/R4Kt2U2P+Hirrx3UU3e+4Bx67s0G31SYNPX0j6L9Usj2ocpgt9LajhCjzQTYVAVDvyRm3orU9VStvoUI28uWhOQSrSFjGS3ObVi6ccj/prb1ArSvjxjMiFsvdGyc1za/vD+pfm/tc5SJOAELefRHwxOl+uTD1KbS7KzG2geWfzILgAAUALvBoA8nq2m2pLbSAH8Wz6f0K0reA+IplV12uN6dFLbu2xRuk8UVdmhDWdNPUn7e67spBJ0ZB+TnEmyTLvXbdo0dtxe8RHaQUvF+5kODEz1F2jzCqrNgY5gYd2dggu/1ApI108Jj20Bmn2dFA16dZALDRwEXWlhHtB8kOZah64HeK4/BKj3Mcm5tuP4hLq82ipAuOEvN/5Ixaz5aG/faEWaUh8S4xMf3v7yZmJH+GzpBuWo6mf0ridxSTSSJ1UtNlZS/4uClxmnn62BiAWzm/QffmiZ0S4jQIpYxcIB+bbPleD+eonuaLznsmmlZGCLYUp3xnusjmv99+lvb7qnqIzRj69+Varqnnnc4UsrtbT2jzFQSHFDWOe59qw6REZHhynOhJK+oAxTnClsCc/3PM0Gv1PTOa+5cyqO/Q+Ne9NZYfDyga3jKohaDF94ffO7SBW5SjuJZRRmIYX4GdChs5qm3BvksdrGQ5ow0c3D4KWfLQLdQSB3wcYxgyhJRuP+CsYpAPDpAmJYOfNMBo7d9Z8s/+6bg3I68DXANuxGoymF9a8TUg+q0qyphyUtlpi1/4U17wuvxPCoCu8NWdia+MmF/3Evo5g/HputLmnXwn4GpQZq4ZYr2WZBBp9oK90lOSuGoASCMjYUEVPzJ2cMpvCsUJ1yvBP1RKpIEAykDYWSrmgMipIa2WH7Z 6DA9hYpW JdadCY+RmaSzw3Ed4BgP9Q2/jsM0FhBtxzz/GH9CQcKKF6CCca0DQWizWMz0wH4bpMPvULMH+W8eH0Sv4pX/xh8a2G+r8eU46/Tahh3Ge777EToj4M3Aqd82gGUI2/wdWFMtV6mxHmU9MIAiAr2aCeV1leDfcseOk+7JwZe/b3IHsV3ps5D1w8kqQDVpQxDCOvZaiNWb2LfT6+Nr1NUehIwuslHQcB/s5qSiqtIiyW/48Vv6ghErRq+euY4iIZbIJW6+P2Q7hkHC9uNy8ZbglUrSpSjogDN4A6LotWDsvDDX5SDE4k24LEYWMTjMMWOhd5/uLv/UDG+nftyhAMV7+Bf86GKBI+1X3PzD0XFwtwJaNSG0mkgPEu0NktdEtD5QIITr+2tmzxq+n4dbBeSNRub+aFkXE3PAYs4GVW+LXx1Gqq5yUycCQnD2et82whcpqywVuKR1yi1vQx7UmKdEGdQijW1NrORcQd/+AN4wfFVaHIBqqdweBgcNFoPjSTEJJhW2WNPW8qlBZDTIjWLnojLjatD04mQas61ZRxpZ4zLZu4t9DHqWhnF9fLHTfkAmlcKtGgtLNP9JjjuJnKs9xCcxFvaaeBPIhZYnRvr09MrLdSy8jbUOJ5bspbbXtnKczxRSdlRRnA3oxwoX0zNJaZPIMfauiego8GSqOTohduE86g8HCHYlnuPT16QxYa3MqxMgyrciPtdEAwssw7MVH1zR/1xTIYMxF9UafMMmuVJad+ZImKFsb5o/lgjy2D+omchjn0hyuKZ5nLndr0JXqjsf4kiMl/CKZj1rIpaeZjEGo3GRgMvLqi+sRRw== 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: On Mon, Sep 25, 2023 at 10:27:02PM -0500, Eric W. Biederman wrote: > Kees Cook writes: > > > On Mon, Sep 25, 2023 at 05:27:12PM +0200, Sebastian Ott wrote: > >> On Mon, 25 Sep 2023, Eric W. Biederman wrote: > >> > > >> > Implement a helper elf_load that wraps elf_map and performs all > >> > of the necessary work to ensure that when "memsz > filesz" > >> > the bytes described by "memsz > filesz" are zeroed. > >> > > >> > Link: https://lkml.kernel.org/r/20230914-bss-alloc-v1-1-78de67d2c6dd@weissschuh.net > >> > Reported-by: Sebastian Ott > >> > Reported-by: Thomas Weißschuh > >> > Signed-off-by: "Eric W. Biederman" > >> > --- > >> > fs/binfmt_elf.c | 111 +++++++++++++++++++++--------------------------- > >> > 1 file changed, 48 insertions(+), 63 deletions(-) > >> > > >> > Can you please test this one? > > > > Eric thanks for doing this refactoring! This does look similar to the > > earlier attempt: > > https://lore.kernel.org/lkml/20221106021657.1145519-1-pedro.falcato@gmail.com/ > > and it's a bit easier to review. > > I need to context switch away for a while so Kees if you will > I will let you handle the rest of this one. > > > A couple of thoughts running through my head for anyone whose ambitious > might include cleaning up binfmt_elf.c > > The elf_bss variable in load_elf_binary can be removed. > > Work for a follow on patch is using my new elf_load in load_elf_interp > and possibly in load_elf_library. (More code size reduction). > > An outstanding issue is if the first segment has filesz 0, and has a > randomized locations. But that is the same as today. > > There is a whole question does it make sense for the elf loader > to have it's own helper vm_brk_flags in mm/mmap.c or would it > make more sense for binfmt_elf to do what binfmt_elf_fdpic does and > have everything to go through vm_mmap. > > I think replacing vm_brk_flags with vm_mmap would allow fixing the > theoretical issue of filesz 0 and randomizing locations. > > > > In this change I replaced an open coded padzero that did not clear > all of the way to the end of the page, with padzero that does. Yeah, the resulting code is way more readable now. > I also stopped checking the return of padzero as there is at least > one known case where testing for failure is the wrong thing to do. > It looks like binfmt_elf_fdpic may have the proper set of tests > for when error handling can be safely completed. > > I found a couple of commits in the old history > https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git, > that look very interesting in understanding this code. > > commit 39b56d902bf3 ("[PATCH] binfmt_elf: clearing bss may fail") > commit c6e2227e4a3e ("[SPARC64]: Missing user access return value checks in fs/binfmt_elf.c and fs/compat.c") > commit 5bf3be033f50 ("v2.4.10.1 -> v2.4.10.2") > > Looking at commit 39b56d902bf3 ("[PATCH] binfmt_elf: clearing bss may fail"): > > commit 39b56d902bf35241e7cba6cc30b828ed937175ad > > Author: Pavel Machek > > Date: Wed Feb 9 22:40:30 2005 -0800 > > > > [PATCH] binfmt_elf: clearing bss may fail > > > > So we discover that Borland's Kylix application builder emits weird elf > > files which describe a non-writeable bss segment. > > > > So remove the clear_user() check at the place where we zero out the bss. I > > don't _think_ there are any security implications here (plus we've never > > checked that clear_user() return value, so whoops if it is a problem). > > > > Signed-off-by: Pavel Machek > > Signed-off-by: Andrew Morton > > Signed-off-by: Linus Torvalds > > It seems pretty clear that binfmt_elf_fdpic with skipping clear_user > for non-writable segments and otherwise calling clear_user (aka padzero) > and checking it's return code is the right thing to do. > > I just skipped the error checking as that avoids breaking things. > > It looks like Borland's Kylix died in 2005 so it might be safe to > just consider read-only segments with memsz > filesz an error. I really feel like having a read-only BSS is a pathological state that should be detected early? > Looking at commit 5bf3be033f50 ("v2.4.10.1 -> v2.4.10.2") the > binfmt_elf.c bits confirm my guess that the weird structure is because > before that point binfmt_elf.c assumed there would be only a single > segment with memsz > filesz. Which is why the code was structured so > weirdly. Agreed. > Looking a little farther it looks like the binfmt_elf.c was introduced > in Linux v1.0, with essentially the same structure in load_elf_binary as > it has now. Prior to that Linux hard coded support for a.out binaries > in execve. So if someone wants to add a Fixes tag it should be > "Fixes: v1.0" > > Which finally explains to me why the code is so odd. For the most part > the code has only received maintenance for the last 30 years or so. > Strictly 29 years, but 30 has a better ring to it. > > Anyway those are my rambling thoughts that might help someone. > For now I will be happy if we can get my elf_load helper tested > to everyone's satisfaction and merged. I'm probably going to pull most of this email into the commit log for the v2 patch -- there's good history here worth capturing. -- Kees Cook