Date: Tue, 4 Sep 2012 14:49:07 +0200 From: Svatopluk Kraus <onwahe@gmail.com> To: Konstantin Belousov <kostikbel@gmail.com> Cc: freebsd-current@freebsd.org Subject: Re: [patch] mmap() MAP_TEXT implementation (to use for shared libraries) Message-ID: <CAFHCsPWBkU23kk-vnMoahMUBkyfbJXoH=jj=DTqwV520mGC5Fw@mail.gmail.com> In-Reply-To: <20120903124642.GI33100@deviant.kiev.zoral.com.ua> References: <CAFHCsPX6HrCXHA%2BS31Dz9QP8eiwbo21Vzju4K4paohciu2vPTw@mail.gmail.com> <20120903124642.GI33100@deviant.kiev.zoral.com.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, Sep 3, 2012 at 2:46 PM, Konstantin Belousov <kostikbel@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)
>> @@ -199,7 +199,8 @@
>> 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)
@@ -442,5 +442,10 @@
*/
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)
>> @@ -1258,10 +1258,13 @@
>> 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;
>> @@ -1271,7 +1274,6 @@
>> return (error);
>> }
>> foff = *foffp;
>> - flags = *flagsp;
>> obj = vp->v_object;
>> if (vp->v_type == VREG) {
>> /*
>> @@ -1294,7 +1296,7 @@
>> return (error);
>> }
>> }
>> - if (locktype == LK_EXCLUSIVE) {
>> + if (writeable_shared) {
>> *writecounted = TRUE;
>> vnode_pager_update_writecount(obj, 0, objsize);
>> }
>> @@ -1337,6 +1339,14 @@
>> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAFHCsPWBkU23kk-vnMoahMUBkyfbJXoH=jj=DTqwV520mGC5Fw>