mailing list of musl libc
 help / color / mirror / code / Atom feed
* [musl] Dynamic linker segfault
@ 2022-01-04 21:27 Nihal Jere
  2022-01-04 21:42 ` Rich Felker
  0 siblings, 1 reply; 9+ messages in thread
From: Nihal Jere @ 2022-01-04 21:27 UTC (permalink / raw)
  To: musl

Hi,

ldd from master segfaults when run on [1] and [2]. It happens on
this[3] line. It seems to happen due to the intersection of a few
factors:

1. The segment at the lowest address is read-only.
2. A segment on the the same page is read/write.
3. The read/write segment has memsz > filesz.

This results in a segfault, as it tries to memset[3] on the mmap
created here[4], which has the same protection as the segment at
the lowest address (i.e. read-only).

As far as I can see, the options are to either:

a. detect this and throw an error.
b. 'or' together the protection flags of all the segments on the page.

I'm not sure what the right behavior is, but I don't think segfaulting
is right, and I'm sure there are people here what's correct.

Best,
Nihal

[1] https://github.com/golang/go/blob/master/src/debug/elf/testdata/go-relocation-test-gcc930-ranges-no-rela-x86-64
[2] https://github.com/golang/go/blob/master/src/debug/elf/testdata/go-relocation-test-gcc930-ranges-with-rela-x86-64
[3] https://git.musl-libc.org/cgit/musl/tree/ldso/dynlink.c#n783
[4] https://git.musl-libc.org/cgit/musl/tree/ldso/dynlink.c#n742

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [musl] Dynamic linker segfault
  2022-01-04 21:27 [musl] Dynamic linker segfault Nihal Jere
@ 2022-01-04 21:42 ` Rich Felker
  2022-01-05  8:56   ` Florian Weimer
  0 siblings, 1 reply; 9+ messages in thread
From: Rich Felker @ 2022-01-04 21:42 UTC (permalink / raw)
  To: Nihal Jere; +Cc: musl

On Tue, Jan 04, 2022 at 03:27:10PM -0600, Nihal Jere wrote:
> Hi,
> 
> ldd from master segfaults when run on [1] and [2]. It happens on
> this[3] line. It seems to happen due to the intersection of a few
> factors:
> 
> 1. The segment at the lowest address is read-only.
> 2. A segment on the the same page is read/write.
> 3. The read/write segment has memsz > filesz.

This is a malformed program file not compatible with the machine page
size (4k). Arguably it should be detected as p_align < PAGESIZE -- in
a sense, p_align for LOAD segments is the maximum supported page size
for the program file, and machines not capable of providing a page
size that small can't map/run it. In theory the loader could allow
this if all the differences between segments satisfy the right
congruences and have matching permissions where the maps would
overlap, but I'm not sure that's useful.

What tooling generated this file?

> This results in a segfault, as it tries to memset[3] on the mmap
> created here[4], which has the same protection as the segment at
> the lowest address (i.e. read-only).
> 
> As far as I can see, the options are to either:
> 
> a. detect this and throw an error.
> b. 'or' together the protection flags of all the segments on the page.
> 
> I'm not sure what the right behavior is, but I don't think segfaulting
> is right, and I'm sure there are people here what's correct.

There is a huge range of "syntactically valid but not correct" things
ELF can represent, and musl has never attempted to catch or diagnose
all such errors. It might be desirable to increase the level to which
we catch common ones, but proceeding on this probably depends on how
this file came to be. If it was made by intentionally passing bad
options to the linker or with a custom linker that's not honoring the
semantic requirements for an executable for the platform, I think
there's a lot of lower-hanging fruit if we wanted to start diagnosing
this sort of thing.

Of course diagnosing p_align < PAGESIZE as an error might make sense
in general.

Rich

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [musl] Dynamic linker segfault
  2022-01-04 21:42 ` Rich Felker
@ 2022-01-05  8:56   ` Florian Weimer
  2022-01-05 13:49     ` Rich Felker
  0 siblings, 1 reply; 9+ messages in thread
From: Florian Weimer @ 2022-01-05  8:56 UTC (permalink / raw)
  To: Rich Felker; +Cc: Nihal Jere, musl

* Rich Felker:

> This is a malformed program file not compatible with the machine page
> size (4k). Arguably it should be detected as p_align < PAGESIZE -- in
> a sense, p_align for LOAD segments is the maximum supported page size
> for the program file, and machines not capable of providing a page
> size that small can't map/run it. In theory the loader could allow
> this if all the differences between segments satisfy the right
> congruences and have matching permissions where the maps would
> overlap, but I'm not sure that's useful.

