Re: [patch] mmap() MAP_TEXT implementation (to use for shared libraries)

On Mon, Sep 3, 2012 at 2:46 PM, Konstantin Belousov <kostikbel_at_gmail.com> wrote:
> On Mon, Sep 03, 2012 at 12:35:08PM +0200, Svatopluk Kraus wrote:
>> Hi,
>>
>>   I found out that while the running excecutables and a dynamic linker
>> are protected against writing (ETXTBSY), the loaded shared libraries
>> are not protected. The libraries are mapped by mmap() in dynamic
>> linker (rtld) and there is no way how to set VV_TEXT flag on the
>> libraries vnodes in mmap() code.
>>
>>   In linux compability code \compat\linux\linux_misc.c, linux_uselib()
>> sets VV_TEXT flags on a library vnode. In Solaris, MAP_TEXT flag
>> exists which informs mmap() that the mapped region will be used
>> primarily for executing instructions (for better MMU utilization).
>> With these on mind, I propose to implement MAP_TEXT option in mmap()
>> and in case that underlying object is a vnode, set VV_TEXT flag on it.
>>
>>   I already have implemented it and with rtld map_object() patch it
>> works fine for me (of course). The rtld patch looks easy, however I'm
>> not sure about mmap patch.
>>
>>   After some investigation, it looks that VV_TEXT once set on a vnode
>> remains set until last reference on the vnode is left. So, I don't
>> bother with VV_TEXT unset in munmap() to be consistent. The
>> executables and dynamic linker are activated in kernel, so VV_TEXT is
>> set before activation and cleared if something failed. Shared library
>> activation is done in dynamic linker (i.e., in userland). It's done in
>> steps and mmaping the library is one from them. So, I think that
>> VV_TEXT can be set in mmap() just after everything is finished
>> successfully.
> This is right, the object reference counter is also used as
> VV_TEXT counter. It is somewhat unaccurate, but in practice does
> not cause issues.
>
>>
>>   The patch itself is implemented in vm_mmap_vnode(). If I want to set
>> VV_TEXT flag on a vnode, I need an exclusive lock. In current code,
>> the exclusive lock flag is (mis)used as a flag for
>> vnode_pager_update_writecount() call. (I hope that I didn't miss
>> something.) So, the patch is bigger slightly.
>>
>>   I defined the MAP_TEXT flag in extented flags sections. However, I'm
>> feeling the relation to MAP_STACK flag, but not sure if and when
>> reserved flags (in other flags section) can be re-used.
>>
>>        Svata
>>
>>
>>   Index: libexec/rtld-elf/map_object.c
>> ===================================================================
>> --- libexec/rtld-elf/map_object.c     (revision 239770)
>> +++ libexec/rtld-elf/map_object.c     (working copy)
>> _at__at_ -199,7 +199,8 _at__at_
>>       data_prot = convert_prot(segs[i]->p_flags);
>>       data_flags = convert_flags(segs[i]->p_flags) | MAP_FIXED;
>>       if (mmap(data_addr, data_vlimit - data_vaddr, data_prot,
>> -       data_flags | MAP_PREFAULT_READ, fd, data_offset) == (caddr_t) -1) {
>> +       data_flags | MAP_PREFAULT_READ | MAP_TEXT, fd, data_offset) ==
>> +         (caddr_t) -1) {
> I am not sure that we shall mark all segments mappings with MAP_TEXT.
> I understand the logic of the change, since we do not want data segment
> to be changed under us. Still, having MAP_TEXT for non-text segments looks
> strange.
>

I agree. However, only way how to recognize a text segment is an
executable flag set. The new patch for map_object.c is following:

Index: libexec/rtld-elf/map_object.c
===================================================================
--- libexec/rtld-elf/map_object.c	(revision 239770)
+++ libexec/rtld-elf/map_object.c	(working copy)
_at__at_ -442,5 +442,10 _at__at_
      */
     if (!(elfflags & PF_W))
 	flags |= MAP_NOCORE;
+    /*
+     * Executable mappings are marked "MAP_TEXT".
+     */
+    if (elfflags & PF_X)
+	flags |= MAP_TEXT;
     return flags;
 }


>>           _rtld_error("%s: mmap of data failed: %s", path,
>>               rtld_strerror(errno));
>>           goto error1;
>> Index: sys/vm/vm_mmap.c
>> ===================================================================
>> --- sys/vm/vm_mmap.c  (revision 239770)
>> +++ sys/vm/vm_mmap.c  (working copy)
>> _at__at_ -1258,10 +1258,13 _at__at_
>>       struct mount *mp;
>>       struct ucred *cred;
>>       int error, flags, locktype, vfslocked;
>> +     int writeable_shared;
>>
>>       mp = vp->v_mount;
>>       cred = td->td_ucred;
>> -     if ((*maxprotp & VM_PROT_WRITE) && (*flagsp & MAP_SHARED))
>> +     flags = *flagsp;
>> +     writeable_shared = ((*maxprotp & VM_PROT_WRITE) && (flags & MAP_SHARED));
>> +     if (writeable_shared || ((flags & MAP_TEXT) != 0))
>>               locktype = LK_EXCLUSIVE;
>>       else
>>               locktype = LK_SHARED;
>> _at__at_ -1271,7 +1274,6 _at__at_
>>               return (error);
>>       }
>>       foff = *foffp;
>> -     flags = *flagsp;
>>       obj = vp->v_object;
>>       if (vp->v_type == VREG) {
>>               /*
>> _at__at_ -1294,7 +1296,7 _at__at_
>>                               return (error);
>>                       }
>>               }
>> -             if (locktype == LK_EXCLUSIVE) {
>> +             if (writeable_shared) {
>>                       *writecounted = TRUE;
>>                       vnode_pager_update_writecount(obj, 0, objsize);
>>               }
>> _at__at_ -1337,6 +1339,14 _at__at_
>>               error = ENOMEM;
>>               goto done;
>>       }
>> +     /*
>> +      * If MAP_TEXT is announced, set VV_TEXT so no one can write
>> +      * to the executable.
>> +      */
>> +     if ((flags & MAP_TEXT) != 0) {
>> +             ASSERT_VOP_ELOCKED(vp, "vv_text");
>> +             vp->v_vflag |= VV_TEXT;
>> +     }
> I do not think we want to set VV_TEXT for device vnodes.
>

I agree too. However, my patch doesn't set VV_TEXT for device vnodes.
Device vnodes never enter into patched part of code.

Svata