1470701 - Possible crash in elfhack bootstrap code when run-time page size doesn't match alignment of executable PT_LOADs

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Comment 1

[Mike Hommey [:glandium]]

Attachment: Bug 1470701 - Use run-time page size when changing mapping permissions in elfhack injected code.

When a binary has a PT_GNU_RELRO segment, the elfhack injected code
uses mprotect to add the writable flag to relocated pages before
applying relocations, removing it afterwards. To do so, the elfhack
program uses the location and size of the PT_GNU_RELRO segment, and
adjusts it to be aligned according to the PT_LOAD alignment.

The problem here is that the PT_LOAD alignment doesn't necessarily match
the actual page alignment, and the resulting mprotect may end up not
covering the full extent of what the dynamic linker has protected
read-only according to the PT_GNU_RELRO segment. In turn, this can lead
to a crash on startup when trying to apply relocations to the still
read-only locations.

Practically speaking, this doesn't end up being a problem on x86, where
the PT_LOAD alignment is usually 4096, which happens to be the page
size, but on Debian armhf, it is 64k, while the run time page size can be
4k.

Review commit: https://reviewboard.mozilla.org/r/252546/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/252546/

Comment 2

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Comment on attachment 8987304 [details]
Bug 1470701 - Use run-time page size when changing mapping permissions in elfhack injected code.

https://reviewboard.mozilla.org/r/252546/#review259314

::: build/unix/elfhack/inject.c:67
(Diff revision 1)
>  static inline __attribute__((always_inline))
> -void relro_pre()
> +void do_relocations_with_relro(void)
>  {
> +    long page_size = sysconf_cb(_SC_PAGESIZE);
> +    uintptr_t aligned_relro_start = ((uintptr_t) relro_start) & ~(page_size - 1);
> +    uintptr_t aligned_relro_end = ((uintptr_t) relro_end) & ~(page_size - 1);

Am I missing something, or should this be rounding up and not down?

Comment 3

[Mike Hommey [:glandium]]

(In reply to Jed Davis [:jld] (⏰UTC-6) from comment #2)
> Comment on attachment 8987304 [details]
> Bug 1470701 - Use run-time page size when changing mapping permissions in
> elfhack injected code.
> 
> https://reviewboard.mozilla.org/r/252546/#review259314
> 
> ::: build/unix/elfhack/inject.c:67
> (Diff revision 1)
> >  static inline __attribute__((always_inline))
> > -void relro_pre()
> > +void do_relocations_with_relro(void)
> >  {
> > +    long page_size = sysconf_cb(_SC_PAGESIZE);
> > +    uintptr_t aligned_relro_start = ((uintptr_t) relro_start) & ~(page_size - 1);
> > +    uintptr_t aligned_relro_end = ((uintptr_t) relro_end) & ~(page_size - 1);
> 
> Am I missing something, or should this be rounding up and not down?

No, the rounding is down because if the end of the segment is not page aligned, that last page can't be made read-only, since it will contain bits that are read-write.

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 8987304 [details]
Bug 1470701 - Use run-time page size when changing mapping permissions in elfhack injected code.

https://reviewboard.mozilla.org/r/252546/#review259726

::: build/unix/elfhack/inject.c:65
(Diff revision 1)
> +    long page_size = sysconf_cb(_SC_PAGESIZE);
> +    uintptr_t aligned_relro_start = ((uintptr_t) relro_start) & ~(page_size - 1);
> +    uintptr_t aligned_relro_end = ((uintptr_t) relro_end) & ~(page_size - 1);

jld's concern about `aligned_relro_end` probably merits a comment about what is going on here.

Comment 5

[Mike Hommey [:glandium]]

Comment on attachment 8987304 [details]
Bug 1470701 - Use run-time page size when changing mapping permissions in elfhack injected code.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252546/diff/1-2/

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ded856841553
Use run-time page size when changing mapping permissions in elfhack injected code. r=froydnj

Comment 7

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/ded856841553