We've been looking at this on the glibc side recently.  The use case is
supporting large data alignments (greater than the kernel page size)
while not pessimizing multi-page-size targets such as POWER and AArch64.
With a p_align < PAGESIZE check, binaries need to specify p_align as the
largest support kernel page size (64K on those targets).  On a 64K page
kernel, this alignment happens naturally, but on a 4K (or 16K) kernel,
the loader needs to carve out a larger memory area and align it manually
to achieve 64K alignment (because that's what p_align says).  So far
this didn't happen because we only checked p_align against the kernel
size, we did not actually use it for aligning the mappings …

Thanks,
Florian


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [musl] Dynamic linker segfault
  2022-01-05  8:56   ` Florian Weimer
@ 2022-01-05 13:49     ` Rich Felker
  2022-01-05 14:00       ` Florian Weimer
  0 siblings, 1 reply; 9+ messages in thread
From: Rich Felker @ 2022-01-05 13:49 UTC (permalink / raw)
  To: Florian Weimer; +Cc: Nihal Jere, musl

On Wed, Jan 05, 2022 at 09:56:25AM +0100, Florian Weimer wrote:
> * Rich Felker:
> 
> > This is a malformed program file not compatible with the machine page
> > size (4k). Arguably it should be detected as p_align < PAGESIZE -- in
> > a sense, p_align for LOAD segments is the maximum supported page size
> > for the program file, and machines not capable of providing a page
> > size that small can't map/run it. In theory the loader could allow
> > this if all the differences between segments satisfy the right
> > congruences and have matching permissions where the maps would
> > overlap, but I'm not sure that's useful.
> 
> We've been looking at this on the glibc side recently.  The use case is
> supporting large data alignments (greater than the kernel page size)
> while not pessimizing multi-page-size targets such as POWER and AArch64.

I'm not clear how it pessimizes these targets (beyond what's
fundamentally necessary) unless you're artificially aligning segment
contents on disk to a large alignment boundary to prevent over-mapping
(undermining separate-code for example). And if you're doing that, you
need the full alignment anyway to support machines with larger
hardware pagesize. Otherwise you'd get back the overmapping (and
unwanted perission exposure).

The only other thing I can think of is pessimizing ASLR by requiring
more alignment (throwing away a few bits of position entropy).

In any case, do you know if this test file is somehow related to that
work, or is it just a guess? It doesn't seem to be related to me since
it's essentially a "pageless" mapping setup.

Rich

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [musl] Dynamic linker segfault
  2022-01-05 13:49     ` Rich Felker
@ 2022-01-05 14:00       ` Florian Weimer
  2022-01-05 15:00         ` Rich Felker
  0 siblings, 1 reply; 9+ messages in thread
From: Florian Weimer @ 2022-01-05 14:00 UTC (permalink / raw)
  To: Rich Felker; +Cc: Nihal Jere, musl

* Rich Felker:

> On Wed, Jan 05, 2022 at 09:56:25AM +0100, Florian Weimer wrote:
>> * Rich Felker:
>> 
>> > This is a malformed program file not compatible with the machine page
>> > size (4k). Arguably it should be detected as p_align < PAGESIZE -- in
>> > a sense, p_align for LOAD segments is the maximum supported page size
>> > for the program file, and machines not capable of providing a page
>> > size that small can't map/run it. In theory the loader could allow
>> > this if all the differences between segments satisfy the right
>> > congruences and have matching permissions where the maps would
>> > overlap, but I'm not sure that's useful.
>> 
>> We've been looking at this on the glibc side recently.  The use case is
>> supporting large data alignments (greater than the kernel page size)
>> while not pessimizing multi-page-size targets such as POWER and AArch64.
>
> I'm not clear how it pessimizes these targets (beyond what's
> fundamentally necessary) unless you're artificially aligning segment
> contents on disk to a large alignment boundary to prevent over-mapping
> (undermining separate-code for example). And if you're doing that, you
> need the full alignment anyway to support machines with larger
> hardware pagesize. Otherwise you'd get back the overmapping (and
> unwanted perission exposure).

Hmm.  Maybe I should rephrase:

With a p_align < PAGESIZE check in place, portable binaries need to use
the value 65536.  When running with page size 4096, the loader cannot
know whether p_align was set to this value merely to satisfy the p_align
< PAGESIZE check, or because there is actually some section alignment
that requires 65536 byte alignment.  There is no kernel interface to
request 65536 byte alignment, so the loader has to do extra work to
satisfy this request.  And in the first case (no actual 65536 byte
alignment requirement), that work is unnecessary.

