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]

Message-ID: <CA+55aFyPPZ0wayqPMCZtfP1w9nN-GnKJxYc9i_XM_Qk8d-zJzg@mail.gmail.com>
Date:	Tue, 15 May 2012 11:41:03 -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 Tue, May 15, 2012 at 10:19 AM, Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
>
>  - 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)

Actually, I think I have a simpler approach.

We already actually have two *different* security_file_mmap() calls:
it's just that currently the difference is shown by the last argument
to the function ("addr_only").

Which is totally static by call-site.

And the ones that have addr_only set are all inside the mmap_sem region.

So here's my new suggestion, which avoids the semantic changes: let's
just split that "security_file_mmap()" call into two. One that checks
the file, and one that checks the address.

And we'll continue to check the address within the mmap_sem region,
but nobody will care, because that one doesn't need any other locking.

So we'll just split the file check - along with the ima_file_mmap() -
into the "security_mmap_file()" function (no "addr_only" argument),
and move that outside the lock.

And then the address-only check (let's call it "security_mmap_addr()")
we leave at the place we *currently* do our security_file_mmap()
check, along with (obviously) the calls that currently have
"addr_only=1" and only give the address anyway).

That cleans up the whole nasty piece of crap calling convention too,
getting rid of idiotic unreadable code like

    ret = security_file_mmap(NULL, 0, 0, 0, new_addr, 1);

and replacing it with

    ret = security_mmap_addr(new_addr);

which actually shows what it is doing.

Hmm? That looks like a fairly mechanical conversion, and we could
start with Mimi's patch that already splits it up that way (except it
calls "security_mmap_file()" ima_file_mmap() and doesn't call  the LSM
function pointer there)

                   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/