[Bug dynamic-link/24484] New: RISC-V libraries can have read-write .dynamic sections, but ld.so loads them as read-only

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

[Bug dynamic-link/24484] New: RISC-V libraries can have read-write .dynamic sections, but ld.so loads them as read-only

agner at agner dot org
https://sourceware.org/bugzilla/show_bug.cgi?id=24484

            Bug ID: 24484
           Summary: RISC-V libraries can have read-write .dynamic
                    sections, but ld.so loads them as read-only
           Product: glibc
           Version: 2.29
            Status: NEW
          Severity: normal
          Priority: P2
         Component: dynamic-link
          Assignee: unassigned at sourceware dot org
          Reporter: palmer at gcc dot gnu.org
  Target Milestone: ---

David pointed this out on the mailing list:

While working on enabling D front-end (GDC) in GCC we noticed that druntime
was segfaulting if it is linked dynamically. This was tracked to
DL_RO_DYN_SECTION.

DL_RO_DYN_SECTION lines seem to be copied from MIPS file (which is the only
user of it), but the comment doesn't apply to RISC-V. There is no such
requirement in RISC-V ABI.

and Jim did a lot of analysis:

> > While working on enabling D front-end (GDC) in GCC we noticed that druntime
> > was segfaulting if it is linked dynamically. This was tracked to
> > DL_RO_DYN_SECTION.

My analysis of this when it first came up...

I believe this was blindly copied from MIPS, and that no one on the
RISC-V side will know why it is there, because it was blindly copied
from MIPS.

Looking at the MIPS SVR4 ABI, I see
.dynamic This is the same as the generic ABI section of the same type, but
the MIPS-specific version does not include the SHF_WRITE at-
tribute.

I don't see any other reason in the spec for this to be read-only, but
making it read-only probably improves program security.  I don't see
an equivalent statement in the RISC-V ABI.

Looking at bfd/elfxx-mips.c _bfd_mips_elf_create_dynamic_sections(), we see
code
  flags = (SEC_ALLOC | SEC_LOAD | SEC_HAS_CONTENTS | SEC_IN_MEMORY
           | SEC_LINKER_CREATED | SEC_READONLY);

  /* The psABI requires a read-only .dynamic section, but the VxWorks
     EABI doesn't.  */
  if (!htab->is_vxworks)
    {
      s = bfd_get_linker_section (abfd, ".dynamic");
      if (s != NULL)
        {
          if (! bfd_set_section_flags (abfd, s, flags))
            return FALSE;
        }
    }
that forces .dynamic to be read-only everywhere except vxworks.  I
don't see equivalent code in the RISC-V bfd port.

Looking at ld, in emulparams/, there are multiple mips scripts that
set TEXT_DYNAMIC, which forces .dynamic into the text segment instead
of the data segment.  This is then unset in the vxworks scripts.
Interestingly, hppa also sets TEXT_DYNAMIC, but the hppa support in
glibc is probably not well maintained.  Anyways, there is no
equivalent code for RISC-V.

So it appears unnecessary for the RISC-V glibc port.

If MIPS dynamic linking support is ever added to the druntime library,
then it will need the same change that you made for RISC-V, so in that
case it would not be a RISC-V specific change.  However, there aren't
a lot of people doing MIPS development work anymore, so this may never
come up.

Jim
PS Since I wrote my analysis, MIPS has been resurrected, so maybe
someone will care about that again.

Florian pointed out that a bugzilla entry was needed, so I've gone and opened
one.

--
You are receiving this mail because:
You are on the CC list for the bug.
Reply | Threaded
Open this post in threaded view
|

[Bug dynamic-link/24484] RISC-V libraries can have read-write .dynamic sections, but ld.so loads them as read-only

agner at agner dot org
https://sourceware.org/bugzilla/show_bug.cgi?id=24484

Florian Weimer <fweimer at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
                 CC|                            |fweimer at redhat dot com
         Resolution|---                         |FIXED
   Target Milestone|---                         |2.30
              Flags|                            |security-

--- Comment #1 from Florian Weimer <fweimer at redhat dot com> ---
Fixed by this commit for glibc 2.30 (not picked up automatically because of the
unexpected bug number syntax):

