[musl] Invalid page size reference in __dls2

[musl] Invalid page size reference in __dls2

[Markus Wichmann]

Hi all,

__dls2 calls kernel_mapped_dso(), and that one uses the PAGE_SIZE macro.
Whenever <bits/limits.h> does not define PAGESIZE, PAGE_SIZE is defined
as libc.page_size. That variable is only initialized at the start of
__dls3, so the DSO descriptor for libc ends up being wrong.

Since the libc object has static storage duration, page_size is
initialized with zero. So at least nothing undefined happens. The impact
is, it will calculate the relro pointers as being zero, so no relro will
happen, and it will calculate maximum and minimum addresses as being
zero, therefore setting map to base and map_len to zero. This will cause
dladdr() not to find the libc. Yeah, not the biggest of impacts.

This, again, affects all architectures that don't define PAGESIZE, so at
this time those are

aarch64
arm
m68k
microblaze
mips
mips64
mipsn32
powerpc
powerpc64
riscv64

I don't know whether references to libc are even valid in __dls2, but it
is defined as "hidden", so that ought to be good enough. In that case it
may be enough to just move the initialization. Otherwise it may be
necessary to add page size as parameter to kernel_mapped_dso(). Then
__dls2 can look it up in the aux vector at its leisure.

Ciao,
Markus

Re: [musl] Invalid page size reference in __dls2

[Rich Felker]

