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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 15 May 2012 10:19:46 -0700
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Mimi Zohar <zohar@...ibm.com>
Cc:	linux-security-module@...r.kernel.org,
	Mimi Zohar <zohar@...ux.vnet.ibm.com>,
	linux-kernel@...r.kernel.org, Al Viro <viro@...iv.linux.org.uk>
Subject: Re: [PATCH] vfs: fix IMA lockdep circular locking dependency

On Sun, May 13, 2012 at 7:47 PM, Mimi Zohar <zohar@...ibm.com> wrote:
> From: Mimi Zohar <zohar@...ux.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/