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

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?

Svata

>
> --
> John Baldwin