Messages in this thread | | | Subject | Re: [PATCH] vfs: fix IMA lockdep circular locking dependency | From | Mimi Zohar <> | Date | Wed, 16 May 2012 11:47:05 -0400 |
| |
On Wed, 2012-05-16 at 10:06 -0400, Eric Paris wrote: > On Wed, May 16, 2012 at 9:52 AM, Mimi Zohar <zohar@linux.vnet.ibm.com> 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
| |