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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id EC1F3C7EE23 for ; Mon, 12 Jun 2023 08:42:36 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233181AbjFLIme (ORCPT ); Mon, 12 Jun 2023 04:42:34 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:46930 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233370AbjFLImd (ORCPT ); Mon, 12 Jun 2023 04:42:33 -0400 X-Greylist: delayed 2019 seconds by postgrey-1.37 at lindbergh.monkeyblade.net; Mon, 12 Jun 2023 01:42:31 PDT Received: from out03.mta.xmission.com (out03.mta.xmission.com [166.70.13.233]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 6EDF3B7; Mon, 12 Jun 2023 01:42:30 -0700 (PDT) Received: from in02.mta.xmission.com ([166.70.13.52]:49274) by out03.mta.xmission.com with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.93) (envelope-from ) id 1q8cba-00GVnS-6T; Mon, 12 Jun 2023 02:08:50 -0600 Received: from ip68-110-29-46.om.om.cox.net ([68.110.29.46]:41498 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 1q8cbY-00F8nl-Ss; Mon, 12 Jun 2023 02:08:49 -0600 From: "Eric W. Biederman" To: Christian Marangi Cc: Alexander Viro , Christian Brauner , Kees Cook , Mark Brown , Dave Martin , Catalin Marinas , linux-fsdevel@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, stable@vger.kernel.org References: <20230607144227.8956-1-ansuelsmth@gmail.com> Date: Mon, 12 Jun 2023 03:08:21 -0500 In-Reply-To: <20230607144227.8956-1-ansuelsmth@gmail.com> (Christian Marangi's message of "Wed, 7 Jun 2023 16:42:27 +0200") Message-ID: <875y7tumq2.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 X-XM-SPF: eid=1q8cbY-00F8nl-Ss;;;mid=<875y7tumq2.fsf@email.froward.int.ebiederm.org>;;;hst=in02.mta.xmission.com;;;ip=68.110.29.46;;;frm=ebiederm@xmission.com;;;spf=pass X-XM-AID: U2FsdGVkX19mPd4u3NrhQg/U/AK1EfBVUc3XuMgnfKI= X-SA-Exim-Connect-IP: 68.110.29.46 X-SA-Exim-Mail-From: ebiederm@xmission.com Subject: Re: [PATCH] binfmt_elf: dynamically allocate note.data in parse_elf_properties 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) Precedence: bulk List-ID: X-Mailing-List: stable@vger.kernel.org Christian Marangi writes: > Dynamically allocate note.data in parse_elf_properties to fix > compilation warning on some arch. > > On some arch note.data exceed the stack limit for a single function and > this cause the following compilation warning: > fs/binfmt_elf.c: In function 'parse_elf_properties.isra': > fs/binfmt_elf.c:821:1: error: the frame size of 1040 bytes is larger than 1024 bytes [-Werror=frame-larger-than=] > 821 | } > | ^ > cc1: all warnings being treated as errors > > Fix this by dynamically allocating the array. > Update the sizeof of the union to the biggest element allocated. > > Fixes: 00e19ceec80b ("ELF: Add ELF program property parsing support") > Signed-off-by: Christian Marangi > Cc: stable@vger.kernel.org # v5.8+ > --- > fs/binfmt_elf.c | 36 +++++++++++++++++++++++++----------- > 1 file changed, 25 insertions(+), 11 deletions(-) > > diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c > index 44b4c42ab8e8..90daa623ca13 100644 > --- a/fs/binfmt_elf.c > +++ b/fs/binfmt_elf.c > @@ -768,7 +768,7 @@ static int parse_elf_properties(struct file *f, const struct elf_phdr *phdr, > { > union { > struct elf_note nhdr; > - char data[NOTE_DATA_SZ]; > + char *data; > } note; If you are going to dynamically allocate this not that way please. Only dynamically allocating one member of a union is to put it politely completely broken. The entire union needs to be dynamically allocated. Given that the entire thing is 1024 bytes in size. I can understand the concern. Hmm. The entire union is a buffer for the entire note section. So 1K is understandable if it needs to hold all of the notes. Of course only a single note is a wrapper of a bunch of gnu_properties. Hopefully that single note comes first. The notehdr + name take 16 bytes. The only supported gnu_property takes 12 bytes. I think 16 in practice. Hmm. So we could do with a smaller buffer. Hmm. The code does not check that all phdr->p_filesz bytes are actually read. So I would suggest defining the union to be ELF_EXEC_PAGESIZE bytes, dynamically allocating it like we do all of the other buffers we read elf headers into, and then use elf_read to verify that we read all of phdr->p_filesz bytes. Just like we do for the elf program headers. I think having a second pattern for reading data is more likely to be a problem than a dynamic memory allocation. Especially since this code only runs on one architecture in practice. The changes will cost nothing except on arm64 and it will be as cheap as it can be, being simply a single page allocation. Either that or you can up your stack limit on 64bit architectures like everyone else. Eric