Re: [RFC] namei: prevent sgid-hardlinks for unmapped gids

[Dave Chinner <david@fromorbit.com>]

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