diff mbox series

binfmt_elf: dynamically allocate note.data in parse_elf_properties

Message ID 20230607144227.8956-1-ansuelsmth@gmail.com
State New
Headers
Series binfmt_elf: dynamically allocate note.data in parse_elf_properties |

Commit Message

Christian Marangi June 7, 2023, 2:42 p.m. UTC 
  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(-)

Comments

Christian Marangi June 7, 2023, 6:31 p.m. UTC | #1 
On Wed, Jun 07, 2023 at 02:19:51PM -0700, Kees Cook wrote:
> On Wed, Jun 07, 2023 at 04:42:27PM +0200, Christian Marangi wrote:
> > Dynamically allocate note.data in parse_elf_properties to fix
> > compilation warning on some arch.
> 
> I'd rather avoid dynamic allocation as much as possible in the exec
> path, but we can balance it against how much it may happen.
>

I guess there isn't a good way to handle this other than static global
variables and kmalloc. But check the arch question for additional info
on the case.

> > 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
> 
> Which architectures see this warning?
> 

This is funny. On OpenWRT we are enforcing WERROR and we had FRAME_WARN
hardcoded to 1024. (the option is set to 2048 on 64bit arch)

ARCH_USE_GNU_PROPERTY is set only on arm64 that have a FRAME_WARN set to
2048.

So this was triggered by building arm64 with FRAME_WARN set to 1024.

Now with the configuration of 2048 the stack warn is not triggered, but
I wonder if it may happen to have a 32bit system with
ARCH_USE_GNU_PROPERTY. That would effectively trigger the warning.

So this is effectively a patch that fix a currently not possible
configuration, since:

!IS_ENABLED(CONFIG_ARCH_USE_GNU_PROPERTY) will result in node.data
effectively never allocated by the compiler are the function will return
0 on everything that doesn't have CONFIG_ARCH_USE_GNU_PROPERTY.

> > Fix this by dynamically allocating the array.
> > Update the sizeof of the union to the biggest element allocated.
> 
> How common are these notes? I assume they're very common; I see them
> even in /bin/true:
> 
> $ readelf -lW /bin/true | grep PROP
>   GNU_PROPERTY   0x000338 0x0000000000000338 0x0000000000000338 0x000030 0x000030 R   0x8
> 
> -- 

Is there a way to check if this kmalloc actually cause perf regression?
Kees Cook June 7, 2023, 9:19 p.m. UTC | #2 
On Wed, Jun 07, 2023 at 04:42:27PM +0200, Christian Marangi wrote:
> Dynamically allocate note.data in parse_elf_properties to fix
> compilation warning on some arch.

I'd rather avoid dynamic allocation as much as possible in the exec
path, but we can balance it against how much it may happen.

> 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

Which architectures see this warning?

> Fix this by dynamically allocating the array.
> Update the sizeof of the union to the biggest element allocated.

How common are these notes? I assume they're very common; I see them
even in /bin/true:

$ readelf -lW /bin/true | grep PROP
  GNU_PROPERTY   0x000338 0x0000000000000338 0x0000000000000338 0x000030 0x000030 R   0x8
Kees Cook June 7, 2023, 11:37 p.m. UTC | #3 
On Wed, Jun 07, 2023 at 08:31:58PM +0200, Christian Marangi wrote:
> On Wed, Jun 07, 2023 at 02:19:51PM -0700, Kees Cook wrote:
> > On Wed, Jun 07, 2023 at 04:42:27PM +0200, Christian Marangi wrote:
> > > Dynamically allocate note.data in parse_elf_properties to fix
> > > compilation warning on some arch.
> > 
> > I'd rather avoid dynamic allocation as much as possible in the exec
> > path, but we can balance it against how much it may happen.
> >
> 
> I guess there isn't a good way to handle this other than static global
> variables and kmalloc. But check the arch question for additional info
> on the case.
> 
> > > 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
> > 
> > Which architectures see this warning?
> > 
> 
> This is funny. On OpenWRT we are enforcing WERROR and we had FRAME_WARN
> hardcoded to 1024. (the option is set to 2048 on 64bit arch)

Ah-ha. Okay, I was wondering how you got that. :)

