[PATCH] elf: Try not pointing empty PT_LOAD segment's offset past EOF

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] elf: Try not pointing empty PT_LOAD segment's offset past EOF

Bálint Réczey
Hi,

Strip sometimes leaves file offset of empty PT_LOAD segment pointing
past end of file as observed on Ubuntu 19.10's gzip binary. This is
traditionally considered a non-issue as loaders tolerated that, but
WSL1's ELF loader does not run the gzip binary [1] due to this offset.

I have opened a bug [2] in bugzilla, but I was advised to send the
patch here, too.

Please consider accepting the patch. I believe this does not cause any
regression and it fixes ELF loading on WSL.

Thanks,
Balint

[1] https://github.com/microsoft/WSL/issues/4461
[2] https://sourceware.org/bugzilla/show_bug.cgi?id=25237

0001-elf-Try-not-pointing-empty-PT_LOAD-segment-s-offset-.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] elf: Try not pointing empty PT_LOAD segment's offset past EOF

Alan Modra-3
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.

--
Alan Modra
Australia Development Lab, IBM
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] elf: Try not pointing empty PT_LOAD segment's offset past EOF

Bálint Réczey
Hi Alan,

Alan Modra <[hidden email]> ezt írta (időpont: 2019. dec. 8., V, 3:12):

>
> 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've recompiled gzip in a PPA [1] with the modified binutils and also
a few other packages since then and they seem to work.

Observing the generalted binary I indeed found that the offset is
zero, but it did not cause any problem and the first PT_LOAD's file
offset is also zero.

New gzip:
$ readelf  --program-headers /bin/gzip
...
Program Headers:
  Type           Offset             VirtAddr           PhysAddr
                 FileSiz            MemSiz              Flags  Align
  PHDR           0x0000000000000040 0x0000000000000040 0x0000000000000040
                 0x0000000000000310 0x0000000000000310  R      0x8
  INTERP         0x0000000000000350 0x0000000000000350 0x0000000000000350
                 0x000000000000001c 0x000000000000001c  R      0x1
      [Requesting program interpreter: /lib64/ld-linux-x86-64.so.2]
  LOAD           0x0000000000000000 0x0000000000000000 0x0000000000000000
                 0x00000000000027a0 0x00000000000027a0  R      0x1000
  LOAD           0x0000000000003000 0x0000000000003000 0x0000000000003000
                 0x000000000000ea05 0x000000000000ea05  R E    0x1000
  LOAD           0x0000000000012000 0x0000000000012000 0x0000000000012000
                 0x0000000000003e80 0x0000000000003e80  R      0x1000
  LOAD           0x0000000000016690 0x0000000000017690 0x0000000000017690
                 0x0000000000000d70 0x0000000000000d70  RW     0x1000
  LOAD           0x0000000000000000 0x000000000001a000 0x000000000001a000
                 0x0000000000000000 0x00000000000ca048  RW     0x1000
  DYNAMIC        0x0000000000016b80 0x0000000000017b80 0x0000000000017b80
                 0x00000000000001f0 0x00000000000001f0  RW     0x8
...

The difference between the original gzip and the one built with the
patched binutils:

$ diff -Naur orig.elfdump new.elfdump
--- orig.elfdump    2019-12-08 11:59:57.544897124 +0000
+++ new.elfdump    2019-12-08 12:22:37.930722784 +0000
@@ -14,12 +14,12 @@
   LOAD           0x0000000000000000 0x0000000000000000 0x0000000000000000
                  0x00000000000027a0 0x00000000000027a0  R      0x1000
   LOAD           0x0000000000003000 0x0000000000003000 0x0000000000003000
-                 0x000000000000ea15 0x000000000000ea15  R E    0x1000
+                 0x000000000000ea05 0x000000000000ea05  R E    0x1000
   LOAD           0x0000000000012000 0x0000000000012000 0x0000000000012000
                  0x0000000000003e80 0x0000000000003e80  R      0x1000
   LOAD           0x0000000000016690 0x0000000000017690 0x0000000000017690
                  0x0000000000000d70 0x0000000000000d70  RW     0x1000
-  LOAD           0x0000000000018000 0x000000000001a000 0x000000000001a000
+  LOAD           0x0000000000000000 0x000000000001a000 0x000000000001a000
                  0x0000000000000000 0x00000000000ca048  RW     0x1000
   DYNAMIC        0x0000000000016b80 0x0000000000017b80 0x0000000000017b80
                  0x00000000000001f0 0x00000000000001f0  RW     0x8
$

Cheers,
Balint

[1] https://launchpad.net/~rbalint/+archive/ubuntu/scratch/+packages
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] elf: Try not pointing empty PT_LOAD segment's offset past EOF

Bálint Réczey
Hi Alan,

Bálint Réczey <[hidden email]> ezt írta (időpont: 2019. dec.
8., V, 13:27):

>
> Hi Alan,
>
> Alan Modra <[hidden email]> ezt írta (időpont: 2019. dec. 8., V, 3:12):
> >
> > 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've recompiled gzip in a PPA [1] with the modified binutils and also
> a few other packages since then and they seem to work.
>
> Observing the generalted binary I indeed found that the offset is
> zero, but it did not cause any problem and the first PT_LOAD's file
> offset is also zero.

Is there anything else that should be tested?

Cheers,
Balint
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] elf: Try not pointing empty PT_LOAD segment's offset past EOF

Florian Weimer-5
In reply to this post by Alan Modra-3
* 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.

Thanks,
Florian

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] elf: Try not pointing empty PT_LOAD segment's offset past EOF

Bálint Réczey
Hi,

Florian Weimer <[hidden email]> 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.