> In any case, do you know if this test file is somehow related to that
> work, or is it just a guess? It doesn't seem to be related to me since
> it's essentially a "pageless" mapping setup.

The glibc test seems to be just buggy: First we verify p_align against
the page size, then we use that p_align value to check the alignment of
the PT_LOAD segment (mainly file offset congruency).  The p_align check
against the page size looks completely optional to me if we check file
offset congruency directly against the run-time page size.

The ELF specification explicitly describes the p_align values 0 and 1 as
valid, indicating no alignment constraint.  So a p_align < PAGESIZE
check is buggy in that regard as well.  This also conflicts with your
interpretation as p_align as the maximum supported page size.

Thanks,
Florian


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [musl] Dynamic linker segfault
  2022-01-05 14:00       ` Florian Weimer
@ 2022-01-05 15:00         ` Rich Felker
  2022-01-07 13:58           ` Florian Weimer
  0 siblings, 1 reply; 9+ messages in thread
From: Rich Felker @ 2022-01-05 15:00 UTC (permalink / raw)
  To: Florian Weimer; +Cc: Nihal Jere, musl

On Wed, Jan 05, 2022 at 03:00:53PM +0100, Florian Weimer wrote:
> * Rich Felker:
> 
> > On Wed, Jan 05, 2022 at 09:56:25AM +0100, Florian Weimer wrote:
> >> * Rich Felker:
> >> 
> >> > This is a malformed program file not compatible with the machine page
> >> > size (4k). Arguably it should be detected as p_align < PAGESIZE -- in
> >> > a sense, p_align for LOAD segments is the maximum supported page size
> >> > for the program file, and machines not capable of providing a page
> >> > size that small can't map/run it. In theory the loader could allow
> >> > this if all the differences between segments satisfy the right
> >> > congruences and have matching permissions where the maps would
> >> > overlap, but I'm not sure that's useful.
> >> 
> >> We've been looking at this on the glibc side recently.  The use case is
> >> supporting large data alignments (greater than the kernel page size)
> >> while not pessimizing multi-page-size targets such as POWER and AArch64.
> >
> > I'm not clear how it pessimizes these targets (beyond what's
> > fundamentally necessary) unless you're artificially aligning segment
> > contents on disk to a large alignment boundary to prevent over-mapping
> > (undermining separate-code for example). And if you're doing that, you
> > need the full alignment anyway to support machines with larger
> > hardware pagesize. Otherwise you'd get back the overmapping (and
> > unwanted perission exposure).
> 
> Hmm.  Maybe I should rephrase:
> 
> With a p_align < PAGESIZE check in place, portable binaries need to use
> the value 65536.  When running with page size 4096, the loader cannot
> know whether p_align was set to this value merely to satisfy the p_align
> < PAGESIZE check, or because there is actually some section alignment
> that requires 65536 byte alignment.  There is no kernel interface to
> request 65536 byte alignment, so the loader has to do extra work to
> satisfy this request.  And in the first case (no actual 65536 byte
> alignment requirement), that work is unnecessary.

Unfortunately it's impossible to distinguish between such an alignment
requirement and __attribute__((__aligned__(65536))) appearing in
section contents that went into the segment, so disregarding it is a
bug (one musl also has) I think.

> > In any case, do you know if this test file is somehow related to that
> > work, or is it just a guess? It doesn't seem to be related to me since
> > it's essentially a "pageless" mapping setup.
> 
> The glibc test seems to be just buggy: First we verify p_align against
> the page size, then we use that p_align value to check the alignment of
> the PT_LOAD segment (mainly file offset congruency).  The p_align check
> against the page size looks completely optional to me if we check file
> offset congruency directly against the run-time page size.
> 
> The ELF specification explicitly describes the p_align values 0 and 1 as
> valid, indicating no alignment constraint.  So a p_align < PAGESIZE
> check is buggy in that regard as well.  This also conflicts with your
> interpretation as p_align as the maximum supported page size.

The ELF specification describes syntax not semantic requirements on
the platform to support anything the syntax can represent. The
semantics of the above can't be honored (without memcpy instead of
mmap, which is really something you don't want to support) if they
produce congruences incompatible with the layout, and they can't be
honored at all if they're incompatible with the permissions
requirements. Even when they don't conflict with the permissions
requirements, they may expose unintended mapped data (which for users
wanting separate-code is problematic, since it introduces gadgets). So
I'm not convinced it should be supported even when it "can".

Note also that segments (PT_LOAD) are not the only type of program
headers, and p_align of 0 or 1 may make more sense for other types.

Rich

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [musl] Dynamic linker segfault
  2022-01-05 15:00         ` Rich Felker
