Messages in this thread | | | From | Linus Torvalds <> | Date | Tue, 15 May 2012 10:19:46 -0700 | Subject | Re: [PATCH] vfs: fix IMA lockdep circular locking dependency |
| |
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*?
Sure, that changes semantics. But does the "security_ops->file_mmap()" function really need mmap_sem protection?
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
| |