Date: Fri, 20 Nov 2015 09:02:41 +1100
From: Dave Chinner <david@fromorbit.com>
To: Kees Cook <keescook@chromium.org>
Cc: Jan Kara <jack@suse.cz>, "Theodore Ts'o" <tytso@mit.edu>,
        Andy Lutomirski <luto@amacapital.net>, Theodore Tso <tytso@google.com>,
        Willy Tarreau <w@1wt.eu>, Dirk Steinmetz <public@rsjtdrjgfuzkfg.com>,
        Michael Kerrisk-manpages <mtk.manpages@gmail.com>,
        Serge Hallyn <serge.hallyn@ubuntu.com>,
        Seth Forshee <seth.forshee@canonical.com>,
        Alexander Viro <viro@zeniv.linux.org.uk>,
        Linux FS Devel <linux-fsdevel@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        "Eric W . Biederman" <ebiederm@xmission.com>,
        Serge Hallyn <serge.hallyn@canonical.com>,
        "security@kernel.org" <security@kernel.org>
Subject: Re: [RFC] namei: prevent sgid-hardlinks for unmapped gids
Message-ID: <20151119220241.GD19199@dastard>
References: <CALCETrVKuAW=qYKsB_Dkmq2LDwSV1sdP8sKbuDcGzye3zn_e2A@mail.gmail.com>
 <CAGXu5jJ3yKaeWJb1b6aJTNJMrX1f+vTabMn3ATuz_cuksuXDog@mail.gmail.com>
 <CALCETrU3WyynfLmqf7Krb+veF=2AxXbd2o1p3R3caxqjXFYO=Q@mail.gmail.com>
 <CAGXu5jJUd5sZpRtx3EDgazp9G013FCJ=mwBbAOH=9Qb1TQXs_Q@mail.gmail.com>
 <CAGXu5jJH8VCbHcwLNaoDw5uxGcUzpckT6GJyp1u_wANfURwLhw@mail.gmail.com>
 <CALCETrVaB8saXsUrp-qfCAV=ocDfhAuM4PQ8pHjEStS4vDNF6w@mail.gmail.com>
 <CAGXu5j++4L3BLNPmBDwU9JA3UdqmFAWEKxDcveLOmJmxwgNiVA@mail.gmail.com>
 <20151108020206.GA3880@thunk.org>
 <20151110150829.GC3156@quack.suse.cz>
 <CAGXu5jJ443eKJDHUM5vAXkZkztdOVVosD4egniTk2pUJJ1_0Jw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAGXu5jJ443eKJDHUM5vAXkZkztdOVVosD4egniTk2pUJJ1_0Jw@mail.gmail.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2166
Lines: 49

On Thu, Nov 19, 2015 at 12:11:11PM -0800, Kees Cook wrote:
> On Tue, Nov 10, 2015 at 7:08 AM, Jan Kara <jack@suse.cz> wrote:
> > On Sat 07-11-15 21:02:06, Ted Tso wrote:
> >> On Fri, Nov 06, 2015 at 09:05:57PM -0800, Kees Cook wrote:
> >> > >>>> They're certainly not used early enough -- we need to remove suid when
> >> > >>>> the page becomes writable via mmap (wp_page_shared), not when
> >> > >>>> writeback happens, or at least not only when writeback happens.
> >> > >>>
> >> > >>> Well, I'm shy about the change there. For example, we don't strip in
> >> > >>> on open(RDWR), just on write().
> >> > >>
> >> > >> I take it back. Hooking wp_page_shared looks expensive. :) Maybe we do
> >> > >> need to hook the mmap?
> >> > >
> >> > > But file_update_time already pokes at the same (or nearby) cachelines,
> >> > > I think -- why would it be expensive?  The whole thing could be
> >> > > guarded by if (unlikely(is setuid)), right?
> >> >
> >> > Yeah, true. I added file_remove_privs calls near all the
> >> > file_update_time calls, to no effect. Added to wp_page_shared too,
> >> > nothing. Hmmm.
> >>
> >> Why not put the the should_remove_suid() call in
> >> filemap_page_mkwrite(), or maybe do_page_mkwrite()?
> >
> > page_mkwrite() callbacks are IMHO the right place for this check (and
> > change).  Just next to file_update_time() call. You get proper filesystem
> 
> Should file_update_time() just be modified to include
> file_remove_privs()? They seem to regularly go together.

They might have similar call sites, but they are completely
different operations. timestamp updates are optional, highly
configurable and behaviour is filesystem implementation specific,
whilst file_remove_privs() is mandatory and must be done in a
crash-safe manner (i.e. via transactions). Hence, IMO, they need to
be kept separate even if the call sites are similar.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/