Date: Fri, 7 Sep 2012 19:02:08 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: Svatopluk Kraus <onwahe@gmail.com> Cc: freebsd-current@freebsd.org Subject: Re: [patch] mmap() MAP_TEXT implementation (to use for shared libraries) Message-ID: <20120907160208.GZ33100@deviant.kiev.zoral.com.ua> In-Reply-To: <CAFHCsPXJc0w=6t5JwxD6kT=-pCuk6b9bruf=xA=rZXEfWOyrow@mail.gmail.com> References: <CAFHCsPX6HrCXHA%2BS31Dz9QP8eiwbo21Vzju4K4paohciu2vPTw@mail.gmail.com> <CAFHCsPWBkU23kk-vnMoahMUBkyfbJXoH=jj=DTqwV520mGC5Fw@mail.gmail.com> <20120904130039.GX33100@deviant.kiev.zoral.com.ua> <201209041200.27100.jhb@freebsd.org> <CAFHCsPXJc0w=6t5JwxD6kT=-pCuk6b9bruf=xA=rZXEfWOyrow@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
--XAHTXMpTldIJKQ/i
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
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@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@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 l=
inker
> >> > >> are protected against writing (ETXTBSY), the loaded shared librar=
ies
> >> > >> 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_use=
lib()
> >> > >> 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 mma=
p()
> >> > >> and in case that underlying object is a vnode, set VV_TEXT flag o=
n 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 v=
node
> >> > >> 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_TEX=
T is
> >> > >> set before activation and cleared if something failed. Shared lib=
rary
> >> > >> activation is done in dynamic linker (i.e., in userland). It's do=
ne 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 t=
o set
> >> > >> VV_TEXT flag on a vnode, I need an exclusive lock. In current cod=
e,
> >> > >> 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
> >> > >> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> >> > >> --- libexec/rtld-elf/map_object.c (revision 239770)
> >> > >> +++ libexec/rtld-elf/map_object.c (working copy)
> >> > >> @@ -199,7 +199,8 @@
> >> > >> data_prot =3D convert_prot(segs[i]->p_flags);
> >> > >> data_flags =3D 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) =3D=3D (=
caddr_t)
> > -1) {
> >> > >> + data_flags | MAP_PREFAULT_READ | MAP_TEXT, fd, data_offse=
t) =3D=3D
> >> > >> + (caddr_t) -1) {
> >> > > I am not sure that we shall mark all segments mappings with MAP_TE=
XT.
> >> > > I understand the logic of the change, since we do not want data se=
gment
> >> > > to be changed under us. Still, having MAP_TEXT for non-text segmen=
ts
> > 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
> >> > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> >> > --- 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 |=3D MAP_NOCORE;
> >> > + /*
> >> > + * Executable mappings are marked "MAP_TEXT".
> >> > + */
> >> > + if (elfflags & PF_X)
> >> > + flags |=3D MAP_TEXT;
> >> > return flags;
> >> > }
> >> >
> >> >
> >> > >> _rtld_error("%s: mmap of data failed: %s", path,
> >> > >> rtld_strerror(errno));
> >> > >> goto error1;
> >> > >> Index: sys/vm/vm_mmap.c
> >> > >> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> >> > >> --- 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 =3D vp->v_mount;
> >> > >> cred =3D td->td_ucred;
> >> > >> - if ((*maxprotp & VM_PROT_WRITE) && (*flagsp & MAP_SHARED))
> >> > >> + flags =3D *flagsp;
> >> > >> + writeable_shared =3D ((*maxprotp & VM_PROT_WRITE) && (flags=
&
> > MAP_SHARED));
> >> > >> + if (writeable_shared || ((flags & MAP_TEXT) !=3D 0))
> >> > >> locktype =3D LK_EXCLUSIVE;
> >> > >> else
> >> > >> locktype =3D LK_SHARED;
> >> > >> @@ -1271,7 +1274,6 @@
> >> > >> return (error);
> >> > >> }
> >> > >> foff =3D *foffp;
> >> > >> - flags =3D *flagsp;
> >> > >> obj =3D vp->v_object;
> >> > >> if (vp->v_type =3D=3D VREG) {
> >> > >> /*
> >> > >> @@ -1294,7 +1296,7 @@
> >> > >> return (error);
> >> > >> }
> >> > >> }
> >> > >> - if (locktype =3D=3D LK_EXCLUSIVE) {
> >> > >> + if (writeable_shared) {
> >> > >> *writecounted =3D TRUE;
> >> > >> vnode_pager_update_writecount(obj, 0, objsi=
ze);
> >> > >> }
> >> > >> @@ -1337,6 +1339,14 @@
> >> > >> error =3D ENOMEM;
> >> > >> goto done;
> >> > >> }
> >> > >> + /*
> >> > >> + * If MAP_TEXT is announced, set VV_TEXT so no one can write
> >> > >> + * to the executable.
> >> > >> + */
> >> > >> + if ((flags & MAP_TEXT) !=3D 0) {
> >> > >> + ASSERT_VOP_ELOCKED(vp, "vv_text");
> >> > >> + vp->v_vflag |=3D 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 basical=
ly
> >> 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 modificati=
ons
> >> to the file. Note that this is not a problem for executables, becau=
se
> >> 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 nee=
ds
> > 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 requi=
re
> > a file opened with FREAD. Hmm, perhaps rtld could open a separate fd f=
or
> > PROT_EXEC mappings that used O_EXEC and mmap()'ing an O_EXEC fd could e=
nable
> > 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.
>=20
> 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.
--XAHTXMpTldIJKQ/i
Content-Type: application/pgp-signature
Content-Disposition: inline
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (FreeBSD)
iEYEARECAAYFAlBKGn8ACgkQC3+MBN1Mb4gkSwCghehBoaSb/w0YPJv4r6hg4jC3
lJ4An16tu/g9+BK71WiUBuqGGPqZVc8q
=ZoO3
-----END PGP SIGNATURE-----
--XAHTXMpTldIJKQ/i--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120907160208.GZ33100>