[-- Attachment #1: Type: text/plain, Size: 1577 bytes --]

On Mon, Nov 28, 2022 at 08:47:40PM +0100, Markus Wichmann wrote:
> Hi all,
> 
> __dls2 calls kernel_mapped_dso(), and that one uses the PAGE_SIZE macro.
> Whenever <bits/limits.h> does not define PAGESIZE, PAGE_SIZE is defined
> as libc.page_size. That variable is only initialized at the start of
> __dls3, so the DSO descriptor for libc ends up being wrong.
> 
> Since the libc object has static storage duration, page_size is
> initialized with zero. So at least nothing undefined happens. The impact
> is, it will calculate the relro pointers as being zero, so no relro will
> happen, and it will calculate maximum and minimum addresses as being
> zero, therefore setting map to base and map_len to zero. This will cause
> dladdr() not to find the libc. Yeah, not the biggest of impacts.
> 
> This, again, affects all architectures that don't define PAGESIZE, so at
> this time those are
> 
> aarch64
> arm
> m68k
> microblaze
> mips
> mips64
> mipsn32
> powerpc
> powerpc64
> riscv64
> 
> I don't know whether references to libc are even valid in __dls2, but it
> is defined as "hidden", so that ought to be good enough. In that case it
> may be enough to just move the initialization. Otherwise it may be
> necessary to add page size as parameter to kernel_mapped_dso(). Then
> __dls2 can look it up in the aux vector at its leisure.

Nice catch. The references to libc are not valid in __dls2. If they
were, I would just re-run kernel_mapped_dso() from __dls2b or
something to get the right relro map, but I think instead we should do
something like the attached.

Rich

[-- Attachment #2: ldso_page_size.diff --]
[-- Type: text/plain, Size: 752 bytes --]

diff --git a/ldso/dynlink.c b/ldso/dynlink.c
index 8068fb37..fb13a7b1 100644
--- a/ldso/dynlink.c
+++ b/ldso/dynlink.c
@@ -21,9 +21,15 @@
 #include <sys/membarrier.h>
 #include "pthread_impl.h"
 #include "fork_impl.h"
-#include "libc.h"
 #include "dynlink.h"
 
+static size_t ldso_page_size;
+#ifndef PAGE_SIZE
+define PAGE_SIZE ldso_page_size;
+#endif
+
+#include "libc.h"
+
 #define malloc __libc_malloc
 #define calloc __libc_calloc
 #define realloc __libc_realloc
@@ -1723,6 +1729,7 @@ hidden void __dls2(unsigned char *base, size_t *sp)
 	ldso.phnum = ehdr->e_phnum;
 	ldso.phdr = laddr(&ldso, ehdr->e_phoff);
 	ldso.phentsize = ehdr->e_phentsize;
+	search_vec(auxv, &ldso_page_size, AT_PAGESZ);
 	kernel_mapped_dso(&ldso);
 	decode_dyn(&ldso);
 

Re: [musl] Invalid page size reference in __dls2

[A. Wilcox]

[-- Attachment #1: Type: text/plain, Size: 480 bytes --]


On Nov 30, 2022, at 9:12 AM, Rich Felker <dalias@libc.org> wrote:
> 
> Nice catch. The references to libc are not valid in __dls2. If they
> were, I would just re-run kernel_mapped_dso() from __dls2b or
> something to get the right relro map, but I think instead we should do
> something like the attached.
> 
> Rich
> <ldso_page_size.diff>

LGTM but needs a # before the ‘define’.

This explains a weird error I was seeing with gcompat on aarch64.

Best,
-A.

[-- Attachment #2: Type: text/html, Size: 5248 bytes --]

Re: [musl] Invalid page size reference in __dls2

[Rich Felker]

[-- Attachment #1: Type: text/plain, Size: 585 bytes --]

On Wed, Nov 30, 2022 at 05:28:38PM -0600, A. Wilcox wrote:
> 
> On Nov 30, 2022, at 9:12 AM, Rich Felker <dalias@libc.org> wrote:
> > 
> > Nice catch. The references to libc are not valid in __dls2. If they
> > were, I would just re-run kernel_mapped_dso() from __dls2b or
> > something to get the right relro map, but I think instead we should do
> > something like the attached.
> > 
> > Rich
> > <ldso_page_size.diff>
> 
> LGTM but needs a # before the ‘define’.
> 
> This explains a weird error I was seeing with gcompat on aarch64.

Does this look better (message included)?


[-- Attachment #2: 0001-ldso-fix-invalid-early-references-to-extern-linkage-.patch --]
[-- Type: text/plain, Size: 2218 bytes --]

From f47a8cdd250d9163fcfb39bf4e9d813957c0b187 Mon Sep 17 00:00:00 2001
From: Rich Felker <dalias@aerifal.cx>
Date: Wed, 30 Nov 2022 18:59:08 -0500
Subject: [PATCH] ldso: fix invalid early references to extern-linkage
 libc.page_size

when PAGE_SIZE is not constant, internal/libc.h defines it to expand
to libc.page_size. however, kernel_mapped_dso, reachable from stage 2
of the dynamic linker bootstrap (__dls2), needs PAGE_SIZE to interpret
the relro range. at this point the libc object is both uninitialized
and invalid to access according to our model for bootstrapping, which
does not assume any external-linkage objects are accessible until
stages 2b/3. in practice it likely worked because hidden visibility
tends to behave like internal linkage, but this is not a property that
the dynamic linker was designed to rely upon.

this bug likely manifested as relro malfunction on archs with variable
page size, due to incorrect mask when aligning the relro bounds to
page boundaries.

while there are certainly more direct ways to fix the known problem
point here, a maximally future-proof way is to just bypass the libc.h
PAGE_SIZE definition in the dynamic linker and instead have dynlink.c
define its own internal-linkage object for variable page size. then,
if anything else in stage 2 ever ends up referencing PAGE_SIZE, it
will just automatically work right.
---
 ldso/dynlink.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/ldso/dynlink.c b/ldso/dynlink.c
index 8068fb37..09f3b0a8 100644
--- a/ldso/dynlink.c
+++ b/ldso/dynlink.c
@@ -21,9 +21,15 @@
 #include <sys/membarrier.h>
 #include "pthread_impl.h"
 #include "fork_impl.h"
-#include "libc.h"
 #include "dynlink.h"
 
+static size_t ldso_page_size;
+#ifndef PAGE_SIZE
+#define PAGE_SIZE ldso_page_size
+#endif
+
+#include "libc.h"
+
 #define malloc __libc_malloc
 #define calloc __libc_calloc
 #define realloc __libc_realloc
@@ -1723,6 +1729,7 @@ hidden void __dls2(unsigned char *base, size_t *sp)
 	ldso.phnum = ehdr->e_phnum;
 	ldso.phdr = laddr(&ldso, ehdr->e_phoff);
 	ldso.phentsize = ehdr->e_phentsize;
+	search_vec(auxv, &ldso_page_size, AT_PAGESZ);
 	kernel_mapped_dso(&ldso);
 	decode_dyn(&ldso);
 
-- 
2.21.0

Re: [musl] Invalid page size reference in __dls2

[A. Wilcox]

[-- Attachment #1: Type: text/plain, Size: 199 bytes --]

On Nov 30, 2022, at 6:09 PM, Rich Felker <dalias@libc.org> wrote:
> 
> Does this look better (message included)?
> 
> <0001-ldso-fix-invalid-early-references-to-extern-linkage-.patch>

LGTM, +1.

-A.

[-- Attachment #2: Type: text/html, Size: 1741 bytes --]