Why does _dl_protect_relro align the end address down?

Why does _dl_protect_relro align the end address down?

[Joel Molin]

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

Hi all,

I'm wondering if there's a reason why _dl_protect_relro aligns the end
address down, and whether it wouldn't be a pure improvement to align it up?

I've copied the relevant part of it below for reference (at
569cfcc6bf35c28112ca8d7112e9eb4a22bed5b8), but the effect of it is:
 - when start and end are on the same page, nothing is protected.
 - if end is not on a page boundary, parts of the region intended for
protection is left unprotected.

Notably this behavior is entirely silent, making it easy to believe that
you have hardened your executable when in fact you have not. This seems
very brittle.

I have no real world issue. I was writing a linker for fun, and was putting
my relro segment at the start of a page, it seemed the straightforward
thing to do. GNU ld 2.39.0 instead puts the relro segment at the end of a
page (I'm not familiar enough to know whether this is a cause, effect, or
unrelated).

I did however do some looking around and found
https://bugs.launchpad.net/ubuntu/+source/binutils/+bug/1412553, so it
seems like this has caused problems for projects with users.

Since mprotect will protect entire pages anyway, wouldn't it make a lot
more sense here to say `end = ALIGN_UP(...)`? Apart from feeling more
intuitive, it also seems like it would avoid page size inconsistencies
between runtime and link editing like the one above.

I couldn't really find any documentation about this that spelled out the
"align at end" requirement, but I'm happy to be proven wrong.

Happy to provide more examples if desired.

Thanks,
Joel

```
void
_dl_protect_relro (struct link_map *l)
{
  ElfW(Addr) start = ALIGN_DOWN((l->l_addr
+ l->l_relro_addr),
GLRO(dl_pagesize));
  ElfW(Addr) end = ALIGN_DOWN((l->l_addr
      + l->l_relro_addr
      + l->l_relro_size),
     GLRO(dl_pagesize));
  if (start != end
      && __mprotect ((void *) start, end - start, PROT_READ) < 0)
    {
...
```

Re: Why does _dl_protect_relro align the end address down?

[Florian Weimer]

* Joel Molin via Libc-help:

> Since mprotect will protect entire pages anyway, wouldn't it make a lot
> more sense here to say `end = ALIGN_UP(...)`? Apart from feeling more
> intuitive, it also seems like it would avoid page size inconsistencies
> between runtime and link editing like the one above.

ALIGN_UP might make memory read-only that should not be.

There's some discussion about the alignment here:

  PT_GNU_RELRO is somewhat broken
  <https://sourceware.org/pipermail/libc-alpha/2022-May/138638.html>

I still think this is fairly broken, but there does not seem to be much
desire to fix it.

Thanks,
Florian

Re: Why does _dl_protect_relro align the end address down?

[Adhemerval Zanella Netto]

On 17/01/23 04:57, Florian Weimer via Libc-help wrote:
> * Joel Molin via Libc-help:
> 
>> Since mprotect will protect entire pages anyway, wouldn't it make a lot
>> more sense here to say `end = ALIGN_UP(...)`? Apart from feeling more
>> intuitive, it also seems like it would avoid page size inconsistencies
>> between runtime and link editing like the one above.
> 
> ALIGN_UP might make memory read-only that should not be.
> 
> There's some discussion about the alignment here:
> 
>   PT_GNU_RELRO is somewhat broken
>   <https://sourceware.org/pipermail/libc-alpha/2022-May/138638.html>
> 
> I still think this is fairly broken, but there does not seem to be much
> desire to fix it.

It was not clear to me from that discussion what you think it is still
broken with PT_GNU_RELRO.

Re: Why does _dl_protect_relro align the end address down?

[Florian Weimer]

* Adhemerval Zanella Netto:

> On 17/01/23 04:57, Florian Weimer via Libc-help wrote:
>> * Joel Molin via Libc-help:
>> 
>>> Since mprotect will protect entire pages anyway, wouldn't it make a lot
>>> more sense here to say `end = ALIGN_UP(...)`? Apart from feeling more
>>> intuitive, it also seems like it would avoid page size inconsistencies
>>> between runtime and link editing like the one above.
>> 
>> ALIGN_UP might make memory read-only that should not be.
>> 
>> There's some discussion about the alignment here:
>> 
>>   PT_GNU_RELRO is somewhat broken
>>   <https://sourceware.org/pipermail/libc-alpha/2022-May/138638.html>
>> 
>> I still think this is fairly broken, but there does not seem to be much
>> desire to fix it.
>
> It was not clear to me from that discussion what you think it is still
> broken with PT_GNU_RELRO.

Using ALIGN_DOWN for the start address can't be right.

Thanks,
Florian

Re: Why does _dl_protect_relro align the end address down?

[Adhemerval Zanella Netto]