commit deacca0054a1c42151ac027171ef3c2aba6bc566
Author: David Abdurachmanov <[hidden email]>
Date:   Tue Apr 9 13:25:29 2019 +0200

    riscv: remove DL_RO_DYN_SECTION

    While working on enabling D front-end (GDC) in GCC we noticed that
    druntime was segfaulting if it is linked dynamically. This was tracked
    to DL_RO_DYN_SECTION.

    DL_RO_DYN_SECTION lines seem to be copied from MIPS file (which is the
    only user of it), but the comment doesn't apply to RISC-V. There is no
    such requirement in RISC-V ABI.

            [BZ#24484]
            * sysdeps/riscv/ldsodefs.h: Remove DL_RO_DYN_SECTION as it is not
            required by RISC-V ABI.

--
You are receiving this mail because:
You are on the CC list for the bug.
Reply | Threaded
Open this post in threaded view
|

[Bug dynamic-link/24484] RISC-V libraries can have read-write .dynamic sections, but ld.so loads them as read-only

agner at agner dot org
In reply to this post by agner at agner dot org
https://sourceware.org/bugzilla/show_bug.cgi?id=24484

--- Comment #2 from Andreas Schwab <[hidden email]> ---
Isn't that an ABI break?

--
You are receiving this mail because:
You are on the CC list for the bug.
Reply | Threaded
Open this post in threaded view
|

[Bug dynamic-link/24484] RISC-V libraries can have read-write .dynamic sections, but ld.so loads them as read-only

agner at agner dot org
In reply to this post by agner at agner dot org
https://sourceware.org/bugzilla/show_bug.cgi?id=24484

Andreas Schwab <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |REOPENED
         Resolution|FIXED                       |---

--- Comment #3 from Andreas Schwab <[hidden email]> ---
I think this needs to be reverted, and this bug should be closed as WONTFIX.

--
You are receiving this mail because:
You are on the CC list for the bug.
Reply | Threaded
Open this post in threaded view
|

[Bug dynamic-link/24484] RISC-V libraries can have read-write .dynamic sections, but ld.so loads them as read-only

agner at agner dot org
In reply to this post by agner at agner dot org
https://sourceware.org/bugzilla/show_bug.cgi?id=24484

--- Comment #4 from Florian Weimer <fweimer at redhat dot com> ---
(In reply to Andreas Schwab from comment #2)
> Isn't that an ABI break?

Would you please explain the nature of the ABI break?  Thanks.

Do you expect tools to get confused because the dynamic section is now
relocated at run time?

--
You are receiving this mail because:
You are on the CC list for the bug.
Reply | Threaded
Open this post in threaded view
|

[Bug dynamic-link/24484] RISC-V libraries can have read-write .dynamic sections, but ld.so loads them as read-only

agner at agner dot org
In reply to this post by agner at agner dot org
https://sourceware.org/bugzilla/show_bug.cgi?id=24484

--- Comment #5 from Andreas Schwab <[hidden email]> ---
It breaks libphobos in gcc 9.

--
You are receiving this mail because:
You are on the CC list for the bug.
Reply | Threaded
Open this post in threaded view
|

[Bug dynamic-link/24484] RISC-V libraries can have read-write .dynamic sections, but ld.so loads them as read-only

agner at agner dot org
In reply to this post by agner at agner dot org
https://sourceware.org/bugzilla/show_bug.cgi?id=24484

--- Comment #6 from Florian Weimer <fweimer at redhat dot com> ---
(In reply to Andreas Schwab from comment #5)
> It breaks libphobos in gcc 9.

In which way?  Does it try to parse the dynamic section?

--
You are receiving this mail because:
You are on the CC list for the bug.
Reply | Threaded
Open this post in threaded view
|

[Bug dynamic-link/24484] RISC-V libraries can have read-write .dynamic sections, but ld.so loads them as read-only

agner at agner dot org
In reply to this post by agner at agner dot org
https://sourceware.org/bugzilla/show_bug.cgi?id=24484

--- Comment #7 from Florian Weimer <fweimer at redhat dot com> ---
Looks like this is expected to change, from
libphobos/libdruntime/gcc/sections/elf_shared.d:

  version (CRuntime_Musl)
      strtab = cast(const(char)*)(info.dlpi_addr + dyn.d_un.d_ptr); // relocate
  else version (linux)
  {
      // This might change in future glibc releases (after 2.29) as dynamic
sections
      // are not required to be read-only on RISC-V. This was copy & pasted
from MIPS
      // while upstreaming RISC-V support. Otherwise MIPS is the only arch
which sets
      // in glibc: #define DL_RO_DYN_SECTION 1
      version (RISCV_Any)
          strtab = cast(const(char)*)(info.dlpi_addr + dyn.d_un.d_ptr); //
relocate
      else
          strtab = cast(const(char)*)dyn.d_un.d_ptr;
  }

Looks like this code needs fixing on MIPS, too.

Is there an easy way to detect whether the dynamic segment has been relocated?
Then we should just at the check to libphobos.

--
You are receiving this mail because:
You are on the CC list for the bug.
Reply | Threaded
Open this post in threaded view
|

[Bug dynamic-link/24484] RISC-V libraries can have read-write .dynamic sections, but ld.so loads them as read-only

agner at agner dot org
In reply to this post by agner at agner dot org
https://sourceware.org/bugzilla/show_bug.cgi?id=24484

--- Comment #8 from Andreas Schwab <[hidden email]> ---
You cannot change it, the ship has sailed.

--
You are receiving this mail because:
You are on the CC list for the bug.
Reply | Threaded
Open this post in threaded view
|

[Bug dynamic-link/24484] RISC-V libraries can have read-write .dynamic sections, but ld.so loads them as read-only

agner at agner dot org
In reply to this post by agner at agner dot org
https://sourceware.org/bugzilla/show_bug.cgi?id=24484

--- Comment #9 from Palmer Dabbelt <palmer at gcc dot gnu.org> ---
Sorry, I'm still a bit confused about this being an ABI break.  If the ABI
doesn't require these sections to be read-only then isn't this an
implementation bug in glibc as opposed to an ABI break?  If I understand
correctly we were already producing these read-write sections in binutils, so
it's really a difference between binutils and glibc -- either we change
binutils and the ABI document to match what glibc is doing, or we change glibc
to match what binutils and the ABI document say.  In this case I'd prefer to
have the glibc change, as it seems like the more normal direction.

Am I misunderstanding something?

--
You are receiving this mail because:
You are on the CC list for the bug.
Reply | Threaded
Open this post in threaded view
|

[Bug dynamic-link/24484] RISC-V libraries can have read-write .dynamic sections, but ld.so loads them as read-only

agner at agner dot org
In reply to this post by agner at agner dot org
https://sourceware.org/bugzilla/show_bug.cgi?id=24484

--- Comment #10 from Florian Weimer <fweimer at redhat dot com> ---
(In reply to Palmer Dabbelt from comment #9)
> Sorry, I'm still a bit confused about this being an ABI break.  If the ABI
> doesn't require these sections to be read-only then isn't this an
> implementation bug in glibc as opposed to an ABI break?

If the dynamic segment isn't read-only, glibc relocates it, so the address
values contained in it change.  I think this is the nature of the ABI break,
not the read-write vs read-only status as such.

--
You are receiving this mail because:
You are on the CC list for the bug.
Reply | Threaded
Open this post in threaded view
|

[Bug dynamic-link/24484] RISC-V libraries can have read-write .dynamic sections, but ld.so loads them as read-only

agner at agner dot org
In reply to this post by agner at agner dot org
https://sourceware.org/bugzilla/show_bug.cgi?id=24484

Jim Wilson <wilson at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |wilson at gcc dot gnu.org

--- Comment #11 from Jim Wilson <wilson at gcc dot gnu.org> ---
The problem here is that the D language runtime library libphobos has some code
identical to the D_PTR macro in sysdeps/generic/ldsodefs.h which depends on the
value of DL_RO_DYN_SECTION.  This is a static compile time check in the
libphobos code.  So changing the value of DL_RO_DYN_SECTION breaks the D
language support.  We can make the same change to libphobos, but that leaves us
in the situation where a new libphobos won't work with an old glibc, and an old
libphobos won't work with a new glibc.  That is the ABI break.

There is no RISC-V D language support before the gcc-9.1.0 release, which
happened about a week ago, so we have some time to fix this if we are quick.
We can just say that RISC-V D isn't supported in 9.1 and requires 9.2 for
instance.  This can be fixed if libphobos is changed to use a dynamic run-time
check for the DL_RO_DYN_SECTION value.  But this looks complicated.  We can't
use DL_RO_DYN_SECTION or D_PTR because those are glibc internal macros, and you
can't include a C language header file in a D language source file anyways.
The code in elf_get_dynamic_info() in elf/get-dynamic-info.h that uses
DL_RO_DYN_SECTION doesn't set any flags to indicate whether it did anything or
not.  So there doesn't seem to be anything we can easily check there.  We have
the phdrs, but the dynamic section was already read/write from the beginning,
so that doesn't help.  It seems that we need to add a new feature to glibc so
that the application can query whether the target has DL_RO_DYN_SECTION set or
not, which is returned via a function not a macro because we can't use C header
files, and then modify libphobos to use the new feature, in a failsafe way,
e.g. if the feature doesn't exist then we use the old scheme where MIPS and
RISC-V have DL_RO_DYN_SECTION set, and if the feature exists, then we use the
new scheme of checking the feature value to see if the target has
DL_RO_DYN_SECTION set.  And we need to do all of this before the gcc-9.2
release.

This seems like a awful lot of trouble for what appears to be an minor feature.
 There is also the possibility that there may be other programs or libraries
that have the same problem, and we just haven't noticed yet.  The simpler
solution is to just accept that we can't change the value of DL_RO_DYN_SECTION
unless we are doing a major ABI break for some other reason.

--
You are receiving this mail because:
You are on the CC list for the bug.