Re: [PATCH] vfs: fix IMA lockdep circular locking dependency

[Mimi Zohar <zohar@linux.vnet.ibm.com>]

On Tue, 2012-05-15 at 10:19 -0700, Linus Torvalds wrote:
> On Sun, May 13, 2012 at 7:47 PM, Mimi Zohar <zohar@us.ibm.com> wrote:
> > From: Mimi Zohar <zohar@linux.vnet.ibm.com>
> >
> > This patch has been updated to move the ima_file_mmap() call from
> > security_file_mmap() to the new vm_mmap() function.
> 
> Ugh. I really have to admit to hating this.
> 
> Quite frankly, I'd much rather apply a patch that moved the call to
> security_file_mmap() entirely outside the mmap_sem instead.
> 
> You did basically that, but you only moved the ima_file_mmap()
> portion. Why not move it *all*?

As moving the ima_file_mmap() before taking the mmap_sem is not optimal,
I never even considered moving the security_file_mmap() hook itself, nor
was this suggestion ever made during the initial RFC discussions last
fall.

Unlike execve, which locks the file before calling the security hook,
here the file locking occurs afterwards.  So, instead of moving the
ima_file_mmap() hook closer to where the file is locked, this patch
moves it even farther away.
 
> Sure, that changes semantics. But does the "security_ops->file_mmap()"
> function really need mmap_sem protection?

Having this sort of discussion should probably include the LSM hook
users.  With the Subject line above, not sure how many of them are
reading this.  Am cc'ing others...

Sorry, need to review usage, before commenting ...

Mimi

> As far as I can tell, the *only* thing that the security layer tends
> to care about is that address (ie the whole "dac_mmap_min_addr"
> thing), or the file.
> 
> And the *file* doesn't need or want any mmap_sem protection.
> 
> The actual final address does "need" the mmap_sem, but in fact none of
> the security models really care, except for the obvious NULL mapping
> thing. And that can only happen with MAP_FIXED *or* when a person
> gives an explicit suggested address.
> 
> So I would suggest:
> 
>  - never test the  default mmap address case (ie the case of a NULL
> 'addr' without PROT_FIXED set).
> 
>  - move the whole call to security_file_mmap() to outside the
> mmap_sem, and test the *suggested* address (which is not the same as
> the final address)
> 
> Yes, this makes the assumption that arch_get_unmapped_area() will not
> return a bad address. We'd have to think about that. I already found
> one possible worry, where the default arch_get_unmapped_area() does
> this:
> 
>         if (addr) {
>                 addr = PAGE_ALIGN(addr);
> 
> and maybe we need to make sure that that PAGE_ALIGN() does not
> overflow into 0. Things like that (probably right thing to do: make
> sure that 'addr' is already aligned when checking security), but the
> point being that it looks like a *really* bad idea to require us
> holding the mmap_sem() for this silly security check, when we could
> just do it up-front because nobody really cares.
> 
> Ok?
> 
> I would also actually suggest that we move the "cap_file_mmap()" call
> from the security model ->file_mmap() function into
> "security_file_mmap()", so that all the security models don't even
> have to remember to do that. Because a security model that *doesn't*
> do the dac_mmap_min_addr comparison really is broken (we used to have
> that bug in SELinux).
> 
> What do you think?
> 
>                     Linus