From owner-freebsd-current@FreeBSD.ORG Tue Sep 4 12:49:09 2012 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 61A83106566C for ; Tue, 4 Sep 2012 12:49:09 +0000 (UTC) (envelope-from onwahe@gmail.com) Received: from mail-we0-f182.google.com (mail-we0-f182.google.com [74.125.82.182]) by mx1.freebsd.org (Postfix) with ESMTP id E39CA8FC15 for ; Tue, 4 Sep 2012 12:49:08 +0000 (UTC) Received: by weyx56 with SMTP id x56so4358013wey.13 for ; Tue, 04 Sep 2012 05:49:08 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=W6LYSQ8qXqGRApVPyVhNKZ9cCVY1R+QU46Ku1sPMxXY=; b=qPsur/A2WvcOtFzEJGKW9TIxUtmRtmVz2Q3OqaNpMgrBirxcK9FE1o4dg8YlDu40LC W1JnvlSH4lT6voy8+I6NHyCy4Wnay33Wv/301L2T15G6SYBDDLghTxSeeruPj/QbT/TV qvmD/AslXM2NCqpT4elpwyClWCUjYrRKCRQdImcHdwGOKIxNzpeoE5UQ98+tBoabv9cT OcKev3qJmlRdJoT+nJBfGrBTMrkPBHsLJYmgGPeYb7lKapXCb1FBboT0VwU8vEuedThD pJTTP1bRfJDOhrHovjj8p8Rd/OlVdwUwhSBLDIi4Lntlk7PV9fQjgFElzijyYGr9lYpR miUw== MIME-Version: 1.0 Received: by 10.216.241.202 with SMTP id g52mr12126133wer.212.1346762948049; Tue, 04 Sep 2012 05:49:08 -0700 (PDT) Received: by 10.194.0.145 with HTTP; Tue, 4 Sep 2012 05:49:07 -0700 (PDT) In-Reply-To: <20120903124642.GI33100@deviant.kiev.zoral.com.ua> References: <20120903124642.GI33100@deviant.kiev.zoral.com.ua> Date: Tue, 4 Sep 2012 14:49:07 +0200 Message-ID: From: Svatopluk Kraus To: Konstantin Belousov Content-Type: text/plain; charset=ISO-8859-1 Cc: freebsd-current@freebsd.org Subject: Re: [patch] mmap() MAP_TEXT implementation (to use for shared libraries) X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 04 Sep 2012 12:49:09 -0000 On Mon, Sep 3, 2012 at 2:46 PM, Konstantin Belousov 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