Cheers,
Balint
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] elf: Try not pointing empty PT_LOAD segment's offset past EOF

Alan Modra-3
On Tue, Dec 10, 2019 at 09:44:28PM +0100, Bálint Réczey wrote:

> Hi,
>
> Florian Weimer <[hidden email]> 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.

Using zero will almost certainly fail for some binaries.

--
Alan Modra
Australia Development Lab, IBM
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] elf: Try not pointing empty PT_LOAD segment's offset past EOF

Bálint Réczey
Hi Alan,

Alan Modra <[hidden email]> 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 <[hidden email]> 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:

if (p->p_offset > maxpagesize) p->p_offset -= maxpagesize;

, but I believe % maxpagesize is simpler and still works.

Cheers,
Balint

>
> --
> Alan Modra
> Australia Development Lab, IBM
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] elf: Try not pointing empty PT_LOAD segment's offset past EOF

Alan Modra-3
On Wed, Dec 11, 2019 at 08:53:30AM +0100, Bálint Réczey wrote:

> Hi Alan,
>
> Alan Modra <[hidden email]> 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 <[hidden email]> 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.

>
> 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.

"p->p_offset = off % maxpagesize" might be OK, I think.

--
Alan Modra
Australia Development Lab, IBM
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] elf: Try not pointing empty PT_LOAD segment's offset past EOF

Bálint Réczey
Alan Modra <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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

0001-elf-Try-not-pointing-empty-PT_LOAD-segment-s-offset-.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] elf: Try not pointing empty PT_LOAD segment's offset past EOF

Bálint Réczey
Hi Alan,

Bálint Réczey <[hidden email]> ezt írta (időpont: 2019. dec.
11., Sze, 12:44):
>
> Alan Modra <[hidden email]> ezt írta (időpont: 2019. dec. 11., Sze, 12:02):
> >
...
> > "p->p_offset = off % maxpagesize" might be OK, I think.
>
> That's perfect, thanks. Attaching the updated patch.

I have attached the updated patch to to bug, too.
Would you like me to perform any specific additional testing?

Thanks,
Balint
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] elf: Try not pointing empty PT_LOAD segment's offset past EOF

Alan Modra-3
On Fri, Dec 13, 2019 at 10:32:19AM +0100, Bálint Réczey wrote:

> Hi Alan,
>
> Bálint Réczey <[hidden email]> ezt írta (időpont: 2019. dec.
> 11., Sze, 12:44):
> >
> > Alan Modra <[hidden email]> ezt írta (időpont: 2019. dec. 11., Sze, 12:02):
> > >
> ...
> > > "p->p_offset = off % maxpagesize" might be OK, I think.
> >
> > That's perfect, thanks. Attaching the updated patch.
>
> I have attached the updated patch to to bug, too.
> Would you like me to perform any specific additional testing?

I've run the binutils testsuite over multiple targets without seeing
any problems so will commit the following variant.

        PR 25237
        * elf.c (assign_file_positions_for_load_sections): Attempt to
        keep meaningless p_offset for PT_LOAD segments without file
        contents within file size.

diff --git a/bfd/elf.c b/bfd/elf.c
index 1aa2603ee8..fd447fdb28 100644
--- a/bfd/elf.c
+++ b/bfd/elf.c
@@ -5752,7 +5752,15 @@ 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;
+    {
+      p->p_offset = off;
+      if (no_contents)
+ /* Put meaningless p_offset for PT_LOAD segments
+   without file contents somewhere within the first
+   page, in an attempt to not point past EOF.  */
+ p->p_offset = off % (p->p_align > maxpagesize
+     ? p->p_align : maxpagesize);
+    }
   else
     {
       file_ptr adjust;


--
Alan Modra
Australia Development Lab, IBM
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] elf: Try not pointing empty PT_LOAD segment's offset past EOF

Bálint Réczey
Hi Alan,

Alan Modra <[hidden email]> ezt írta (időpont: 2019. dec. 13., P, 12:36):

>
> On Fri, Dec 13, 2019 at 10:32:19AM +0100, Bálint Réczey wrote:
> > Hi Alan,
> >
> > Bálint Réczey <[hidden email]> ezt írta (időpont: 2019. dec.
> > 11., Sze, 12:44):
> > >
> > > Alan Modra <[hidden email]> ezt írta (időpont: 2019. dec. 11., Sze, 12:02):
> > > >
> > ...
> > > > "p->p_offset = off % maxpagesize" might be OK, I think.
> > >
> > > That's perfect, thanks. Attaching the updated patch.
> >
> > I have attached the updated patch to to bug, too.
> > Would you like me to perform any specific additional testing?
>
> I've run the binutils testsuite over multiple targets without seeing
> any problems so will commit the following variant.
>
>         PR 25237
>         * elf.c (assign_file_positions_for_load_sections): Attempt to
>         keep meaningless p_offset for PT_LOAD segments without file
>         contents within file size.
>
> diff --git a/bfd/elf.c b/bfd/elf.c
> index 1aa2603ee8..fd447fdb28 100644
> --- a/bfd/elf.c
> +++ b/bfd/elf.c
> @@ -5752,7 +5752,15 @@ 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;
> +           {
> +             p->p_offset = off;
> +             if (no_contents)
> +               /* Put meaningless p_offset for PT_LOAD segments
> +                  without file contents somewhere within the first
> +                  page, in an attempt to not point past EOF.  */
> +               p->p_offset = off % (p->p_align > maxpagesize
> +                                    ? p->p_align : maxpagesize);
> +           }
>           else
>             {
>               file_ptr adjust;

Thank you!

Cheers,
Balint