[PING,v2] Test for __mprotect failure in _dl_map_segments [BZ #20831]
Commit Message
* elf/dl-map-segments.h (_dl_map_segments): Test for failure
of __mprotect to change protection on the excess portion
to disallow all access.
---
I understand the patch is trivial, but anyway, there is a bug and it has
to be fixed.
If there are no comments, I'd push it rather than go on with these ping
reposts.
---
ChangeLog | 7 +++++++
elf/dl-map-segments.h | 21 +++++++++++++--------
2 files changed, 20 insertions(+), 8 deletions(-)
Comments
On Mon, Mar 20, 2017 at 3:35 PM, Dmitry V. Levin <ldv@altlinux.org> wrote:
>
> I understand the patch is trivial, but anyway, there is a bug and it has
> to be fixed.
> If there are no comments, I'd push it rather than go on with these ping
> reposts.
We have a collective problem where nobody feels empowered to say "yes"
to patches.
Also, you found this bug by fault injection -- do you have reason to
believe that this mprotect call can actually fail? If so, under what
circumstances, and how bad are the consequences?
> + {
> + /* Change protection on the excess portion to disallow all access;
> + the portions we do not remap later will be inaccessible as if
> + unallocated. Then jump into the normal segment-mapping loop to
> + handle the portion of the segment past the end of the file
> + mapping. */
> + int rc;
> + rc = __mprotect ((caddr_t) (l->l_addr + c->mapend),
> + loadcmds[nloadcmds - 1].mapstart - c->mapend,
> + PROT_NONE);
> + if (__glibc_unlikely (rc < 0))
> + return DL_MAP_SEGMENTS_ERROR_MPROTECT;
> + }
The variable 'rc' appears to be unnecessary. Why not just
if (__glibc_unlikely (__mprotect (...) < 0))
return DL_MAP_SEGMENTS_ERROR_MPROTECT;
?
On Mon, Mar 20, 2017 at 03:58:22PM -0400, Zack Weinberg wrote:
> On Mon, Mar 20, 2017 at 3:35 PM, Dmitry V. Levin <ldv@altlinux.org> wrote:
> >
> > I understand the patch is trivial, but anyway, there is a bug and it has
> > to be fixed.
> > If there are no comments, I'd push it rather than go on with these ping
> > reposts.
>
> We have a collective problem where nobody feels empowered to say "yes"
> to patches.
Yes, we have this problem, unfortunately.
> Also, you found this bug by fault injection -- do you have reason to
> believe that this mprotect call can actually fail? If so, under what
> circumstances, and how bad are the consequences?
Every mprotect call that increases memory fragmentation can legitimately
fail with ENOMEM, the fault injection technique is just a very easy way
to reproduce the error.
> > + {
> > + /* Change protection on the excess portion to disallow all access;
> > + the portions we do not remap later will be inaccessible as if
> > + unallocated. Then jump into the normal segment-mapping loop to
> > + handle the portion of the segment past the end of the file
> > + mapping. */
> > + int rc;
> > + rc = __mprotect ((caddr_t) (l->l_addr + c->mapend),
> > + loadcmds[nloadcmds - 1].mapstart - c->mapend,
> > + PROT_NONE);
> > + if (__glibc_unlikely (rc < 0))
> > + return DL_MAP_SEGMENTS_ERROR_MPROTECT;
> > + }
>
> The variable 'rc' appears to be unnecessary. Why not just
>
> if (__glibc_unlikely (__mprotect (...) < 0))
> return DL_MAP_SEGMENTS_ERROR_MPROTECT;
>
> ?
I want to keep the code readable. If I did this, the line would get
too long and I'd have to cut
loadcmds[nloadcmds - 1].mapstart - c->mapend
into pieces making it harder to comprehend.
On Mon, Mar 20, 2017 at 4:27 PM, Dmitry V. Levin <ldv@altlinux.org> wrote:
> On Mon, Mar 20, 2017 at 03:58:22PM -0400, Zack Weinberg wrote:
>> Also, you found this bug by fault injection -- do you have reason to
>> believe that this mprotect call can actually fail? If so, under what
>> circumstances, and how bad are the consequences?
>
> Every mprotect call that increases memory fragmentation can legitimately
> fail with ENOMEM, the fault injection technique is just a very easy way
> to reproduce the error.
OK, I'll accept that as sufficient reason to go forward with the patch.
>> The variable 'rc' appears to be unnecessary. Why not just
>>
>> if (__glibc_unlikely (__mprotect (...) < 0))
>> return DL_MAP_SEGMENTS_ERROR_MPROTECT;
>>
>> ?
>
> I want to keep the code readable. If I did this, the line would get
> too long and I'd have to cut
> loadcmds[nloadcmds - 1].mapstart - c->mapend
> into pieces making it harder to comprehend.
You could do it like this:
{
/* Change protection on the excess portion to disallow all access;
the portions we do not remap later will be inaccessible as if
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
(__mprotect ((caddr_t) (l->l_addr + c->mapend),
loadcmds[nloadcmds - 1].mapstart - c->mapend,
PROT_NONE) < 0))
return DL_MAP_SEGMENTS_ERROR_MPROTECT;
}
with the arguments to mprotect not any further rightward.
zw
@@ -64,14 +64,19 @@ _dl_map_segments (struct link_map *l, int fd,
l->l_addr = l->l_map_start - c->mapstart;
if (has_holes)
- /* Change protection on the excess portion to disallow all access;
- the portions we do not remap later will be inaccessible as if
- unallocated. Then jump into the normal segment-mapping loop to
- handle the portion of the segment past the end of the file
- mapping. */
- __mprotect ((caddr_t) (l->l_addr + c->mapend),
- loadcmds[nloadcmds - 1].mapstart - c->mapend,
- PROT_NONE);
+ {
+ /* Change protection on the excess portion to disallow all access;
+ the portions we do not remap later will be inaccessible as if
+ unallocated. Then jump into the normal segment-mapping loop to
+ handle the portion of the segment past the end of the file
+ mapping. */
+ int rc;
+ rc = __mprotect ((caddr_t) (l->l_addr + c->mapend),
+ loadcmds[nloadcmds - 1].mapstart - c->mapend,
+ PROT_NONE);
+ if (__glibc_unlikely (rc < 0))
+ return DL_MAP_SEGMENTS_ERROR_MPROTECT;
+ }
l->l_contiguous = 1;