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 <[email protected]>
---
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);
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
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
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
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 <[email protected]>
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 <[email protected]>
> ---
>
> 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);
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
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
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
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
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
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
<[email protected]>
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
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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
>
>