@ 2022-01-07 13:58           ` Florian Weimer
  2022-01-07 17:35             ` Rich Felker
  0 siblings, 1 reply; 9+ messages in thread
From: Florian Weimer @ 2022-01-07 13:58 UTC (permalink / raw)
  To: Rich Felker; +Cc: Nihal Jere, musl

* Rich Felker:

>> With a p_align < PAGESIZE check in place, portable binaries need to use
>> the value 65536.  When running with page size 4096, the loader cannot
>> know whether p_align was set to this value merely to satisfy the p_align
>> < PAGESIZE check, or because there is actually some section alignment
>> that requires 65536 byte alignment.  There is no kernel interface to
>> request 65536 byte alignment, so the loader has to do extra work to
>> satisfy this request.  And in the first case (no actual 65536 byte
>> alignment requirement), that work is unnecessary.
>
> Unfortunately it's impossible to distinguish between such an alignment
> requirement and __attribute__((__aligned__(65536))) appearing in
> section contents that went into the segment, so disregarding it is a
> bug (one musl also has) I think.

Agreed.

>> > In any case, do you know if this test file is somehow related to that
>> > work, or is it just a guess? It doesn't seem to be related to me since
>> > it's essentially a "pageless" mapping setup.
>> 
>> The glibc test seems to be just buggy: First we verify p_align against
>> the page size, then we use that p_align value to check the alignment of
>> the PT_LOAD segment (mainly file offset congruency).  The p_align check
>> against the page size looks completely optional to me if we check file
>> offset congruency directly against the run-time page size.
>> 
>> The ELF specification explicitly describes the p_align values 0 and 1 as
>> valid, indicating no alignment constraint.  So a p_align < PAGESIZE
>> check is buggy in that regard as well.  This also conflicts with your
>> interpretation as p_align as the maximum supported page size.
>
> The ELF specification describes syntax not semantic requirements on
> the platform to support anything the syntax can represent.

That's not entirely accurate, there are exceptions.  p_align seems to be
one such exception:

p_align
    As ``Program Loading'' describes in this chapter of the processor
    supplement, loadable process segments must have congruent values for
    p_vaddr and p_offset, modulo the page size. This member gives the
    value to which the segments are aligned in memory and in the
    file. Values 0 and 1 mean no alignment is required. Otherwise,
    p_align should be a positive, integral power of 2, and p_vaddr
    should equal p_offset, modulo p_align.

Most processor supplements do not discuss p_align unfortunately?

> The semantics of the above can't be honored (without memcpy instead of
> mmap, which is really something you don't want to support) if they
> produce congruences incompatible with the layout, and they can't be
> honored at all if they're incompatible with the permissions
> requirements. Even when they don't conflict with the permissions
> requirements, they may expose unintended mapped data (which for users
> wanting separate-code is problematic, since it introduces gadgets). So
> I'm not convinced it should be supported even when it "can".

The congruency compatibility is a property of the file layout, not the
p_align value.  If the file layout is incompatible, changing p_align
doesn't make the object loadable.

As far as I can tell, p_align is not at all useful, except for (e.g.)
__attribute__((__aligned__(65536))) for global.  But even that wasn't
implemented widely.

