From owner-freebsd-current@FreeBSD.ORG Fri Sep 7 15:11:04 2012 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 802DA1065694 for ; Fri, 7 Sep 2012 15:11:04 +0000 (UTC) (envelope-from onwahe@gmail.com) Received: from mail-wg0-f50.google.com (mail-wg0-f50.google.com [74.125.82.50]) by mx1.freebsd.org (Postfix) with ESMTP id 0D7888FC21 for ; Fri, 7 Sep 2012 15:11:03 +0000 (UTC) Received: by wgbds11 with SMTP id ds11so2445113wgb.31 for ; Fri, 07 Sep 2012 08:11:02 -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=LiFf5B8LV0Xk6hGyd2Azgstk/Pcfz0lfrpsljs5s7/A=; b=0bYIeHYgE1U5Whicnobssw4/h7wfuLdYxA/TsxtlweRN99O/yiMvn8x4fXSgCX8oLB CySw7JjPrvNMIXislwNowmMKujvQUVGXm9P5Sd83Ghm7985O9Bxl3JobFKXkk/WETnh7 Dl0ir8UgSj7wOpYJgjR1Be1pQt6s5wGCR4vnZ3hQc7QJOaDep2dVxmtd/byMuB13gw8u Nr1vMIUQigkk7YSMxMNQZqRh83k5BsTGImrdXWnwXb7jiAmmBqQbL+90n6U8P7SmKY7/ sVN4mciLiR57C0SZ67OQA9ssiz6+QTxl7JFcgw9HwSHWMwrt9V+RneFgfxByKH+4PUSo vcdQ== MIME-Version: 1.0 Received: by 10.180.82.164 with SMTP id j4mr12888189wiy.18.1347030661996; Fri, 07 Sep 2012 08:11:01 -0700 (PDT) Received: by 10.194.0.145 with HTTP; Fri, 7 Sep 2012 08:11:01 -0700 (PDT) In-Reply-To: <20120904130039.GX33100@deviant.kiev.zoral.com.ua> References: <20120903124642.GI33100@deviant.kiev.zoral.com.ua> <20120904130039.GX33100@deviant.kiev.zoral.com.ua> Date: Fri, 7 Sep 2012 17:11:01 +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: Fri, 07 Sep 2012 15:11:04 -0000 On Tue, Sep 4, 2012 at 3:00 PM, Konstantin Belousov wrote: > On Tue, Sep 04, 2012 at 02:49:07PM +0200, Svatopluk Kraus wrote: >> 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. > Hm, yes. > > Anyway, after thinking about the patch more, I see two issues: > > 1. You are setting VV_TEXT without checking v_writecount. This basically > nullifies the main reason for the patch, since existing writer can still > write or truncate the shared library after the mapping. You are right. I was thinking about it before. Now, when you pointed me to v_writecount, I found it in exec_check_permissions(). Thanks. So, executables activated in kernel are well protected. But the question is: for what, when shared libraries are avoided? > 2. I do not see what would prevent malicious local user from mmaping > arbitrary file readonly with MAP_TEXT, thus blocking any modifications > to the file. Note that this is not a problem for executables, because > kernel only sets VV_TEXT on executables if +x permission is set and > file is valid binary which kernel is able to execute. > > E.g. you might block log writes with VV_TEXT, or other user editing > session or whatever, having just read access to corresponding files. > > Am I wrong ? You are right again. I was too focused to my embeded single user boxes. Well, in general, setting VV_TEXT according to MAP_TEXT in mmap() is not secure.