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 DF9D0EE14A9 for ; Thu, 7 Sep 2023 01:10:29 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id C804F280024; Wed, 6 Sep 2023 21:10:28 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id C2F13280023; Wed, 6 Sep 2023 21:10:28 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id B1E6F280024; Wed, 6 Sep 2023 21:10:28 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id 9E977280023 for ; Wed, 6 Sep 2023 21:10:28 -0400 (EDT) Received: from smtpin06.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id 6CCD6C0BE9 for ; Thu, 7 Sep 2023 01:10:28 +0000 (UTC) X-FDA: 81208020936.06.C9FD9AC Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by imf13.hostedemail.com (Postfix) with ESMTP id A05AF20016 for ; Thu, 7 Sep 2023 01:10:26 +0000 (UTC) Authentication-Results: imf13.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=fIq6LNc3; spf=pass (imf13.hostedemail.com: domain of gerg@kernel.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=gerg@kernel.org; dmarc=pass (policy=none) header.from=kernel.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1694049026; 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=zBHXK82sCxmhz+HmHCYkWj5A99i/EHZgeCRAfspBao8=; b=cuJwSC9RBspQm73oBgMvhKi04DNSuc7GedeUnYBkrenJ0bCMxWawsckgTuq3xCnLGKq3ZA +AQam01gKRipEXToCzpZzUf7H7spfhuBeTMhrg3jUTiS2Lb10PtL2LgbEs0jQcakGzvC51 NMpeSFO6p8O86SUG+ftWdge830/nZb0= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1694049026; a=rsa-sha256; cv=none; b=Wm6BOzoHG9lkEF5W9tH1vtYfQl2r3q/VdNahiSYDo701X5R3wr/tl9iUsUQBnjS/jExYmS 2cv8mv6YOPHnA4cbS/4fRccCh6bdrrAmLs2FITsmN28zwU1ZBiOsuOPhl3a+CMuBK8ahjM vk+uv8cfbI/kyNE8U38C4fWCwkIe83w= ARC-Authentication-Results: i=1; imf13.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=fIq6LNc3; spf=pass (imf13.hostedemail.com: domain of gerg@kernel.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=gerg@kernel.org; dmarc=pass (policy=none) header.from=kernel.org Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id B7871611D6; Thu, 7 Sep 2023 01:10:25 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id B3352C433C8; Thu, 7 Sep 2023 01:10:23 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1694049025; bh=NVYr91ZTB33fuG48FDFSm4hUOros8XmqjUrEi2FOsKY=; h=Date:Subject:From:To:Cc:References:In-Reply-To:From; b=fIq6LNc3zpP4anpSAGaaL2DHHTWePT+5PVo3PgzNcIDlWjTAJFTze/pFvUQnshqJt R6AWijwpR5f50MMrVQJXR+Hy8qumTl5r6A28AMoRbA02Fr+Zvrja+hgxO0SSSosDQ3 AFycUeXVRBMneUeOBJXMIAxSqeE5JqQqpIaBnniO5pTD5es5eErukcN0eEf29f+YuC fx7hGzjbOHHfpXNYvdHVxEKONUmssORXUMYLZd6EEc2KnK5zWfxIPCoOnFin2LcyvP e0uBZCz1iQIiLWE2BxW7K/weE/+NKjUJjx95/vbu9o9yrAUbZlWRL9EOew9og+p3rs 96jD4veTmvguw== Message-ID: <2556121f-d086-e79b-b1d7-c33eedaa05ac@kernel.org> Date: Thu, 7 Sep 2023 11:10:21 +1000 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.13.0 Subject: Re: [PATCH] fs: binfmt_elf_efpic: fix personality for fdpic ELF Content-Language: en-US From: Greg Ungerer To: Kees Cook Cc: linux-arm@lists.infradead.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, viro@zeniv.linux.org.uk, ebiederm@xmission.com, brauner@kernel.org References: <20230711133955.483393-1-gerg@kernel.org> <202307110901.7E9A0D0AE5@keescook> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Rspamd-Queue-Id: A05AF20016 X-Rspam-User: X-Stat-Signature: bbbu7oj88qhhktgwfsqopoay3w3686fq X-Rspamd-Server: rspam03 X-HE-Tag: 1694049026-99069 X-HE-Meta: U2FsdGVkX1/zqya+rKV/kSrmGTR+3usjVirDZ91wWs7feP5bDeiHp6yG3eBLrfWkeXNkpg2phYKpqsczdDRd66xt1KDPOazGmI0tqSY6cWBo3878UPq/db1bUlcZ+dvRd1cbJLRilPBto351FpgBcPBY+5XH4L4B3Q5wv2Epi0eOVsewQvYa+nwG+W043rBNJK0Dz1H6ZfqtmxlwNPTFE6/Vi0bCOD+s0E2z61S9WuvwieT6TN3JPnmlHgURa0vRxvPkeqqONopGTSUUTRPYZpgIXJXvzH9VNMq2zlH60jEN6hNwKw/UrE2EyUYM+p9YPSovJm8wEXnMxTROGr3zgsPehV7U/39w1B/y7RVt8Gw1EDBMaRsE+K5Q1tzk2cTIusnVSS+sD7h9Ck0UebVBDghITKY7UTfCXtn1dY+cRh1YPzUfM0n5WVsoG6R91Y/dgnCwHk67YlSo+SGx1dMCypLhBTZuoWTp087PMEKMCtKo/GvSWe+7/gzdeGxPUpgyX79dseGBX/AiKHKWs4aQ5HbMZrpeYBusUBU/1zqufJz5UfHpe5gEBkWCrgEpUcHeKLTVljjRs6BmLeUsWELxcRSAyBuTvoPUnPynfi1ozR/O7PyGRapHW2iGP+Pya25kLviGGxn97SqzjEa2/NIlQhkWKTGYy6/U15Solda0C92hUM8S1k/DMwmfjC2/Ij2ohzpTn52r9wzU5p4HS+1/UFzViANTnImW3ayDDngEAK4O9ZOdCkZna2nXEdS3Ao2idQQoGbS/ksy6x2qMX/vYJtVoBLq7/oORHa2/hu1vsT0bADo962F635G9PahAyEsnh1TjQLKRMrfb5ZazL6ceNtmJ0KE/nLUnoUVZgr3xKWwEYTWOB9BrDBjIQjG/qe4EBUr7pHQcx1bl9OtyTZmHDHvL+LaBDXQbzvZtJ83qiV4JzK/hMeOqUhU4o6+IgqGUGGttBzXUFDmC+Xssozs ehKRPDQu nwrVrvtctGUfzHi90qzzBk/pRkj4Ve+/UN7GQU5UPar+rbyBJZcdUz+ZAoEJcjnLBjafHIGXq8al8zmNNFTRSIiGodzv1Q+jP2iVxuEuZ3SQKd/pukwdgigYeSsPNmEVq42UUQoHlLnhYA5X1j8QEZKxeovkROqlH9uYWDICNOxiP3p5D9kQMrPhavasPysQaETj/haoC593fOzoqvD5yi9hdJMKKCgeDAPLFR6rYz6kKSrIjAaAPBryJLJegLCRaqTZDIyBzBqkAtPpkaaYk6aPzvOPmM2FAvtlM9/1m9c7TjdqL+PHbZLSzNuewLayXsUBp5dlZnaQTf5tue2SNk04o8mFsv2on92cMF1KoHUlsLR7BHicnNueMHX18DOkIbJCwc9pD+8KKoBM= 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 12/7/23 13:43, Greg Ungerer wrote: > Hi Kees, > > On 12/7/23 02:11, Kees Cook wrote: >> On Tue, Jul 11, 2023 at 11:39:55PM +1000, Greg Ungerer wrote: >>> The elf-fdpic loader hard sets the process personality to either >>> PER_LINUX_FDPIC for true elf-fdpic binaries or to PER_LINUX for >>> normal ELF binaries (in this case they would be constant displacement >>> compiled with -pie for example). The problem with that is that it >>> will lose any other bits that may be in the ELF header personality >>> (such as the "bug emulation" bits). >>> >>> On the ARM architecture the ADDR_LIMIT_32BIT flag is used to signify >>> a normal 32bit binary - as opposed to a legacy 26bit address binary. >>> This matters since start_thread() will set the ARM CPSR register as >>> required based on this flag. If the elf-fdpic loader loses this bit >>> the process will be mis-configured and crash out pretty quickly. >>> >>> Modify elf-fdpic loaders personality setting for ELF binaries so that >>> it preserves the upper three bytes by using the SET_PERSONALITY macro >>> to set it. This macro in the generic case sets PER_LINUX but and >>> preserves the upper bytes. Architectures can override this for their >>> specific use case, and ARM does exactly this. >> >> Thanks for tracking this down! >> >> There are some twisty macros in use across all the architectures here! >> >> I notice the bare set_personality() call remains, though. Is that right? >> >> For example, ARM (and sh and xtensa) also sets: >> >> #define elf_check_fdpic(x) ((x)->e_ident[EI_OSABI] == ELFOSABI_ARM_FDPIC) >> >> so it's possible the first half of the "if" below could get executed, >> and ARM (and possibly other architectures) would again lose the other >> flags, if I'm reading correctly. > > Yes, it is all a little confusing, and the fdpic handling is a little different > to the standard ELF handling in binfmt_elf.c (with its use of SET_PERSONALITY2). > > >> (And the fact that PER_LINUX is actually 0x0 is oddly handled, leaving >> it implicit in most architectures.) >> >> What seems perhaps more correct is to remove the "if" entirely and make >> sure that SET_PERSONALITY() checks the header flags on all architectures? > > I had thought along those same lines as well. Changing it to be something more > like this: > >     SET_PERSONALITY(exec_params.hdr); >     if (elf_check_fdpic(&exec_params.hdr)) >             current->personality |= FDPIC_FUNCPTRS; > > Which I think better handles any arch specifics via the SET_PERSONALITY() use. > But I chickened out since I can't test fdpic binaries at this time. I have done some more extensive testing, with fdpic setups now, and this definitely suffers the same problem. So it needs a fix more like this. I am generating a v2 that essentially does the above - so it fixes both the normal ELF and ELF-FDPOIC binary cases. Regards Greg >> But I'm less familiar with this area, so please let me know what I'm >> missing. :) > > Me too :-) > It is definitely broken for loading standard ELF binaries on a noMMU system > using binfmt_elf_fdpic.c, which is what led me down this path. It loses the > ADDR_LIMIT_32BIT bit in the personality and that causes application crashing. > > >>> Signed-off-by: Greg Ungerer >>> --- >>> >>> Is anyone out there using elf-fdpic on ARM? >> >> It would seem you're the first? :) (_Should_ it be usable on ARM?) > > I was assuming that it must have worked at some time. The binfmt_elf_fdpic > loader was enabled for ARM in commit 50b2b2e691cd ("ARM: add ELF_FDPIC support") > by Nicolas Pitre. But that was way back in 2017. > > Regards > Greg > > >> -Kees >> >>> This seems to break it rather badly due to the loss of that ADDR_LIMIT_32BIT >>> bit from the process personality. >>> >>>   fs/binfmt_elf_fdpic.c | 2 +- >>>   1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c >>> index a05eafcacfb2..f29ae1d96fd7 100644 >>> --- a/fs/binfmt_elf_fdpic.c >>> +++ b/fs/binfmt_elf_fdpic.c >>> @@ -348,7 +348,7 @@ static int load_elf_fdpic_binary(struct linux_binprm *bprm) >>>       if (elf_check_fdpic(&exec_params.hdr)) >>>           set_personality(PER_LINUX_FDPIC); >>>       else >>> -        set_personality(PER_LINUX); >>> +        SET_PERSONALITY(exec_params.hdr); >>>       if (elf_read_implies_exec(&exec_params.hdr, executable_stack)) >>>           current->personality |= READ_IMPLIES_EXEC; >>> -- >>> 2.25.1 >>> >>