[PATCH] integrity: fix IMA inode leak

[PATCH] integrity: fix IMA inode leak

[Hugh Dickins]

CONFIG_IMA=y inode activity leaks iint_cache and radix_tree_node objects
until the system runs out of memory.  Nowhere is calling ima_inode_free()
a.k.a. ima_iint_delete().  Fix that by calling it from destroy_inode().

Signed-off-by: Hugh Dickins <hugh.dickins@tiscali.co.uk>
---

 fs/inode.c |    1 +
 1 file changed, 1 insertion(+)

--- 2.6.30-rc8/fs/inode.c	2009-05-16 10:26:15.000000000 +0100
+++ linux/fs/inode.c	2009-06-06 17:41:21.000000000 +0100
@@ -219,6 +219,7 @@ static struct inode *alloc_inode(struct
 void destroy_inode(struct inode *inode)
 {
 	BUG_ON(inode_has_buffers(inode));
+	ima_inode_free(inode);
 	security_inode_free(inode);
 	if (inode->i_sb->s_op->destroy_inode)
 		inode->i_sb->s_op->destroy_inode(inode);

Re: [PATCH] integrity: fix IMA inode leak

[Linus Torvalds]

On Sat, 6 Jun 2009, Hugh Dickins wrote:
>
> CONFIG_IMA=y inode activity leaks iint_cache and radix_tree_node objects
> until the system runs out of memory.  Nowhere is calling ima_inode_free()
> a.k.a. ima_iint_delete().  Fix that by calling it from destroy_inode().

Shouldn't we call it from "security_inode_free()" instead? And shouldn't 
it be allocated in "security_inode_alloc()"? That sounds like the correct 
nesting here, since the whole integrity thing is under the security 
module.

Hmm?

		Linus

Re: [PATCH] integrity: fix IMA inode leak

[Linus Torvalds]

On Sat, 6 Jun 2009, Linus Torvalds wrote:
> 
> On Sat, 6 Jun 2009, Hugh Dickins wrote:
> >
> > CONFIG_IMA=y inode activity leaks iint_cache and radix_tree_node objects
> > until the system runs out of memory.  Nowhere is calling ima_inode_free()
> > a.k.a. ima_iint_delete().  Fix that by calling it from destroy_inode().
> 
> Shouldn't we call it from "security_inode_free()" instead? And shouldn't 
> it be allocated in "security_inode_alloc()"? That sounds like the correct 
> nesting here, since the whole integrity thing is under the security 
> module.
> 
> Hmm?

Oh well. I applied the patch as-is, since it seems to fix a real issue. 

But I do think fs/inode.c shouldn't care about things like that, and have 
it internal to security_inode_alloc/free(). But I guess that's a separate 
cleanup.

		Linus

Re: [PATCH] integrity: fix IMA inode leak

[Hugh Dickins]

On Sat, 6 Jun 2009, Linus Torvalds wrote:
> On Sat, 6 Jun 2009, Linus Torvalds wrote:
> > On Sat, 6 Jun 2009, Hugh Dickins wrote:
> > >
> > > CONFIG_IMA=y inode activity leaks iint_cache and radix_tree_node objects
> > > until the system runs out of memory.  Nowhere is calling ima_inode_free()
> > > a.k.a. ima_iint_delete().  Fix that by calling it from destroy_inode().
> > 
> > Shouldn't we call it from "security_inode_free()" instead? And shouldn't 
> > it be allocated in "security_inode_alloc()"? That sounds like the correct 
> > nesting here, since the whole integrity thing is under the security 
> > module.
> > 
> > Hmm?

I wondered the same: quite possibly, but I tend to assume there was reason
to do it like that; and thought it best to mirror the security_inode_alloc,
ima_inode_alloc with ima_inode_free, security_inode_free for now.

(I also disliked the obfuscescent #define ima_iint_delete ima_inode_free
in ima_iint.c!)

> 
> Oh well. I applied the patch as-is, since it seems to fix a real issue. 

Thanks, yes.  There is a possibility that it will reveal some warnings
from iint_free which were hidden before; but I've not seen them in normal
working, and I'd anyway prefer a few warnings to my boxes OOMing.  Though
it was only by mistake that I had CONFIG_IMA=y in the first place.

> 
> But I do think fs/inode.c shouldn't care about things like that, and have 
> it internal to security_inode_alloc/free(). But I guess that's a separate 
> cleanup.

The IMA stuff all looks rather bolted in on top; I did fail to persuade
Mimi to move the shmem and shm hooks down a level, so for now at least
they'll stay as they are.

Hugh

Re: [PATCH] integrity: fix IMA inode leak

[Mimi Zohar]

On Sat, 2009-06-06 at 14:18 -0700, Linus Torvalds wrote:
> 
> On Sat, 6 Jun 2009, Hugh Dickins wrote:
> >
> > CONFIG_IMA=y inode activity leaks iint_cache and radix_tree_node objects
> > until the system runs out of memory.  Nowhere is calling ima_inode_free()
> > a.k.a. ima_iint_delete().  Fix that by calling it from destroy_inode().
> 
> Shouldn't we call it from "security_inode_free()" instead? And shouldn't 
> it be allocated in "security_inode_alloc()"? That sounds like the correct 
> nesting here, since the whole integrity thing is under the security 
> module.
> 
> Hmm?
> 
> 		Linus

Mandatory Access Control(MAC) modules (i.e. SELinux, smack, etc) and
integrity (i.e IMA) are two different aspects of security.  The LSM
hooks, which includes security_inode_free(), are used to implement MAC,
not integrity.

Mimi Zohar

Re: [PATCH] integrity: fix IMA inode leak

[Linus Torvalds]

On Sun, 7 Jun 2009, Mimi Zohar wrote:
> 
> Mandatory Access Control(MAC) modules (i.e. SELinux, smack, etc) and
> integrity (i.e IMA) are two different aspects of security.  The LSM
> hooks, which includes security_inode_free(), are used to implement MAC,
> not integrity.

So?

It's under security/integrity. And it's a level of detail that fs/inode.c 
really doesn't care about.

The VFS layer cares NOT AT ALL about your "different aspects of security", 
nor should it. The fact that security people think SELinux and IMA are 
different is irrelavant - fs/inode.c just doesn't care. Why should it?

		Linus

Re: [PATCH] integrity: fix IMA inode leak

[Mimi Zohar]

On Sun, 2009-06-07 at 16:09 -0700, Linus Torvalds wrote:
> 
> On Sun, 7 Jun 2009, Mimi Zohar wrote:
> > 
> > Mandatory Access Control(MAC) modules (i.e. SELinux, smack, etc) and
> > integrity (i.e IMA) are two different aspects of security.  The LSM
> > hooks, which includes security_inode_free(), are used to implement MAC,
> > not integrity.
> 
> So?
> 
> It's under security/integrity. And it's a level of detail that fs/inode.c 
> really doesn't care about.
> 
> The VFS layer cares NOT AT ALL about your "different aspects of security", 
> nor should it. The fact that security people think SELinux and IMA are 
> different is irrelavant - fs/inode.c just doesn't care. Why should it?
> 
> 		Linus

Today the security calls are synomymous with MAC.  If I understand
correctly, you're suggesting we need to have a single security layer,
which, depending on the hook, calls either MAC or integrity, or both.

Makes sense. Copying the LSM mailing list on this discussion.

Mimi Zohar

Re: [PATCH] integrity: fix IMA inode leak

[Linus Torvalds]

On Mon, 8 Jun 2009, Mimi Zohar wrote:
> 
> Today the security calls are synomymous with MAC.  If I understand
> correctly, you're suggesting we need to have a single security layer,
> which, depending on the hook, calls either MAC or integrity, or both.

I don't think we need a single security layer per se.

But I do think that we _already_ hide IMA conceptually under the 
"security/" subdirectory, and that the VFS layer shouldn't need to care 
about whatever internal details.

We should not have generic code end up having to know about all the 
details, when we already have a conceptual nesting. It would be much 
better for generic code to just have to worry about one security hook that 
then encompasses all the models, than having several different hooks for 
each detail.

		Linus

Re: [PATCH] integrity: fix IMA inode leak

[Mimi Zohar]

On Mon, 2009-06-08 at 09:15 -0700, Linus Torvalds wrote:
> 
> On Mon, 8 Jun 2009, Mimi Zohar wrote:
> > 
> > Today the security calls are synomymous with MAC.  If I understand
> > correctly, you're suggesting we need to have a single security layer,
> > which, depending on the hook, calls either MAC or integrity, or both.
> 
> I don't think we need a single security layer per se.
> 
> But I do think that we _already_ hide IMA conceptually under the 
> "security/" subdirectory, and that the VFS layer shouldn't need to care 
> about whatever internal details.
> 
> We should not have generic code end up having to know about all the 
> details, when we already have a conceptual nesting. It would be much 
> better for generic code to just have to worry about one security hook that 
> then encompasses all the models, than having several different hooks for 
> each detail.
> 
> 		Linus

Ok, so instead of having a full fledge single security layer, only add
the security layer for those places where both the LSM hooks and IMA
co-exist: security_file_mmap, security_bprm_check, security_inode_alloc,
security_inode_free, and security_file_free. As the LSM hooks are called
'security_XXXX', the call would look something like:

security_all_inode_free() {
        ima_inode_free()
        security_inode_free()
}

Mimi Zohar

Re: [PATCH] integrity: fix IMA inode leak

[James Morris]

On Mon, 8 Jun 2009, Mimi Zohar wrote:

> 
> Ok, so instead of having a full fledge single security layer, only add
> the security layer for those places where both the LSM hooks and IMA
> co-exist: security_file_mmap, security_bprm_check, security_inode_alloc,
> security_inode_free, and security_file_free. As the LSM hooks are called
> 'security_XXXX', the call would look something like:
> 
> security_all_inode_free() {
>         ima_inode_free()
>         security_inode_free()
> }

Yes, it only needs to be a wrapper.  The above is ugly, how about:

security_inode_free()
{
	ima_inode_free();
	lsm_inode_free();
}

I think we may have come full circle on the naming of the LSM hook, but 
'security_*' was never great given that it's only supposed to be covering 
access control.

-- 
James Morris
<jmorris@namei.org>

Re: [PATCH] integrity: fix IMA inode leak

[Mimi Zohar]

On Tue, 2009-06-09 at 09:16 +1000, James Morris wrote:
> On Mon, 8 Jun 2009, Mimi Zohar wrote:
> 
> > 
> > Ok, so instead of having a full fledge single security layer, only add
> > the security layer for those places where both the LSM hooks and IMA
> > co-exist: security_file_mmap, security_bprm_check, security_inode_alloc,
> > security_inode_free, and security_file_free. As the LSM hooks are called
> > 'security_XXXX', the call would look something like:
> > 
> > security_all_inode_free() {
> >         ima_inode_free()
> >         security_inode_free()
> > }
> 
> Yes, it only needs to be a wrapper.  The above is ugly, how about:

agreed!  But changing only these 5 security_ hook names and leaving the
rest alone is even uglier.

> security_inode_free()
> {
> 	ima_inode_free();
> 	lsm_inode_free();
> }
> 
> I think we may have come full circle on the naming of the LSM hook, but 
> 'security_*' was never great given that it's only supposed to be covering 
> access control.

so why not 'mac_'?

Mimi Zohar

Re: [PATCH] integrity: fix IMA inode leak

[Casey Schaufler]

Mimi Zohar wrote:
> On Tue, 2009-06-09 at 09:16 +1000, James Morris wrote:
>   
>> On Mon, 8 Jun 2009, Mimi Zohar wrote:
>>
>>     
>>> Ok, so instead of having a full fledge single security layer, only add
>>> the security layer for those places where both the LSM hooks and IMA
>>> co-exist: security_file_mmap, security_bprm_check, security_inode_alloc,
>>> security_inode_free, and security_file_free. As the LSM hooks are called
>>> 'security_XXXX', the call would look something like:
>>>
>>> security_all_inode_free() {
>>>         ima_inode_free()
>>>         security_inode_free()
>>> }
>>>       
>> Yes, it only needs to be a wrapper.  The above is ugly, how about:
>>     
>
> agreed!  But changing only these 5 security_ hook names and leaving the
> rest alone is even uglier.
>
>   
>> security_inode_free()
>> {
>> 	ima_inode_free();
>> 	lsm_inode_free();
>> }
>>
>> I think we may have come full circle on the naming of the LSM hook, but 
>> 'security_*' was never great given that it's only supposed to be covering 
>> access control.
>>     
>
> so why not 'mac_'?
>   

An LSM could introduce a discretionary scheme. If you use SELinux with
just MCS that's what you get. Although POSIX ACLs can't be implemented
via the LSM (the mode bit interactions preclude doing so) there are other
ACL schemes that could use the LSM. I have gotten suggestions on "label
ownership" that would turn Smack into DAC. If you wanted to call it
Additional Access Control (AAC) or Supplemental Access Control (SAC)
I would go along with it, but not MAC.


> Mimi Zohar
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>
>
>   

Re: [PATCH] integrity: fix IMA inode leak

[Mimi Zohar]

On Sat, 2009-06-06 at 21:18 +0100, Hugh Dickins wrote:
> CONFIG_IMA=y inode activity leaks iint_cache and radix_tree_node objects
> until the system runs out of memory.  Nowhere is calling ima_inode_free()
> a.k.a. ima_iint_delete().  Fix that by calling it from destroy_inode().
> 
> Signed-off-by: Hugh Dickins <hugh.dickins@tiscali.co.uk>

This call was accidentally dropped in the conversion from
using the LIM hooks to embedding the IMA calls directly. I'm 
really sorry.

Acked-by: Mimi Zohar <zohar@linux.vnet.ibm.com>

> ---
> 
>  fs/inode.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> --- 2.6.30-rc8/fs/inode.c	2009-05-16 10:26:15.000000000 +0100
> +++ linux/fs/inode.c	2009-06-06 17:41:21.000000000 +0100
> @@ -219,6 +219,7 @@ static struct inode *alloc_inode(struct
>  void destroy_inode(struct inode *inode)
>  {
>  	BUG_ON(inode_has_buffers(inode));
> +	ima_inode_free(inode);
>  	security_inode_free(inode);
>  	if (inode->i_sb->s_op->destroy_inode)
>  		inode->i_sb->s_op->destroy_inode(inode);