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 40835E81816 for ; Tue, 26 Sep 2023 03:27:38 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id A5E926B0120; Mon, 25 Sep 2023 23:27:37 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id A0E426B0123; Mon, 25 Sep 2023 23:27:37 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 8D6126B013F; Mon, 25 Sep 2023 23:27:37 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id 7BD566B0120 for ; Mon, 25 Sep 2023 23:27:37 -0400 (EDT) Received: from smtpin29.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id 47CA6120E36 for ; Tue, 26 Sep 2023 03:27:37 +0000 (UTC) X-FDA: 81277313754.29.BCCD0F8 Received: from out01.mta.xmission.com (out01.mta.xmission.com [166.70.13.231]) by imf30.hostedemail.com (Postfix) with ESMTP id DEF7580006 for ; Tue, 26 Sep 2023 03:27:33 +0000 (UTC) Authentication-Results: imf30.hostedemail.com; dkim=none; spf=pass (imf30.hostedemail.com: domain of ebiederm@xmission.com designates 166.70.13.231 as permitted sender) smtp.mailfrom=ebiederm@xmission.com; dmarc=pass (policy=none) header.from=xmission.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1695698854; a=rsa-sha256; cv=none; b=Gb2Diu6ojfJLg/gZD5JJnDmszgKYKrbzos4i6GrDvmHmZqc0Au7kbWhYJ1zxydJs2G/puP zGGfh8ZkJZLpUnAZN8xF8FLY33uVT6ZxhIlZ1cVo21qxckPmmiaZ0mr7QwsQbWk/3E2nku CwaHqcsVjKfwqSL56Cjh+gMW7/r7Plg= ARC-Authentication-Results: i=1; imf30.hostedemail.com; dkim=none; spf=pass (imf30.hostedemail.com: domain of ebiederm@xmission.com designates 166.70.13.231 as permitted sender) smtp.mailfrom=ebiederm@xmission.com; dmarc=pass (policy=none) header.from=xmission.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1695698854; 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; bh=K0Ykck2iZR6DkMB4AESUgde1GT36hMBQIU7ks0PlBiw=; b=HbG9WDf07MwMhJTBRMR2bbOqvkgRy4RN6iOIDGcmyS6E5X1P2yE9C8PSEyGWc6ZHi2TU7Z nB7O6VXJqKJAr4tne4rIpKYjQp/fuj/rRQiKFufkvniy5CzIh07NJS7lbYjcRp8hRy/mvU QWNNz9wISx4VlJl6XgkBsXQMLAdn0Oc= Received: from in02.mta.xmission.com ([166.70.13.52]:60716) by out01.mta.xmission.com with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.93) (envelope-from ) id 1qkyjR-0025XF-KK; Mon, 25 Sep 2023 21:27:29 -0600 Received: from ip68-227-168-167.om.om.cox.net ([68.227.168.167]:52928 helo=email.froward.int.ebiederm.org.xmission.com) by in02.mta.xmission.com with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.93) (envelope-from ) id 1qkyjQ-00A2kG-Bx; Mon, 25 Sep 2023 21:27:29 -0600 From: "Eric W. Biederman" To: Kees Cook Cc: Sebastian Ott , Pedro Falcato , Thomas =?utf-8?Q?Wei=C3=9Fschuh?= , 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 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> Date: Mon, 25 Sep 2023 22:27:02 -0500 In-Reply-To: <202309251001.C050864@keescook> (Kees Cook's message of "Mon, 25 Sep 2023 10:06:01 -0700") Message-ID: <87v8bxiph5.fsf@email.froward.int.ebiederm.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-XM-SPF: eid=1qkyjQ-00A2kG-Bx;;;mid=<87v8bxiph5.fsf@email.froward.int.ebiederm.org>;;;hst=in02.mta.xmission.com;;;ip=68.227.168.167;;;frm=ebiederm@xmission.com;;;spf=pass X-XM-AID: U2FsdGVkX1/wOaQp+8aovEG4tuK61x9uVVX7vY5QjGM= X-SA-Exim-Connect-IP: 68.227.168.167 X-SA-Exim-Mail-From: ebiederm@xmission.com Subject: Re: [PATCH] binfmt_elf: Support segments with 0 filesz and misaligned starts X-SA-Exim-Version: 4.2.1 (built Sat, 08 Feb 2020 21:53:50 +0000) X-SA-Exim-Scanned: Yes (on in02.mta.xmission.com) X-Rspam-User: X-Rspamd-Server: rspam06 X-Rspamd-Queue-Id: DEF7580006 X-Stat-Signature: 3wuqcqhsgmc6jwr3joyn7mpyjq7guoff X-HE-Tag: 1695698853-454248 X-HE-Meta: U2FsdGVkX1/N6ZZZ7RuHZdLqn41faDhtILAuHKuvFXHP17uLf/5NQUkjp4ylPynHXD9gZi8he6Q5IFB6Ts3kUo4YeuUA33RTiRXVdSM0qz7lyHn5bYQeSrY/ruqa0vDT+75a+LCxAZe4Y7VV84hmSGUqthcadquReyRV+mWtoqc02rScPlN2TUbKttDjCIq3RmeLbcdgKaEHiXR1sY6aslax7xwGGiRM0YFLkV6dLM9C2uk+dzxK524zTx3wbyJeXnVe/y7SwIoZ+02F+i/tu6R0RtppW+8j9J7GUgLG3h0fHk22+cyj+WhM1m1D4EAtP3U0PekJr0epPgdpAlbCvw4Nf/5tQ/5HxGHVEOcR2FIveAmCOJZYd2ioHoSSOv1SRWmcd6ebiYDUFexob6uBis2jjnF6jyKn9hksCKXbNMc29xP0MGtgXYQhBkphmyCF6nyof6rUP3TxOP3qNNaXmIW7yyo8agYXz24Ad1MPaHkh6RTWTOyzxBFvIhBAGHsi6RtTEYsKpwXJ4D7zUcGWaRmQ0wqL1g1sJxSh5vv+YPN6BVR8yWklQJVGIdq6OEMG/KJJDbfAn+MznO1qeWhe5paZHwZZ9+uZ6LW4cCSXgLRHrByCX4WPGV7kwFT5ZV8CEZ4/Pom+FlPUGnzQphfwaNynj2J6OflFi11lqJV3t9Kh7+0Pc9MHVSztGugG5LCPKymy3zFsnOVEzvNNz6Tf17/paAWcNmtv2gB1yCQYbn6u6s4CkhAvhpyJPv6M+4z3hAT4o9QaXu8DIjquUZ5DKLdM71ltaP3b20mEuYUfzNXbxnogjcQPsUd4ACv6083woWI+/q/GEJXa0OaX7zrTI09Qd7Zfa4B6G81RR95qWVDAHts3dmtjZxJjrhm19ODisDzhLHbRyRxFsFMdarPBxhCR2q3lCUt+hSXjcaGydoyMhzzvj8slyX0xDhCGzX9kg+42woyzMVEvRj0lFre eYvmMoFG schlA0ERicZkRZmMxn/0EurtzRsAPj+HXJw/RpWC6JGfgNS/K83FVL7+08c47Nw94mtuxsW56klNwt7uP5Pna7UgxOlj2bIUVL4O2vH7Nui00KhtPLvJwHG6uCczyxZw3U2bbfyazVyW02U5VE4qrJpsZI4gahA2ZUF0lXMZKhbxdRxsncHiOAmzWx2mOjVTT8sHhuJVERmGW2cvu8VKa//j9Uyz9etLj7kOXHs1aVVbPDvQLLl9qT8nTODXXhssvIIEhPFPsXW25qAP9VQvkhvYsN4gTM3L5ewZRVC+s52KlpK1FN1MaXmVvDYFnQXWPVgQy8UwihMGkhH78spOSLzJLsUT4nA8C4HRvslICQWNcqPZMnQU9W83Gi/bfFno3wpJ3oG9EO/GGrgZb5rfwM8cnkWlS4lm6LGecOMt9mypYpKBEdlBQtjKpRRtNd4K+u3YIIW8lYFiNkhSNxnQwuit/LMoJ2BqxWFygkHn0ofGBnpXGY65sjvRTTOxNfXkUplNzXNbtzf3LKaJRANqg6BMlttH55Pf6+dac 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: 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: >> >=20 >> > 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. >> >=20 >> > Link: https://lkml.kernel.org/r/20230914-bss-alloc-v1-1-78de67d2c6dd@w= eissschuh.net >> > Reported-by: Sebastian Ott >> > Reported-by: Thomas Wei=C3=9Fschuh >> > Signed-off-by: "Eric W. Biederman" >> > --- >> > fs/binfmt_elf.c | 111 +++++++++++++++++++++--------------------------- >> > 1 file changed, 48 insertions(+), 63 deletions(-) >> >=20 >> > 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. 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 >=20 > [PATCH] binfmt_elf: clearing bss may fail >=20=20=20=20=20 > So we discover that Borland's Kylix application builder emits weird e= lf > files which describe a non-writeable bss segment. >=20=20=20=20=20 > So remove the clear_user() check at the place where we zero out the b= ss. I > don't _think_ there are any security implications here (plus we've ne= ver > checked that clear_user() return value, so whoops if it is a problem). >=20=20=20=20=20 > 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. 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. 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. Eric