[PATCH] Test for __mprotect failure in _dl_map_segments [BZ #20831]

[PATCH] Test for __mprotect failure in _dl_map_segments [BZ #20831]

[Dmitry V. Levin]

* elf/dl-map-segments.h (_dl_map_segments): Test for failure
of __mprotect to change protection on the excess portion
to disallow all access.
---
 ChangeLog             |  7 +++++++
 elf/dl-map-segments.h | 21 +++++++++++++--------
 2 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/elf/dl-map-segments.h b/elf/dl-map-segments.h
index e583f64..3dc030b 100644
--- a/elf/dl-map-segments.h
+++ b/elf/dl-map-segments.h
@@ -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;
 
-- 
ldv

[PING] [PATCH] Test for __mprotect failure in _dl_map_segments [BZ #20831]

[Dmitry V. Levin]

* elf/dl-map-segments.h (_dl_map_segments): Test for failure
of __mprotect to change protection on the excess portion
to disallow all access.
---
 ChangeLog             |  7 +++++++
 elf/dl-map-segments.h | 21 +++++++++++++--------
 2 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/elf/dl-map-segments.h b/elf/dl-map-segments.h
index e583f64..3dc030b 100644
--- a/elf/dl-map-segments.h
+++ b/elf/dl-map-segments.h
@@ -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;
 
-- 
ldv

[PING v2] [PATCH] Test for __mprotect failure in _dl_map_segments [BZ #20831]

[Dmitry V. Levin]

* 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(-)

diff --git a/elf/dl-map-segments.h b/elf/dl-map-segments.h
index e583f64..3dc030b 100644
--- a/elf/dl-map-segments.h
+++ b/elf/dl-map-segments.h
@@ -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;
 
-- 
ldv

Re: [PING v2] [PATCH] Test for __mprotect failure in _dl_map_segments [BZ #20831]

[Zack Weinberg]

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;

?

Re: [PING v2] [PATCH] Test for __mprotect failure in _dl_map_segments [BZ #20831]

[Dmitry V. Levin]

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

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.


-- 
ldv

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PING v2] [PATCH] Test for __mprotect failure in _dl_map_segments [BZ #20831]

[Zack Weinberg]

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

[PATCH v2] Check for __mprotect failure in _dl_map_segments [BZ #20831]

[Dmitry V. Levin]

* elf/dl-map-segments.h (_dl_map_segments): Check for failure
of __mprotect to change protection on the excess portion
to disallow all access.
---
v2: change formatting of __glibc_unlikely as suggested by Zack Weinberg
---
 ChangeLog             |  7 +++++++
 elf/dl-map-segments.h | 20 ++++++++++++--------
 2 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/elf/dl-map-segments.h b/elf/dl-map-segments.h
index 31d6861..d36f9bd 100644
--- a/elf/dl-map-segments.h
+++ b/elf/dl-map-segments.h
@@ -64,14 +64,18 @@ _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.  */
+          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;
+        }
 
       l->l_contiguous = 1;
 
-- 
ldv

Re: [PATCH v2] Check for __mprotect failure in _dl_map_segments [BZ #20831]

[Dmitry V. Levin]

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

On Tue, Mar 21, 2017 at 05:20:28PM +0300, Dmitry V. Levin wrote:
> * elf/dl-map-segments.h (_dl_map_segments): Check for failure
> of __mprotect to change protection on the excess portion
> to disallow all access.
> ---
> v2: change formatting of __glibc_unlikely as suggested by Zack Weinberg
> ---
>  ChangeLog             |  7 +++++++
>  elf/dl-map-segments.h | 20 ++++++++++++--------
>  2 files changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/elf/dl-map-segments.h b/elf/dl-map-segments.h
> index 31d6861..d36f9bd 100644
> --- a/elf/dl-map-segments.h
> +++ b/elf/dl-map-segments.h
> @@ -64,14 +64,18 @@ _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.  */
> +          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;
> +        }
>  
>        l->l_contiguous = 1;
>  

There have been no more comments so I've pushed this fix.


-- 
ldv

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]