This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH] elf: Try not pointing empty PT_LOAD segment's offset past EOF
Alan Modra <amodra@gmail.com> ezt írta (időpont: 2019. dec. 11., Sze, 12:02):
>
> On Wed, Dec 11, 2019 at 08:53:30AM +0100, Bálint Réczey wrote:
> > Hi Alan,
> >
> > Alan Modra <amodra@gmail.com> ezt írta (időpont: 2019. dec. 11., Sze, 1:44):
> > >
> > > On Tue, Dec 10, 2019 at 09:44:28PM +0100, Bálint Réczey wrote:
> > > > Hi,
> > > >
> > > > Florian Weimer <fweimer@redhat.com> ezt írta (időpont: 2019. dec. 10.,
> > > > K, 14:23):
> > > > >
> > > > > * Alan Modra:
> > > > >
> > > > > > On Sat, Dec 07, 2019 at 05:22:13PM +0100, Bálint Réczey wrote:
> > > > > >> diff --git a/bfd/elf.c b/bfd/elf.c
> > > > > >> index 1aa2603ee8..e1a9a02eec 100644
> > > > > >> --- a/bfd/elf.c
> > > > > >> +++ b/bfd/elf.c
> > > > > >> @@ -5752,7 +5752,12 @@ assign_file_positions_for_load_sections (bfd *abfd,
> > > > > >> || (p->p_type == PT_NOTE && bfd_get_format (abfd) == bfd_core))
> > > > > >> {
> > > > > >> if (!m->includes_filehdr && !m->includes_phdrs)
> > > > > >> - p->p_offset = off;
> > > > > >> + if (no_contents)
> > > > > >> + /* Try avoiding pointing past the EOF with this empty segment's
> > > > > >> + p_offset. */
> > > > > >> + p->p_offset = p->p_offset % maxpagesize;
> > > > > >> + else
> > > > > >> + p->p_offset = off;
> > > > > >> else
> > > > > >> {
> > > > > >> file_ptr adjust;
> > > > > >
> > > > > > How did you test this patch? I suspect you are just leaving p_offset
> > > > > > at zero and therefore will cause failures on glibc systems.
> > > > >
> > > > > I think glibc requires ordering by increasing virtual address, per the
> > > > > specification. That does not seem to change here.
> > > > >
> > > > > Testing on glibc is of course recommended, though. It also needs to be
> > > > > tested with various kernel versions.
> > > >
> > > > For the record I've tested the rebuilt gzip binary on Ubuntu's
> > > > 5.0.0-37-generic and on 5.3.0-24-generic.
> > >
> > > "p->p_offset = p->p_offset % maxpagesize" is not the best way to write
> > > "p->p_offset = 0". So I'd reject this patch for not doing as the
> > > submitter intended.
> >
> > I'm sorry but my intention was keeping p_offset mod maxpagesize ==
> > p_vaddr mod maxpagesize and as I understand it is not guaranteed when
> > just doing p->p_offset = 0.
> > Keeping the modulo was suggested by you in bugzilla, and thanks for that.
> >
> > > Using zero will almost certainly fail for some binaries.
> >
> > Please tell how can I find/build such binaries, I believe changing
> > only the offset in the file keeping should not cause problem anywhere.
> > If zero indeed causes problems I propose doing:
>
> This is the code in glibc/elf/dl-load.c that will trigger.
>
> if (__glibc_unlikely (((ph->p_vaddr - ph->p_offset)
> & (ph->p_align - 1)) != 0))
> {
> errstring
> = N_("ELF load command address/offset not properly aligned");
> goto call_lose;
> }
>
> I can't give you a recipe for building a binary that might fail, off
> the top of my head.
As I read this it triggers only when the alignment does not match. In
gzip's case p_vaddr is aligned, thus (ph->p_vaddr - ph->p_offset is)
aligned thus it does not trigger.
> >
> > if (p->p_offset > maxpagesize) p->p_offset -= maxpagesize;
> >
> > , but I believe % maxpagesize is simpler and still works.
>
> You're missing the fact that p_offset hasn't been assigned in most
> cases at that point.
Yeah, that's fair.
> "p->p_offset = off % maxpagesize" might be OK, I think.
That's perfect, thanks. Attaching the updated patch.
Cheers,
Balint
From 482a90eb29854ff02a342e0424e3ae41fe1a8cbf Mon Sep 17 00:00:00 2001
From: Balint Reczey <balint.reczey@canonical.com>
Date: Fri, 29 Nov 2019 23:58:00 +0100
Subject: [PATCH] elf: Try not pointing empty PT_LOAD segment's offset past EOF
While this did not cause problems in the past, it crashes WSL's ELF loader.
https://launchpad.net/bugs/1843479
---
bfd/elf.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/bfd/elf.c b/bfd/elf.c
index 1aa2603ee8..3d1deeaf89 100644
--- a/bfd/elf.c
+++ b/bfd/elf.c
@@ -5752,7 +5752,12 @@ assign_file_positions_for_load_sections (bfd *abfd,
|| (p->p_type == PT_NOTE && bfd_get_format (abfd) == bfd_core))
{
if (!m->includes_filehdr && !m->includes_phdrs)
- p->p_offset = off;
+ if (no_contents)
+ /* Try avoiding pointing past the EOF with this empty segment's
+ p_offset. */
+ p->p_offset = off % maxpagesize;
+ else
+ p->p_offset = off;
else
{
file_ptr adjust;
--
2.17.1