On Nov 7, 2023 Roberto Sassu <[email protected]> wrote:
>
> Before the security field of kernel objects could be shared among LSMs with
> the LSM stacking feature, IMA and EVM had to rely on an alternative storage
> of inode metadata. The association between inode metadata and inode is
> maintained through an rbtree.
>
> Because of this alternative storage mechanism, there was no need to use
> disjoint inode metadata, so IMA and EVM today still share them.
>
> With the reservation mechanism offered by the LSM infrastructure, the
> rbtree is no longer necessary, as each LSM could reserve a space in the
> security blob for each inode. However, since IMA and EVM share the
> inode metadata, they cannot directly reserve the space for them.
>
> Instead, request from the 'integrity' LSM a space in the security blob for
> the pointer of inode metadata (integrity_iint_cache structure). The other
> reason for keeping the 'integrity' LSM is to preserve the original ordering
> of IMA and EVM functions as when they were hardcoded.
>
> Prefer reserving space for a pointer to allocating the integrity_iint_cache
> structure directly, as IMA would require it only for a subset of inodes.
> Always allocating it would cause a waste of memory.
>
> Introduce two primitives for getting and setting the pointer of
> integrity_iint_cache in the security blob, respectively
> integrity_inode_get_iint() and integrity_inode_set_iint(). This would make
> the code more understandable, as they directly replace rbtree operations.
>
> Locking is not needed, as access to inode metadata is not shared, it is per
> inode.
>
> Signed-off-by: Roberto Sassu <[email protected]>
> Reviewed-by: Casey Schaufler <[email protected]>
> Reviewed-by: Mimi Zohar <[email protected]>
> ---
> security/integrity/iint.c | 71 +++++-----------------------------
> security/integrity/integrity.h | 20 +++++++++-
> 2 files changed, 29 insertions(+), 62 deletions(-)
>
> diff --git a/security/integrity/iint.c b/security/integrity/iint.c
> index 882fde2a2607..a5edd3c70784 100644
> --- a/security/integrity/iint.c
> +++ b/security/integrity/iint.c
> @@ -231,6 +175,10 @@ static int __init integrity_lsm_init(void)
> return 0;
> }
>
> +struct lsm_blob_sizes integrity_blob_sizes __ro_after_init = {
> + .lbs_inode = sizeof(struct integrity_iint_cache *),
> +};
I'll admit that I'm likely missing an important detail, but is there
a reason why you couldn't stash the integrity_iint_cache struct
directly in the inode's security blob instead of the pointer? For
example:
struct lsm_blob_sizes ... = {
.lbs_inode = sizeof(struct integrity_iint_cache),
};
struct integrity_iint_cache *integrity_inode_get(inode)
{
if (unlikely(!inode->isecurity))
return NULL;
return inode->i_security + integrity_blob_sizes.lbs_inode;
}
--
paul-moore.com
On Fri, 2023-11-17 at 15:57 -0500, Paul Moore wrote:
> On Nov 7, 2023 Roberto Sassu <[email protected]> wrote:
> >
> > Before the security field of kernel objects could be shared among LSMs with
> > the LSM stacking feature, IMA and EVM had to rely on an alternative storage
> > of inode metadata. The association between inode metadata and inode is
> > maintained through an rbtree.
> >
> > Because of this alternative storage mechanism, there was no need to use
> > disjoint inode metadata, so IMA and EVM today still share them.
> >
> > With the reservation mechanism offered by the LSM infrastructure, the
> > rbtree is no longer necessary, as each LSM could reserve a space in the
> > security blob for each inode. However, since IMA and EVM share the
> > inode metadata, they cannot directly reserve the space for them.
> >
> > Instead, request from the 'integrity' LSM a space in the security blob for
> > the pointer of inode metadata (integrity_iint_cache structure). The other
> > reason for keeping the 'integrity' LSM is to preserve the original ordering
> > of IMA and EVM functions as when they were hardcoded.
> >
> > Prefer reserving space for a pointer to allocating the integrity_iint_cache
> > structure directly, as IMA would require it only for a subset of inodes.
> > Always allocating it would cause a waste of memory.
> >
> > Introduce two primitives for getting and setting the pointer of
> > integrity_iint_cache in the security blob, respectively
> > integrity_inode_get_iint() and integrity_inode_set_iint(). This would make
> > the code more understandable, as they directly replace rbtree operations.
> >
> > Locking is not needed, as access to inode metadata is not shared, it is per
> > inode.
> >
> > Signed-off-by: Roberto Sassu <[email protected]>
> > Reviewed-by: Casey Schaufler <[email protected]>
> > Reviewed-by: Mimi Zohar <[email protected]>
> > ---
> > security/integrity/iint.c | 71 +++++-----------------------------
> > security/integrity/integrity.h | 20 +++++++++-
> > 2 files changed, 29 insertions(+), 62 deletions(-)
> >
> > diff --git a/security/integrity/iint.c b/security/integrity/iint.c
> > index 882fde2a2607..a5edd3c70784 100644
> > --- a/security/integrity/iint.c
> > +++ b/security/integrity/iint.c
> > @@ -231,6 +175,10 @@ static int __init integrity_lsm_init(void)
> > return 0;
> > }
> >
> > +struct lsm_blob_sizes integrity_blob_sizes __ro_after_init = {
> > + .lbs_inode = sizeof(struct integrity_iint_cache *),
> > +};
>
> I'll admit that I'm likely missing an important detail, but is there
> a reason why you couldn't stash the integrity_iint_cache struct
> directly in the inode's security blob instead of the pointer? For
> example:
>
> struct lsm_blob_sizes ... = {
> .lbs_inode = sizeof(struct integrity_iint_cache),
> };
>
> struct integrity_iint_cache *integrity_inode_get(inode)
> {
> if (unlikely(!inode->isecurity))
> return NULL;
> return inode->i_security + integrity_blob_sizes.lbs_inode;
> }
It would increase memory occupation. Sometimes the IMA policy
encompasses a small subset of the inodes. Allocating the full
integrity_iint_cache would be a waste of memory, I guess?
On the other hand... (did not think fully about that) if we embed the
full structure in the security blob, we already have a mutex available
to use, and we don't need to take the inode lock (?).
I'm fully convinced that we can improve the implementation
significantly. I just was really hoping to go step by step and not
accumulating improvements as dependency for moving IMA and EVM to the
LSM infrastructure.
Thanks
Roberto
On Mon, Nov 20, 2023 at 3:16 AM Roberto Sassu
<[email protected]> wrote:
> On Fri, 2023-11-17 at 15:57 -0500, Paul Moore wrote:
> > On Nov 7, 2023 Roberto Sassu <[email protected]> wrote:
> > >
> > > Before the security field of kernel objects could be shared among LSMs with
> > > the LSM stacking feature, IMA and EVM had to rely on an alternative storage
> > > of inode metadata. The association between inode metadata and inode is
> > > maintained through an rbtree.
> > >
> > > Because of this alternative storage mechanism, there was no need to use
> > > disjoint inode metadata, so IMA and EVM today still share them.
> > >
> > > With the reservation mechanism offered by the LSM infrastructure, the
> > > rbtree is no longer necessary, as each LSM could reserve a space in the
> > > security blob for each inode. However, since IMA and EVM share the
> > > inode metadata, they cannot directly reserve the space for them.
> > >
> > > Instead, request from the 'integrity' LSM a space in the security blob for
> > > the pointer of inode metadata (integrity_iint_cache structure). The other
> > > reason for keeping the 'integrity' LSM is to preserve the original ordering
> > > of IMA and EVM functions as when they were hardcoded.
> > >
> > > Prefer reserving space for a pointer to allocating the integrity_iint_cache
> > > structure directly, as IMA would require it only for a subset of inodes.
> > > Always allocating it would cause a waste of memory.
> > >
> > > Introduce two primitives for getting and setting the pointer of
> > > integrity_iint_cache in the security blob, respectively
> > > integrity_inode_get_iint() and integrity_inode_set_iint(). This would make
> > > the code more understandable, as they directly replace rbtree operations.
> > >
> > > Locking is not needed, as access to inode metadata is not shared, it is per
> > > inode.
> > >
> > > Signed-off-by: Roberto Sassu <[email protected]>
> > > Reviewed-by: Casey Schaufler <[email protected]>
> > > Reviewed-by: Mimi Zohar <[email protected]>
> > > ---
> > > security/integrity/iint.c | 71 +++++-----------------------------
> > > security/integrity/integrity.h | 20 +++++++++-
> > > 2 files changed, 29 insertions(+), 62 deletions(-)
> > >
> > > diff --git a/security/integrity/iint.c b/security/integrity/iint.c
> > > index 882fde2a2607..a5edd3c70784 100644
> > > --- a/security/integrity/iint.c
> > > +++ b/security/integrity/iint.c
> > > @@ -231,6 +175,10 @@ static int __init integrity_lsm_init(void)
> > > return 0;
> > > }
> > >
> > > +struct lsm_blob_sizes integrity_blob_sizes __ro_after_init = {
> > > + .lbs_inode = sizeof(struct integrity_iint_cache *),
> > > +};
> >
> > I'll admit that I'm likely missing an important detail, but is there
> > a reason why you couldn't stash the integrity_iint_cache struct
> > directly in the inode's security blob instead of the pointer? For
> > example:
> >
> > struct lsm_blob_sizes ... = {
> > .lbs_inode = sizeof(struct integrity_iint_cache),
> > };
> >
> > struct integrity_iint_cache *integrity_inode_get(inode)
> > {
> > if (unlikely(!inode->isecurity))
> > return NULL;
> > return inode->i_security + integrity_blob_sizes.lbs_inode;
> > }
>
> It would increase memory occupation. Sometimes the IMA policy
> encompasses a small subset of the inodes. Allocating the full
> integrity_iint_cache would be a waste of memory, I guess?
Perhaps, but if it allows us to remove another layer of dynamic memory
I would argue that it may be worth the cost. It's also worth
considering the size of integrity_iint_cache, while it isn't small, it
isn't exactly huge either.
> On the other hand... (did not think fully about that) if we embed the
> full structure in the security blob, we already have a mutex available
> to use, and we don't need to take the inode lock (?).
That would be excellent, getting rid of a layer of locking would be significant.
> I'm fully convinced that we can improve the implementation
> significantly. I just was really hoping to go step by step and not
> accumulating improvements as dependency for moving IMA and EVM to the
> LSM infrastructure.
I understand, and I agree that an iterative approach is a good idea, I
just want to make sure we keep things tidy from a user perspective,
i.e. not exposing the "integrity" LSM when it isn't required.
--
paul-moore.com
On Mon, 2023-11-20 at 16:06 -0500, Paul Moore wrote:
> On Mon, Nov 20, 2023 at 3:16 AM Roberto Sassu
> <[email protected]> wrote:
> > On Fri, 2023-11-17 at 15:57 -0500, Paul Moore wrote:
> > > On Nov 7, 2023 Roberto Sassu <[email protected]> wrote:
> > > >
> > > > Before the security field of kernel objects could be shared among LSMs with
> > > > the LSM stacking feature, IMA and EVM had to rely on an alternative storage
> > > > of inode metadata. The association between inode metadata and inode is
> > > > maintained through an rbtree.
> > > >
> > > > Because of this alternative storage mechanism, there was no need to use
> > > > disjoint inode metadata, so IMA and EVM today still share them.
> > > >
> > > > With the reservation mechanism offered by the LSM infrastructure, the
> > > > rbtree is no longer necessary, as each LSM could reserve a space in the
> > > > security blob for each inode. However, since IMA and EVM share the
> > > > inode metadata, they cannot directly reserve the space for them.
> > > >
> > > > Instead, request from the 'integrity' LSM a space in the security blob for
> > > > the pointer of inode metadata (integrity_iint_cache structure). The other
> > > > reason for keeping the 'integrity' LSM is to preserve the original ordering
> > > > of IMA and EVM functions as when they were hardcoded.
> > > >
> > > > Prefer reserving space for a pointer to allocating the integrity_iint_cache
> > > > structure directly, as IMA would require it only for a subset of inodes.
> > > > Always allocating it would cause a waste of memory.
> > > >
> > > > Introduce two primitives for getting and setting the pointer of
> > > > integrity_iint_cache in the security blob, respectively
> > > > integrity_inode_get_iint() and integrity_inode_set_iint(). This would make
> > > > the code more understandable, as they directly replace rbtree operations.
> > > >
> > > > Locking is not needed, as access to inode metadata is not shared, it is per
> > > > inode.
> > > >
> > > > Signed-off-by: Roberto Sassu <[email protected]>
> > > > Reviewed-by: Casey Schaufler <[email protected]>
> > > > Reviewed-by: Mimi Zohar <[email protected]>
> > > > ---
> > > > security/integrity/iint.c | 71 +++++-----------------------------
> > > > security/integrity/integrity.h | 20 +++++++++-
> > > > 2 files changed, 29 insertions(+), 62 deletions(-)
> > > >
> > > > diff --git a/security/integrity/iint.c b/security/integrity/iint.c
> > > > index 882fde2a2607..a5edd3c70784 100644
> > > > --- a/security/integrity/iint.c
> > > > +++ b/security/integrity/iint.c
> > > > @@ -231,6 +175,10 @@ static int __init integrity_lsm_init(void)
> > > > return 0;
> > > > }
> > > >
> > > > +struct lsm_blob_sizes integrity_blob_sizes __ro_after_init = {
> > > > + .lbs_inode = sizeof(struct integrity_iint_cache *),
> > > > +};
> > >
> > > I'll admit that I'm likely missing an important detail, but is there
> > > a reason why you couldn't stash the integrity_iint_cache struct
> > > directly in the inode's security blob instead of the pointer? For
> > > example:
> > >
> > > struct lsm_blob_sizes ... = {
> > > .lbs_inode = sizeof(struct integrity_iint_cache),
> > > };
> > >
> > > struct integrity_iint_cache *integrity_inode_get(inode)
> > > {
> > > if (unlikely(!inode->isecurity))
> > > return NULL;
> > > return inode->i_security + integrity_blob_sizes.lbs_inode;
> > > }
> >
> > It would increase memory occupation. Sometimes the IMA policy
> > encompasses a small subset of the inodes. Allocating the full
> > integrity_iint_cache would be a waste of memory, I guess?
>
> Perhaps, but if it allows us to remove another layer of dynamic memory
> I would argue that it may be worth the cost. It's also worth
> considering the size of integrity_iint_cache, while it isn't small, it
> isn't exactly huge either.
>
> > On the other hand... (did not think fully about that) if we embed the
> > full structure in the security blob, we already have a mutex available
> > to use, and we don't need to take the inode lock (?).
>
> That would be excellent, getting rid of a layer of locking would be significant.
>
> > I'm fully convinced that we can improve the implementation
> > significantly. I just was really hoping to go step by step and not
> > accumulating improvements as dependency for moving IMA and EVM to the
> > LSM infrastructure.
>
> I understand, and I agree that an iterative approach is a good idea, I
> just want to make sure we keep things tidy from a user perspective,
> i.e. not exposing the "integrity" LSM when it isn't required.
Ok, I went back to it again.
I think trying to separate integrity metadata is premature now, too
many things at the same time.
I started to think, does EVM really need integrity metadata or it can
work without?
The fact is that CONFIG_IMA=n and CONFIG_EVM=y is allowed, so we have
the same problem now. What if we make IMA the one that manages
integrity metadata, so that we can remove the 'integrity' LSM?
So, no embedding the full structure in the security blob now, move
integrity_inode_free() and integrity_kernel_module_request() to IMA,
call integrity_iintcache_init() from IMA.
EVM verification of new files would fail without IMA, but it would be
the same now.
Also, evm_verifyxattr() would only work with IMA, as it assumes that
the latter creates integrity metadata and passes them as argument.
Regarding the LSM order, I would take Casey's suggestion of introducing
LSM_ORDER_REALLY_LAST, for EVM.
Thanks
Roberto
On Wed, 2023-11-29 at 13:27 +0100, Roberto Sassu wrote:
> On Mon, 2023-11-20 at 16:06 -0500, Paul Moore wrote:
> > On Mon, Nov 20, 2023 at 3:16 AM Roberto Sassu
> > <[email protected]> wrote:
> > > On Fri, 2023-11-17 at 15:57 -0500, Paul Moore wrote:
> > > > On Nov 7, 2023 Roberto Sassu <[email protected]> wrote:
> > > > >
> > > > > Before the security field of kernel objects could be shared among LSMs with
> > > > > the LSM stacking feature, IMA and EVM had to rely on an alternative storage
> > > > > of inode metadata. The association between inode metadata and inode is
> > > > > maintained through an rbtree.
> > > > >
> > > > > Because of this alternative storage mechanism, there was no need to use
> > > > > disjoint inode metadata, so IMA and EVM today still share them.
> > > > >
> > > > > With the reservation mechanism offered by the LSM infrastructure, the
> > > > > rbtree is no longer necessary, as each LSM could reserve a space in the
> > > > > security blob for each inode. However, since IMA and EVM share the
> > > > > inode metadata, they cannot directly reserve the space for them.
> > > > >
> > > > > Instead, request from the 'integrity' LSM a space in the security blob for
> > > > > the pointer of inode metadata (integrity_iint_cache structure). The other
> > > > > reason for keeping the 'integrity' LSM is to preserve the original ordering
> > > > > of IMA and EVM functions as when they were hardcoded.
> > > > >
> > > > > Prefer reserving space for a pointer to allocating the integrity_iint_cache
> > > > > structure directly, as IMA would require it only for a subset of inodes.
> > > > > Always allocating it would cause a waste of memory.
> > > > >
> > > > > Introduce two primitives for getting and setting the pointer of
> > > > > integrity_iint_cache in the security blob, respectively
> > > > > integrity_inode_get_iint() and integrity_inode_set_iint(). This would make
> > > > > the code more understandable, as they directly replace rbtree operations.
> > > > >
> > > > > Locking is not needed, as access to inode metadata is not shared, it is per
> > > > > inode.
> > > > >
> > > > > Signed-off-by: Roberto Sassu <[email protected]>
> > > > > Reviewed-by: Casey Schaufler <[email protected]>
> > > > > Reviewed-by: Mimi Zohar <[email protected]>
> > > > > ---
> > > > > security/integrity/iint.c | 71 +++++-----------------------------
> > > > > security/integrity/integrity.h | 20 +++++++++-
> > > > > 2 files changed, 29 insertions(+), 62 deletions(-)
> > > > >
> > > > > diff --git a/security/integrity/iint.c b/security/integrity/iint.c
> > > > > index 882fde2a2607..a5edd3c70784 100644
> > > > > --- a/security/integrity/iint.c
> > > > > +++ b/security/integrity/iint.c
> > > > > @@ -231,6 +175,10 @@ static int __init integrity_lsm_init(void)
> > > > > return 0;
> > > > > }
> > > > >
> > > > > +struct lsm_blob_sizes integrity_blob_sizes __ro_after_init = {
> > > > > + .lbs_inode = sizeof(struct integrity_iint_cache *),
> > > > > +};
> > > >
> > > > I'll admit that I'm likely missing an important detail, but is there
> > > > a reason why you couldn't stash the integrity_iint_cache struct
> > > > directly in the inode's security blob instead of the pointer? For
> > > > example:
> > > >
> > > > struct lsm_blob_sizes ... = {
> > > > .lbs_inode = sizeof(struct integrity_iint_cache),
> > > > };
> > > >
> > > > struct integrity_iint_cache *integrity_inode_get(inode)
> > > > {
> > > > if (unlikely(!inode->isecurity))
> > > > return NULL;
> > > > return inode->i_security + integrity_blob_sizes.lbs_inode;
> > > > }
> > >
> > > It would increase memory occupation. Sometimes the IMA policy
> > > encompasses a small subset of the inodes. Allocating the full
> > > integrity_iint_cache would be a waste of memory, I guess?
> >
> > Perhaps, but if it allows us to remove another layer of dynamic memory
> > I would argue that it may be worth the cost. It's also worth
> > considering the size of integrity_iint_cache, while it isn't small, it
> > isn't exactly huge either.
> >
> > > On the other hand... (did not think fully about that) if we embed the
> > > full structure in the security blob, we already have a mutex available
> > > to use, and we don't need to take the inode lock (?).
> >
> > That would be excellent, getting rid of a layer of locking would be significant.
> >
> > > I'm fully convinced that we can improve the implementation
> > > significantly. I just was really hoping to go step by step and not
> > > accumulating improvements as dependency for moving IMA and EVM to the
> > > LSM infrastructure.
> >
> > I understand, and I agree that an iterative approach is a good idea, I
> > just want to make sure we keep things tidy from a user perspective,
> > i.e. not exposing the "integrity" LSM when it isn't required.
>
> Ok, I went back to it again.
>
> I think trying to separate integrity metadata is premature now, too
> many things at the same time.
>
> I started to think, does EVM really need integrity metadata or it can
> work without?
>
> The fact is that CONFIG_IMA=n and CONFIG_EVM=y is allowed, so we have
> the same problem now. What if we make IMA the one that manages
> integrity metadata, so that we can remove the 'integrity' LSM?
>
> So, no embedding the full structure in the security blob now, move
> integrity_inode_free() and integrity_kernel_module_request() to IMA,
> call integrity_iintcache_init() from IMA.
>
> EVM verification of new files would fail without IMA, but it would be
> the same now.
>
> Also, evm_verifyxattr() would only work with IMA, as it assumes that
> the latter creates integrity metadata and passes them as argument.
>
> Regarding the LSM order, I would take Casey's suggestion of introducing
> LSM_ORDER_REALLY_LAST, for EVM.
I attach the diff v5..v7.
Tests passes with both IMA and EVM enabled. I did minor tweaks to the
tests to take into account the possibility that IMA is disabled, and
tests pass also in this case.
Roberto
On Wed, Nov 29, 2023 at 7:28 AM Roberto Sassu
<[email protected]> wrote:
>
> On Mon, 2023-11-20 at 16:06 -0500, Paul Moore wrote:
> > On Mon, Nov 20, 2023 at 3:16 AM Roberto Sassu
> > <[email protected]> wrote:
> > > On Fri, 2023-11-17 at 15:57 -0500, Paul Moore wrote:
> > > > On Nov 7, 2023 Roberto Sassu <[email protected]> wrote:
> > > > >
> > > > > Before the security field of kernel objects could be shared among LSMs with
> > > > > the LSM stacking feature, IMA and EVM had to rely on an alternative storage
> > > > > of inode metadata. The association between inode metadata and inode is
> > > > > maintained through an rbtree.
> > > > >
> > > > > Because of this alternative storage mechanism, there was no need to use
> > > > > disjoint inode metadata, so IMA and EVM today still share them.
> > > > >
> > > > > With the reservation mechanism offered by the LSM infrastructure, the
> > > > > rbtree is no longer necessary, as each LSM could reserve a space in the
> > > > > security blob for each inode. However, since IMA and EVM share the
> > > > > inode metadata, they cannot directly reserve the space for them.
> > > > >
> > > > > Instead, request from the 'integrity' LSM a space in the security blob for
> > > > > the pointer of inode metadata (integrity_iint_cache structure). The other
> > > > > reason for keeping the 'integrity' LSM is to preserve the original ordering
> > > > > of IMA and EVM functions as when they were hardcoded.
> > > > >
> > > > > Prefer reserving space for a pointer to allocating the integrity_iint_cache
> > > > > structure directly, as IMA would require it only for a subset of inodes.
> > > > > Always allocating it would cause a waste of memory.
> > > > >
> > > > > Introduce two primitives for getting and setting the pointer of
> > > > > integrity_iint_cache in the security blob, respectively
> > > > > integrity_inode_get_iint() and integrity_inode_set_iint(). This would make
> > > > > the code more understandable, as they directly replace rbtree operations.
> > > > >
> > > > > Locking is not needed, as access to inode metadata is not shared, it is per
> > > > > inode.
> > > > >
> > > > > Signed-off-by: Roberto Sassu <[email protected]>
> > > > > Reviewed-by: Casey Schaufler <[email protected]>
> > > > > Reviewed-by: Mimi Zohar <[email protected]>
> > > > > ---
> > > > > security/integrity/iint.c | 71 +++++-----------------------------
> > > > > security/integrity/integrity.h | 20 +++++++++-
> > > > > 2 files changed, 29 insertions(+), 62 deletions(-)
> > > > >
> > > > > diff --git a/security/integrity/iint.c b/security/integrity/iint.c
> > > > > index 882fde2a2607..a5edd3c70784 100644
> > > > > --- a/security/integrity/iint.c
> > > > > +++ b/security/integrity/iint.c
> > > > > @@ -231,6 +175,10 @@ static int __init integrity_lsm_init(void)
> > > > > return 0;
> > > > > }
> > > > >
> > > > > +struct lsm_blob_sizes integrity_blob_sizes __ro_after_init = {
> > > > > + .lbs_inode = sizeof(struct integrity_iint_cache *),
> > > > > +};
> > > >
> > > > I'll admit that I'm likely missing an important detail, but is there
> > > > a reason why you couldn't stash the integrity_iint_cache struct
> > > > directly in the inode's security blob instead of the pointer? For
> > > > example:
> > > >
> > > > struct lsm_blob_sizes ... = {
> > > > .lbs_inode = sizeof(struct integrity_iint_cache),
> > > > };
> > > >
> > > > struct integrity_iint_cache *integrity_inode_get(inode)
> > > > {
> > > > if (unlikely(!inode->isecurity))
> > > > return NULL;
> > > > return inode->i_security + integrity_blob_sizes.lbs_inode;
> > > > }
> > >
> > > It would increase memory occupation. Sometimes the IMA policy
> > > encompasses a small subset of the inodes. Allocating the full
> > > integrity_iint_cache would be a waste of memory, I guess?
> >
> > Perhaps, but if it allows us to remove another layer of dynamic memory
> > I would argue that it may be worth the cost. It's also worth
> > considering the size of integrity_iint_cache, while it isn't small, it
> > isn't exactly huge either.
> >
> > > On the other hand... (did not think fully about that) if we embed the
> > > full structure in the security blob, we already have a mutex available
> > > to use, and we don't need to take the inode lock (?).
> >
> > That would be excellent, getting rid of a layer of locking would be significant.
> >
> > > I'm fully convinced that we can improve the implementation
> > > significantly. I just was really hoping to go step by step and not
> > > accumulating improvements as dependency for moving IMA and EVM to the
> > > LSM infrastructure.
> >
> > I understand, and I agree that an iterative approach is a good idea, I
> > just want to make sure we keep things tidy from a user perspective,
> > i.e. not exposing the "integrity" LSM when it isn't required.
>
> Ok, I went back to it again.
>
> I think trying to separate integrity metadata is premature now, too
> many things at the same time.
I'm not bothered by the size of the patchset, it is more important
that we do The Right Thing. I would like to hear in more detail why
you don't think this will work, I'm not interested in hearing about
difficult it may be, I'm interested in hearing about what challenges
we need to solve to do this properly.
> I started to think, does EVM really need integrity metadata or it can
> work without?
>
> The fact is that CONFIG_IMA=n and CONFIG_EVM=y is allowed, so we have
> the same problem now. What if we make IMA the one that manages
> integrity metadata, so that we can remove the 'integrity' LSM?
I guess we should probably revisit the basic idea of if it even makes
sense to enable EVM without IMA? Should we update the Kconfig to
require IMA when EVM is enabled?
> Regarding the LSM order, I would take Casey's suggestion of introducing
> LSM_ORDER_REALLY_LAST, for EVM.
Please understand that I really dislike that we have imposed ordering
constraints at the LSM layer, but I do understand the necessity (the
BPF LSM ordering upsets me the most). I really don't want to see us
make things worse by adding yet another ordering bucket, I would
rather that we document it well and leave it alone ... basically treat
it like the BPF LSM (grrrrrr).
--
paul-moore.com
On 17.11.23 21:57, Paul Moore wrote:
> On Nov 7, 2023 Roberto Sassu <[email protected]> wrote:
>>
>> Before the security field of kernel objects could be shared among LSMs with
>> the LSM stacking feature, IMA and EVM had to rely on an alternative storage
>> of inode metadata. The association between inode metadata and inode is
>> maintained through an rbtree.
>>
>> Because of this alternative storage mechanism, there was no need to use
>> disjoint inode metadata, so IMA and EVM today still share them.
>>
>> With the reservation mechanism offered by the LSM infrastructure, the
>> rbtree is no longer necessary, as each LSM could reserve a space in the
>> security blob for each inode. However, since IMA and EVM share the
>> inode metadata, they cannot directly reserve the space for them.
>>
>> Instead, request from the 'integrity' LSM a space in the security blob for
>> the pointer of inode metadata (integrity_iint_cache structure). The other
>> reason for keeping the 'integrity' LSM is to preserve the original ordering
>> of IMA and EVM functions as when they were hardcoded.
>>
>> Prefer reserving space for a pointer to allocating the integrity_iint_cache
>> structure directly, as IMA would require it only for a subset of inodes.
>> Always allocating it would cause a waste of memory.
>>
>> Introduce two primitives for getting and setting the pointer of
>> integrity_iint_cache in the security blob, respectively
>> integrity_inode_get_iint() and integrity_inode_set_iint(). This would make
>> the code more understandable, as they directly replace rbtree operations.
>>
>> Locking is not needed, as access to inode metadata is not shared, it is per
>> inode.
>>
>> Signed-off-by: Roberto Sassu <[email protected]>
>> Reviewed-by: Casey Schaufler <[email protected]>
>> Reviewed-by: Mimi Zohar <[email protected]>
>> ---
>> security/integrity/iint.c | 71 +++++-----------------------------
>> security/integrity/integrity.h | 20 +++++++++-
>> 2 files changed, 29 insertions(+), 62 deletions(-)
>>
>> diff --git a/security/integrity/iint.c b/security/integrity/iint.c
>> index 882fde2a2607..a5edd3c70784 100644
>> --- a/security/integrity/iint.c
>> +++ b/security/integrity/iint.c
>> @@ -231,6 +175,10 @@ static int __init integrity_lsm_init(void)
>> return 0;
>> }
>>
>> +struct lsm_blob_sizes integrity_blob_sizes __ro_after_init = {
>> + .lbs_inode = sizeof(struct integrity_iint_cache *),
>> +};
>
> I'll admit that I'm likely missing an important detail, but is there
> a reason why you couldn't stash the integrity_iint_cache struct
> directly in the inode's security blob instead of the pointer? For
> example:
>
> struct lsm_blob_sizes ... = {
> .lbs_inode = sizeof(struct integrity_iint_cache),
> };
>
> struct integrity_iint_cache *integrity_inode_get(inode)
> {
> if (unlikely(!inode->isecurity))
> return NULL;
Ok, this caught my attention...
I see that selinux_inode() has it, but smack_inode() doesn't.
Some Smack code assumes that the inode security blob is always non-NULL:
static void init_inode_smack(struct inode *inode, struct smack_known *skp)
{
struct inode_smack *isp = smack_inode(inode);
isp->smk_inode = skp;
isp->smk_flags = 0;
}
Is that intended? Should I add the check?
Thanks
Roberto
> return inode->i_security + integrity_blob_sizes.lbs_inode;
> }
>
> --
> paul-moore.com