Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   "Eric W. Biederman" <ebiederm@xmission.com>
To:     Christian Marangi <ansuelsmth@gmail.com>
Cc:     Alexander Viro <viro@zeniv.linux.org.uk>,
        Christian Brauner <brauner@kernel.org>,
        Kees Cook <keescook@chromium.org>,
        Mark Brown <broonie@kernel.org>,
        Dave Martin <Dave.Martin@arm.com>,
        Catalin Marinas <catalin.marinas@arm.com>,
        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
Subject: Re: [PATCH] binfmt_elf: dynamically allocate note.data in
 parse_elf_properties
Precedence: bulk

Christian Marangi <ansuelsmth@gmail.com> 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 <ansuelsmth@gmail.com>
> 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