On Fri, Sep 07, 2012 at 05:12:37PM +0200, Svatopluk Kraus wrote: > On Tue, Sep 4, 2012 at 6:00 PM, John Baldwin <jhb_at_freebsd.org> wrote: > > On Tuesday, September 04, 2012 9:00:39 am 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 <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. > >> 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. > >> > >> 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 ? > > > > Hmm, I do think 2) is a bit of a show-stopper. I do wonder why one needs > > MAP_TEXT at all or if you could key this off of mmap() with PROT_EXEC? > > Do we require +x permissions for PROT_EXEC? No, it seems we only require > > a file opened with FREAD. Hmm, perhaps rtld could open a separate fd for > > PROT_EXEC mappings that used O_EXEC and mmap()'ing an O_EXEC fd could enable > > VV_TEXT? That would require a file to have +x permisson for an mmap() to > > enable VV_TEXT. It would also make MAP_TEXT unneeded. > > It sounds good for me. I will try to patch it this way. However, do > you think that will be acceptable to set +x permission to shared > libraries in general? Setting +x on shared libraries can be done. But setting VV_TEXT for such mappings is definitely non-standard behaviour, that could cause locking surprises for unaware system administrator. The issuw would be very hard to diagnose.
This archive was generated by hypermail 2.4.0 : Wed May 19 2021 - 11:40:30 UTC