Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759407AbbKTALI (ORCPT ); Thu, 19 Nov 2015 19:11:08 -0500 Received: from mail-ig0-f179.google.com ([209.85.213.179]:34324 "EHLO mail-ig0-f179.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758547AbbKTALF (ORCPT ); Thu, 19 Nov 2015 19:11:05 -0500 MIME-Version: 1.0 In-Reply-To: <20151119220241.GD19199@dastard> References: <20151108020206.GA3880@thunk.org> <20151110150829.GC3156@quack.suse.cz> <20151119220241.GD19199@dastard> Date: Thu, 19 Nov 2015 16:11:03 -0800 X-Google-Sender-Auth: _RJqMQAUfvZV7ADk790r4pcW2JY Message-ID: Subject: Re: [RFC] namei: prevent sgid-hardlinks for unmapped gids From: Kees Cook To: Dave Chinner Cc: Jan Kara , "Theodore Ts'o" , Andy Lutomirski , Theodore Tso , Willy Tarreau , Dirk Steinmetz , Michael Kerrisk-manpages , Serge Hallyn , Seth Forshee , Alexander Viro , Linux FS Devel , LKML , "Eric W . Biederman" , Serge Hallyn , "security@kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2520 Lines: 65 On Thu, Nov 19, 2015 at 2:02 PM, Dave Chinner wrote: > On Thu, Nov 19, 2015 at 12:11:11PM -0800, Kees Cook wrote: >> On Tue, Nov 10, 2015 at 7:08 AM, Jan Kara 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. Yeah, that was my worry too. Okay, I think I've got it now. I had misunderstood the purpose of the page_mkwrite variable in wp_page_reuse. My tests pass now. Patch on it's way... -Kees > > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com -- Kees Cook Chrome OS Security -- 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/