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

On Friday, September 07, 2012 12:42:18 pm Konstantin Belousov wrote:
> On Fri, Sep 07, 2012 at 12:21:37PM -0400, John Baldwin wrote:
> > On Friday, September 07, 2012 12:02:08 pm Konstantin Belousov wrote:
> > > 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:
> > > > >> 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?
> > 
> > Shared libraries have +x by default already.  You could make rtld fall back
> > to using the O_RDONLY fd if it can't open an O_EXEC fd in which case you don't
> > get the VV_TEXT protection, but rtld would be backwards compatible.
> Where is it ? On fresh stable/9 machine, I have in /lib:
> -r--r--r--  1 root  wheel     7216 Dec  3  2011 libipx.so.5
> -r--r--r--  1 root  wheel    19800 Jun 28 18:33 libjail.so.1
> -r--r--r--  1 root  wheel    13752 Jun 28 18:33 libkiconv.so.4
> ...

Hmm, I was looking in /usr/local/lib.  It seems at least some ports do
install libraries with +x:

-rwxr-xr-x  1 root  wheel    210494 Apr  8  2011 /usr/local/lib/libxml++-2.6.so.2
-rwxr-xr-x  1 root  wheel   1515416 Apr  8  2011 /usr/local/lib/libxml2.so.5
-rwxr-xr-x  1 root  wheel     44389 Apr  8  2011 /usr/local/lib/libxmlparse.so.1
-rwxr-xr-x  1 root  wheel    100672 Apr  8  2011 /usr/local/lib/libxmltok.so.1
-rwxr-xr-x  1 root  wheel    266775 Apr  8  2011 /usr/local/lib/libxslt.so.2
-rw-r--r--  1 root  wheel    764602 Apr  8  2011 /usr/local/lib/libxvidcore.so.4
-rwxr-xr-x  1 root  wheel     53379 Apr  9  2011 /usr/local/lib/libzip.so.1

> > > 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.
> > 
> > I'm not sure it would be that obscure.  It's the same as getting an error that
> > a binary is busy.  In that case you would resort to 'ps' to find the offending
> > process.  For this case (a shared library being busy), you could look at the
> > output of 'procstat -af' to find which processes have executable mappings of
> > the library.
> 
> I much more worry about rtld reporting 'shared library is busy'. It is fine
> to not be able to write to mapped library.
> 
> Rtld errors are usually quite worrying for users, and they just do not
> understand them.

I think these would be rare?  There's no good reason for anything to write to
a shared library that I can think of.  install(1) does an atomic rename to swap
in the new libraries already.

-- 
John Baldwin