On 17/01/23 11:16, Florian Weimer wrote:
> * Adhemerval Zanella Netto:
> 
>> On 17/01/23 04:57, Florian Weimer via Libc-help wrote:
>>> * Joel Molin via Libc-help:
>>>
>>>> Since mprotect will protect entire pages anyway, wouldn't it make a lot
>>>> more sense here to say `end = ALIGN_UP(...)`? Apart from feeling more
>>>> intuitive, it also seems like it would avoid page size inconsistencies
>>>> between runtime and link editing like the one above.
>>>
>>> ALIGN_UP might make memory read-only that should not be.
>>>
>>> There's some discussion about the alignment here:
>>>
>>>   PT_GNU_RELRO is somewhat broken
>>>   <https://sourceware.org/pipermail/libc-alpha/2022-May/138638.html>
>>>
>>> I still think this is fairly broken, but there does not seem to be much
>>> desire to fix it.
>>
>> It was not clear to me from that discussion what you think it is still
>> broken with PT_GNU_RELRO.
> 
> Using ALIGN_DOWN for the start address can't be right.
> 

Do we have a bug report for this? Because H.J explanation [1] only describes
ld support for the end of the RO segment.

[1] https://sourceware.org/pipermail/libc-alpha/2022-May/138642.html

Re: Why does _dl_protect_relro align the end address down?

[H.J. Lu]

On Tue, Jan 17, 2023 at 6:30 AM Adhemerval Zanella Netto
<adhemerval.zanella@linaro.org> wrote:
>
>
>
> On 17/01/23 11:16, Florian Weimer wrote:
> > * Adhemerval Zanella Netto:
> >
> >> On 17/01/23 04:57, Florian Weimer via Libc-help wrote:
> >>> * Joel Molin via Libc-help:
> >>>
> >>>> Since mprotect will protect entire pages anyway, wouldn't it make a lot
> >>>> more sense here to say `end = ALIGN_UP(...)`? Apart from feeling more
> >>>> intuitive, it also seems like it would avoid page size inconsistencies
> >>>> between runtime and link editing like the one above.
> >>>
> >>> ALIGN_UP might make memory read-only that should not be.
> >>>
> >>> There's some discussion about the alignment here:
> >>>
> >>>   PT_GNU_RELRO is somewhat broken
> >>>   <https://sourceware.org/pipermail/libc-alpha/2022-May/138638.html>
> >>>
> >>> I still think this is fairly broken, but there does not seem to be much
> >>> desire to fix it.
> >>
> >> It was not clear to me from that discussion what you think it is still
> >> broken with PT_GNU_RELRO.
> >
> > Using ALIGN_DOWN for the start address can't be right.
> >
>
> Do we have a bug report for this? Because H.J explanation [1] only describes
> ld support for the end of the RO segment.
>
> [1] https://sourceware.org/pipermail/libc-alpha/2022-May/138642.html

If the end of the PT_GNU_RELRO segment isn't page aligned,
ALIGN_UP may change the executable page to read-only.

-- 
H.J.

Re: Why does _dl_protect_relro align the end address down?

[Joel Molin]

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

Thanks, I think I understand the challenge. Maybe I should have realized
when I saw the external bug I linked.

For as much as my opinion is worth, I guess I would also agree that
ALIGN_UP on the start would at least be consistent. Coupled with an
explanation such as "// Protect pages that are entirely within the relro
region. Since the runtime page size can vary, we can not trust linkers to
get this right and must be conservative." I would not have sent my email
(though I would have continued being sad). Right now it looks like a worst
of both worlds scenario.

Joel

On Tue, Jan 17, 2023 at 7:49 AM H.J. Lu <hjl.tools@gmail.com> wrote:

> On Tue, Jan 17, 2023 at 6:30 AM Adhemerval Zanella Netto
> <adhemerval.zanella@linaro.org> wrote:
> >
> >
> >
> > On 17/01/23 11:16, Florian Weimer wrote:
> > > * Adhemerval Zanella Netto:
> > >
> > >> On 17/01/23 04:57, Florian Weimer via Libc-help wrote:
> > >>> * Joel Molin via Libc-help:
> > >>>
> > >>>> Since mprotect will protect entire pages anyway, wouldn't it make a
> lot
> > >>>> more sense here to say `end = ALIGN_UP(...)`? Apart from feeling
> more
> > >>>> intuitive, it also seems like it would avoid page size
> inconsistencies
> > >>>> between runtime and link editing like the one above.
> > >>>
> > >>> ALIGN_UP might make memory read-only that should not be.
> > >>>
> > >>> There's some discussion about the alignment here:
> > >>>
> > >>>   PT_GNU_RELRO is somewhat broken
> > >>>   <https://sourceware.org/pipermail/libc-alpha/2022-May/138638.html>
> > >>>
> > >>> I still think this is fairly broken, but there does not seem to be
> much
> > >>> desire to fix it.
> > >>
> > >> It was not clear to me from that discussion what you think it is still
> > >> broken with PT_GNU_RELRO.
> > >
> > > Using ALIGN_DOWN for the start address can't be right.
> > >
> >
> > Do we have a bug report for this? Because H.J explanation [1] only
> describes
> > ld support for the end of the RO segment.
> >
> > [1] https://sourceware.org/pipermail/libc-alpha/2022-May/138642.html
>
> If the end of the PT_GNU_RELRO segment isn't page aligned,
> ALIGN_UP may change the executable page to read-only.
>
> --
> H.J.
>