> ARCH_USE_GNU_PROPERTY is set only on arm64 that have a FRAME_WARN set to
> 2048.
> 
> So this was triggered by building arm64 with FRAME_WARN set to 1024.
> 
> Now with the configuration of 2048 the stack warn is not triggered, but
> I wonder if it may happen to have a 32bit system with
> ARCH_USE_GNU_PROPERTY. That would effectively trigger the warning.
> 
> So this is effectively a patch that fix a currently not possible
> configuration, since:
> 
> !IS_ENABLED(CONFIG_ARCH_USE_GNU_PROPERTY) will result in node.data
> effectively never allocated by the compiler are the function will return
> 0 on everything that doesn't have CONFIG_ARCH_USE_GNU_PROPERTY.
> 
> > > Fix this by dynamically allocating the array.
> > > Update the sizeof of the union to the biggest element allocated.
> > 
> > How common are these notes? I assume they're very common; I see them
> > even in /bin/true:
> > 
> > $ readelf -lW /bin/true | grep PROP
> >   GNU_PROPERTY   0x000338 0x0000000000000338 0x0000000000000338 0x000030 0x000030 R   0x8
> > 
> > -- 
> 
> Is there a way to check if this kmalloc actually cause perf regression?

I don't have a good benchmark besides just an exec loop. But since this
isn't reachable in a regular config, I'd rather keep things how there
already are.

-Kees
Eric W. Biederman June 12, 2023, 8:08 a.m. UTC | #4 
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
diff mbox series

Patch

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;
 	loff_t pos;
 	ssize_t n;
@@ -785,29 +785,41 @@  static int parse_elf_properties(struct file *f, const struct elf_phdr *phdr,
 		return -ENOEXEC;
 
 	/* If the properties are crazy large, that's too bad (for now): */
-	if (phdr->p_filesz > sizeof(note))
+	if (phdr->p_filesz > sizeof(*note.data) * NOTE_DATA_SZ)
 		return -ENOEXEC;
 
+	note.data = kcalloc(NOTE_DATA_SZ, sizeof(*note.data), GFP_KERNEL);
+	if (!note.data)
+		return -ENOMEM;
+
 	pos = phdr->p_offset;
 	n = kernel_read(f, &note, phdr->p_filesz, &pos);
 
-	BUILD_BUG_ON(sizeof(note) < sizeof(note.nhdr) + NOTE_NAME_SZ);
-	if (n < 0 || n < sizeof(note.nhdr) + NOTE_NAME_SZ)
-		return -EIO;
+	BUILD_BUG_ON(sizeof(*note.data) * NOTE_DATA_SZ < sizeof(note.nhdr) + NOTE_NAME_SZ);
+	if (n < 0 || n < sizeof(note.nhdr) + NOTE_NAME_SZ) {
+		ret = -EIO;
+		goto exit;
+	}
 
 	if (note.nhdr.n_type != NT_GNU_PROPERTY_TYPE_0 ||
 	    note.nhdr.n_namesz != NOTE_NAME_SZ ||
 	    strncmp(note.data + sizeof(note.nhdr),
-		    GNU_PROPERTY_TYPE_0_NAME, n - sizeof(note.nhdr)))
-		return -ENOEXEC;
+		    GNU_PROPERTY_TYPE_0_NAME, n - sizeof(note.nhdr))) {
+		ret = -ENOEXEC;
+		goto exit;
+	}
 
 	off = round_up(sizeof(note.nhdr) + NOTE_NAME_SZ,
 		       ELF_GNU_PROPERTY_ALIGN);
-	if (off > n)
-		return -ENOEXEC;
+	if (off > n) {
+		ret = -ENOEXEC;
+		goto exit;
+	}
 
-	if (note.nhdr.n_descsz > n - off)
-		return -ENOEXEC;
+	if (note.nhdr.n_descsz > n - off) {
+		ret = -ENOEXEC;
+		goto exit;
+	}
 	datasz = off + note.nhdr.n_descsz;
 
 	have_prev_type = false;
@@ -817,6 +829,8 @@  static int parse_elf_properties(struct file *f, const struct elf_phdr *phdr,
 		have_prev_type = true;
 	} while (!ret);
 
+exit:
+	kfree(note.data);
 	return ret == -ENOENT ? 0 : ret;
 }