elf: Check invalid hole in PT_LOAD segments [BZ #28838] (original) (raw)

Checks

Commit Message

H.J. Lu Jan. 31, 2022, 3:24 p.m. UTC

commit 163f625cf9becbb82dfec63a29e566324129c0cd Author: H.J. Lu hjl.tools@gmail.com Date: Tue Dec 21 12:35:47 2021 -0800

elf: Remove excessive p_align check on PT_LOAD segments [BZ #28688]

removed the p_align check against the page size. It caused the loader crash in shared objects with the invalid p_align. Update _dl_map_segments to detect invalid holes. This fixes BZ #28838.

elf/dl-map-segments.h | 3 +++ 1 file changed, 3 insertions(+)

Comments

• H. J. Lu:

commit 163f625cf9becbb82dfec63a29e566324129c0cd Author: H.J. Lu hjl.tools@gmail.com Date: Tue Dec 21 12:35:47 2021 -0800

elf: Remove excessive p_align check on PT_LOAD segments [BZ #28688]

removed the p_align check against the page size. It caused the loader crash in shared objects with the invalid p_align. Update _dl_map_segments to detect invalid holes. This fixes BZ #28838.

Commit message should reference commit ID and mention the failing test/module name.

---

elf/dl-map-segments.h | 3 +++ 1 file changed, 3 insertions(+)

diff --git a/elf/dl-map-segments.h b/elf/dl-map-segments.h index 172692b120..fd24cf5d01 100644 --- a/elf/dl-map-segments.h +++ b/elf/dl-map-segments.h @@ -113,6 +113,9 @@ _dl_map_segments (struct link_map *l, int fd, unallocated. Then jump into the normal segment-mapping loop to handle the portion of the segment past the end of the file mapping. */ + if (_glibc_unlikely (loadcmds[nloadcmds - 1].mapstart < + c->mapend)) + return N("ELF load command address/offset not page-aligned"); if (__glibc_unlikely (__mprotect ((caddr_t) (l->l_addr + c->mapend), loadcmds[nloadcmds - 1].mapstart - c->mapend,

This seems to be fairly risky because I don't think that so far, we enforce increasing LOAD segment addresses (although required by te EHF specification).

Given

LDFLAGS-tst-p_alignmod3.so += -Wl,-z,max-page-size=0x100,-z,common-page-size=0x100

and RELRO construction for that

.../elf/tst-p_alignmod3.so: cannot change memory protections

seems to be a valid failure string for this test. However, worst case, there could be a different kind of failure, if the RELRO mprotect start is page-aligned by chance, and the kernel rounds up the end address to a page boundary. The RELRO protection then covers more than what the link editor expected, and this can cause crashes later on. But this isn't something we can detect easily, I think.

Thanks, Florian

On Mon, Jan 31, 2022 at 7:40 AM Florian Weimer fweimer@redhat.com wrote:

• H. J. Lu:

commit 163f625cf9becbb82dfec63a29e566324129c0cd Author: H.J. Lu hjl.tools@gmail.com Date: Tue Dec 21 12:35:47 2021 -0800

elf: Remove excessive p_align check on PT_LOAD segments [BZ #28688]

removed the p_align check against the page size. It caused the loader crash in shared objects with the invalid p_align. Update _dl_map_segments to detect invalid holes. This fixes BZ #28838.

Commit message should reference commit ID and mention the failing test/module name.

---

elf/dl-map-segments.h | 3 +++ 1 file changed, 3 insertions(+)

diff --git a/elf/dl-map-segments.h b/elf/dl-map-segments.h index 172692b120..fd24cf5d01 100644 --- a/elf/dl-map-segments.h +++ b/elf/dl-map-segments.h @@ -113,6 +113,9 @@ _dl_map_segments (struct link_map *l, int fd, unallocated. Then jump into the normal segment-mapping loop to handle the portion of the segment past the end of the file mapping. */

•   if (__glibc_unlikely (loadcmds[nloadcmds - 1].mapstart <

•                         c->mapend))

•     return N_("ELF load command address/offset not page-aligned");
       if (__glibc_unlikely
           (__mprotect ((caddr_t) (l->l_addr + c->mapend),
                        loadcmds[nloadcmds - 1].mapstart - c->mapend,
                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

If loadcmds[nloadcmds - 1].mapstart < c->mapend, the length argument passed to __mprotect is a huge number and invalid.

This seems to be fairly risky because I don't think that so far, we enforce increasing LOAD segment addresses (although required by te EHF specification).

Given

LDFLAGS-tst-p_alignmod3.so += -Wl,-z,max-page-size=0x100,-z,common-page-size=0x100

and RELRO construction for that

.../elf/tst-p_alignmod3.so: cannot change memory protections

seems to be a valid failure string for this test. However, worst case, there could be a different kind of failure, if the RELRO mprotect start is page-aligned by chance, and the kernel rounds up the end address to a page boundary. The RELRO protection then covers more than what the link editor expected, and this can cause crashes later on. But this isn't something we can detect easily, I think.

Thanks, Florian

On Mon, Jan 31, 2022 at 7:40 AM Florian Weimer fweimer@redhat.com wrote:

• H. J. Lu:

commit 163f625cf9becbb82dfec63a29e566324129c0cd Author: H.J. Lu hjl.tools@gmail.com Date: Tue Dec 21 12:35:47 2021 -0800

elf: Remove excessive p_align check on PT_LOAD segments [BZ #28688]

removed the p_align check against the page size. It caused the loader crash in shared objects with the invalid p_align. Update _dl_map_segments to detect invalid holes. This fixes BZ #28838.

Commit message should reference commit ID and mention the failing test/module name.

How about this?

commit 163f625cf9becbb82dfec63a29e566324129c0cd Author: H.J. Lu hjl.tools@gmail.com Date: Tue Dec 21 12:35:47 2021 -0800

elf: Remove excessive p_align check on PT_LOAD segments [BZ #28688]

removed the p_align check against the page size. It caused the loader crash on elf/tst-p_align3 when loading elf/tst-p_alignmod3.so, which has the invalid p_align in PT_LOAD segments, added by

commit d8d94863ef125a392b929732b37e07dc927fbcd1 Author: H.J. Lu hjl.tools@gmail.com Date: Tue Dec 21 13:42:28 2021 -0800

The loader crash is random, depending on architecture and toolchain. Update _dl_map_segments to detect invalid holes. This fixes BZ #28838.

---

elf/dl-map-segments.h | 3 +++ 1 file changed, 3 insertions(+)

diff --git a/elf/dl-map-segments.h b/elf/dl-map-segments.h index 172692b120..fd24cf5d01 100644 --- a/elf/dl-map-segments.h +++ b/elf/dl-map-segments.h @@ -113,6 +113,9 @@ _dl_map_segments (struct link_map *l, int fd, unallocated. Then jump into the normal segment-mapping loop to handle the portion of the segment past the end of the file mapping. */

•   if (__glibc_unlikely (loadcmds[nloadcmds - 1].mapstart <

•                         c->mapend))

•     return N_("ELF load command address/offset not page-aligned");
       if (__glibc_unlikely
           (__mprotect ((caddr_t) (l->l_addr + c->mapend),
                        loadcmds[nloadcmds - 1].mapstart - c->mapend,

This seems to be fairly risky because I don't think that so far, we enforce increasing LOAD segment addresses (although required by te EHF specification).

Given

LDFLAGS-tst-p_alignmod3.so += -Wl,-z,max-page-size=0x100,-z,common-page-size=0x100

and RELRO construction for that

.../elf/tst-p_alignmod3.so: cannot change memory protections

seems to be a valid failure string for this test. However, worst case, there could be a different kind of failure, if the RELRO mprotect start is page-aligned by chance, and the kernel rounds up the end address to a page boundary. The RELRO protection then covers more than what the link editor expected, and this can cause crashes later on. But this isn't something we can detect easily, I think.

Thanks, Florian

On Tue, 1 Feb 2022 at 04:24, H.J. Lu hjl.tools@gmail.com wrote:

commit 163f625cf9becbb82dfec63a29e566324129c0cd Author: H.J. Lu hjl.tools@gmail.com Date: Tue Dec 21 12:35:47 2021 -0800

elf: Remove excessive p_align check on PT_LOAD segments [BZ #28688]

removed the p_align check against the page size. It caused the loader crash in shared objects with the invalid p_align. Update _dl_map_segments to detect invalid holes. This fixes BZ #28838.

I am not competent to have an opinion on the correctness of the change, but the test passes on Launchapd's i386 and amd64 builders with it.

Cheers, mwh

---

elf/dl-map-segments.h | 3 +++ 1 file changed, 3 insertions(+)

diff --git a/elf/dl-map-segments.h b/elf/dl-map-segments.h index 172692b120..fd24cf5d01 100644 --- a/elf/dl-map-segments.h +++ b/elf/dl-map-segments.h @@ -113,6 +113,9 @@ _dl_map_segments (struct link_map *l, int fd, unallocated. Then jump into the normal segment-mapping loop to handle the portion of the segment past the end of the file mapping. */ + if (_glibc_unlikely (loadcmds[nloadcmds - 1].mapstart < + c->mapend)) + return N("ELF load command address/offset not page-aligned"); if (__glibc_unlikely (__mprotect ((caddr_t) (l->l_addr + c->mapend), loadcmds[nloadcmds - 1].mapstart - c->mapend,

2.34.1

Patch

@@ -113,6 +113,9 @@ _dl_map_segments (struct link_map *l, int fd, unallocated. Then jump into the normal segment-mapping loop to handle the portion of the segment past the end of the file mapping. */ + if (_glibc_unlikely (loadcmds[nloadcmds - 1].mapstart < + c->mapend)) + return N("ELF load command address/offset not page-aligned"); if (__glibc_unlikely (__mprotect ((caddr_t) (l->l_addr + c->mapend), loadcmds[nloadcmds - 1].mapstart - c->mapend,