Thanks,
Florian


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [musl] Dynamic linker segfault
  2022-01-07 13:58           ` Florian Weimer
@ 2022-01-07 17:35             ` Rich Felker
  2022-01-10 13:30               ` Florian Weimer
  0 siblings, 1 reply; 9+ messages in thread
From: Rich Felker @ 2022-01-07 17:35 UTC (permalink / raw)
  To: Florian Weimer; +Cc: Nihal Jere, musl

On Fri, Jan 07, 2022 at 02:58:54PM +0100, Florian Weimer wrote:
> * Rich Felker:
> 
> >> With a p_align < PAGESIZE check in place, portable binaries need to use
> >> the value 65536.  When running with page size 4096, the loader cannot
> >> know whether p_align was set to this value merely to satisfy the p_align
> >> < PAGESIZE check, or because there is actually some section alignment
> >> that requires 65536 byte alignment.  There is no kernel interface to
> >> request 65536 byte alignment, so the loader has to do extra work to
> >> satisfy this request.  And in the first case (no actual 65536 byte
> >> alignment requirement), that work is unnecessary.
> >
> > Unfortunately it's impossible to distinguish between such an alignment
> > requirement and __attribute__((__aligned__(65536))) appearing in
> > section contents that went into the segment, so disregarding it is a
> > bug (one musl also has) I think.
> 
> Agreed.
> 
> >> > In any case, do you know if this test file is somehow related to that
> >> > work, or is it just a guess? It doesn't seem to be related to me since
> >> > it's essentially a "pageless" mapping setup.
> >> 
> >> The glibc test seems to be just buggy: First we verify p_align against
> >> the page size, then we use that p_align value to check the alignment of
> >> the PT_LOAD segment (mainly file offset congruency).  The p_align check
> >> against the page size looks completely optional to me if we check file
> >> offset congruency directly against the run-time page size.
> >> 
> >> The ELF specification explicitly describes the p_align values 0 and 1 as
> >> valid, indicating no alignment constraint.  So a p_align < PAGESIZE
> >> check is buggy in that regard as well.  This also conflicts with your
> >> interpretation as p_align as the maximum supported page size.
> >
> > The ELF specification describes syntax not semantic requirements on
> > the platform to support anything the syntax can represent.
> 
> That's not entirely accurate, there are exceptions.  p_align seems to be
> one such exception:
> 
> p_align
>     As ``Program Loading'' describes in this chapter of the processor
>     supplement, loadable process segments must have congruent values for
>     p_vaddr and p_offset, modulo the page size. This member gives the
>     value to which the segments are aligned in memory and in the
>     file. Values 0 and 1 mean no alignment is required. Otherwise,
>     p_align should be a positive, integral power of 2, and p_vaddr
>     should equal p_offset, modulo p_align.
> 
> Most processor supplements do not discuss p_align unfortunately?
> 
> > The semantics of the above can't be honored (without memcpy instead of
> > mmap, which is really something you don't want to support) if they
> > produce congruences incompatible with the layout, and they can't be
> > honored at all if they're incompatible with the permissions
> > requirements. Even when they don't conflict with the permissions
> > requirements, they may expose unintended mapped data (which for users
> > wanting separate-code is problematic, since it introduces gadgets). So
> > I'm not convinced it should be supported even when it "can".
> 
> The congruency compatibility is a property of the file layout, not the
> p_align value.  If the file layout is incompatible, changing p_align
> doesn't make the object loadable.
> 
> As far as I can tell, p_align is not at all useful, except for (e.g.)
> __attribute__((__aligned__(65536))) for global.  But even that wasn't
> implemented widely.

By my understanding, p_align implies an understanding by the creator
of the program that the load segment may be "over-mapped" (extra file
contents visible and possibly executable) up to that alignment
boundary due to page granularity. This isn't really a correctness
contract but a security/hardening contract for folks who consider
anti-ROP measures a boundary that should be enforced.

Rich

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [musl] Dynamic linker segfault
  2022-01-07 17:35             ` Rich Felker
@ 2022-01-10 13:30               ` Florian Weimer
  0 siblings, 0 replies; 9+ messages in thread
From: Florian Weimer @ 2022-01-10 13:30 UTC (permalink / raw)
  To: Rich Felker; +Cc: Nihal Jere, musl

* Rich Felker:

> By my understanding, p_align implies an understanding by the creator
> of the program that the load segment may be "over-mapped" (extra file
> contents visible and possibly executable) up to that alignment
> boundary due to page granularity. This isn't really a correctness
> contract but a security/hardening contract for folks who consider
> anti-ROP measures a boundary that should be enforced.

Not sure if I follow.  Do you mean p_align is purely for documenting
layout decisions by the link editor?  I suspect that the necessary
overlap would be visible by other means, too.

Thanks,
Florian


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2022-01-10 13:30 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-04 21:27 [musl] Dynamic linker segfault Nihal Jere
2022-01-04 21:42 ` Rich Felker
2022-01-05  8:56   ` Florian Weimer
2022-01-05 13:49     ` Rich Felker
2022-01-05 14:00       ` Florian Weimer
2022-01-05 15:00         ` Rich Felker
2022-01-07 13:58           ` Florian Weimer
2022-01-07 17:35             ` Rich Felker
2022-01-10 13:30               ` Florian Weimer

Code repositories for project(s) associated with this public inbox

	https://git.vuxu.org/mirror/musl/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).