Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755871Ab2EPPsn (ORCPT ); Wed, 16 May 2012 11:48:43 -0400 Received: from e8.ny.us.ibm.com ([32.97.182.138]:48505 "EHLO e8.ny.us.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750983Ab2EPPsk (ORCPT ); Wed, 16 May 2012 11:48:40 -0400 Message-ID: <1337183225.3522.14.camel@falcor> Subject: Re: [PATCH] vfs: fix IMA lockdep circular locking dependency From: Mimi Zohar To: Eric Paris Cc: James Morris , Linus Torvalds , Al Viro , Mimi Zohar , linux-security-module@vger.kernel.org, linux-kernel@vger.kernel.org Date: Wed, 16 May 2012 11:47:05 -0400 In-Reply-To: References: <1336963631-3541-1-git-send-email-zohar@us.ibm.com> <1337112446.20904.50.camel@falcor> <20120516004251.GO22082@ZenIV.linux.org.uk> <1337175731.2492.4.camel@localhost> <1337176336.3522.5.camel@falcor> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.2.3 (3.2.3-3.fc16) Content-Transfer-Encoding: 7bit Mime-Version: 1.0 X-Content-Scanned: Fidelis XPS MAILER x-cbid: 12051615-9360-0000-0000-00000665EAEB Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3487 Lines: 90 On Wed, 2012-05-16 at 10:06 -0400, Eric Paris wrote: > On Wed, May 16, 2012 at 9:52 AM, Mimi Zohar wrote: > > On Wed, 2012-05-16 at 09:42 -0400, Eric Paris wrote: > >> On Wed, 2012-05-16 at 21:37 +1000, James Morris wrote: > >> > On Tue, 15 May 2012, Linus Torvalds wrote: > >> > > >> > > Hmm? > >> > > >> > diff --git a/security/capability.c b/security/capability.c > >> > index 5bb21b1c448c..9a19c6a54e12 100644 > >> > --- a/security/capability.c > >> > +++ b/security/capability.c > >> > @@ -949,7 +949,6 @@ void __init security_fixup_ops(struct > >> > security_operations *ops) > >> > set_to_cap_if_null(ops, file_alloc_security); > >> > set_to_cap_if_null(ops, file_free_security); > >> > set_to_cap_if_null(ops, file_ioctl); > >> > - set_to_cap_if_null(ops, file_mmap); > >> > set_to_cap_if_null(ops, file_mprotect); > >> > set_to_cap_if_null(ops, file_lock); > >> > set_to_cap_if_null(ops, file_fcntl); > >> > > >> > > >> > Do we need to add addr_map to the fixup ops? > >> > >> No. His patch works just fine without it. If you look he uses: > >> > >> + if (security_ops->mmap_file) { > >> > >> Which means since we didn't set an explicit .mmap_file, even with no > >> other LSM loaded we would be fine. > >> > >> At the moment I'd rather stick with our usual notation of forcing > >> capabilities to define every option even if all it does it return 0. If > >> Linus thinks it's a good idea to do > >> if (security_ops->function) > >> security_ops->funtion(args); > >> In the security server we should do that cleanup separately... > >> > >> -Eric > > > > James was pointing out that security_fixup_ops was not set for > > mmap_addr, not mmap_file(), which should be initialized by > > security_fixup_ops(). > > But it would be flat wrong to put it there. True historically we did > things differently, but this patch is right given Linus is doing it a > different way. > > Had Linus done what you two are suggesting: > +int security_mmap_addr(unsigned long addr) > +{ > + if (security_ops->mmap_addr) { <--- This would call cap_mmap_addr > + int ret = security_ops->mmap_addr(addr); > + if (ret) > + return ret; > + } > + return cap_mmap_addr(addr); <--- This would also call > cap_mmap_addr > +} > > See the problem? > > Like I said, we can do a full cleanup removing all of the useless > capabilities functions and turning them into an inline branch instead > of unconditional jump and return 0. We could possibly even move all > of the cap stacking into this layer instead of pushing it into the LSM > layer. But you shouldn't really do #2 without #1. (Notice here Linus > is doing both of those things, but only with this one hook) > > -Eric Sigh, I was actually thinking back to when LSM was not enabled, and the default to enable caps was via the security_fixup_ops(). The default today, without LSM enabled, is to call the cap functions directly from the security stub functions, like how Linus included the call to cap_mmap_addr(): static inline int security_mmap_addr(unsigned long addr) { return cap_mmap_addr(addr); } Mimi -- 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/