| Message ID | 20230607144227.8956-1-ansuelsmth@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | binfmt_elf: dynamically allocate note.data in parse_elf_properties | expand |
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?
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
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
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 --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, ¬e, 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; }
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(-)