On Wed, Nov 29, 2023 at 1:47 PM Roberto Sassu
<[email protected]> wrote:
> On 11/29/2023 6:22 PM, Paul Moore wrote:
> > 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.
>
> The right thing in my opinion is to achieve the goal with the minimal
> set of changes, in the most intuitive way.
Once again, I want to stress that I don't care about the size of the
change, the number of patches in a patchset, etc. While it's always
nice to be able to minimize the number of changes in a patch/patchset,
that is secondary to making sure we are doing the right thing over the
long term. This is especially important when we are talking about
things that are user visible.
> Until now, there was no solution that could achieve the primary goal of
> this patch set (moving IMA and EVM to the LSM infrastructure) and, at
> the same time, achieve the additional goal you set of removing the
> 'integrity' LSM.
We need to stop thinking about the "integrity" code as a LSM, it isn't
a LSM. It's a vestigial implementation detail that was necessary back
when there could only be one LSM active at a time and there was a
desire to have IMA/EVM active in conjunction with one of the LSMs,
i.e. Smack, SELinux, etc.
IMA and EVM are (or will be) LSMs, "integrity" is not. I recognize
that eliminating the need for the "integrity" code is a relatively new
addition to this effort, but that is only because I didn't properly
understand the relationship between IMA, EVM, and the "integrity" code
until recently. The elimination of the shared "integrity" code is
consistent with promoting IMA and EVM as full LSMs, if there is core
functionality that cannot be split up into the IMA and/or EVM LSMs
then we need to look at how to support that without exposing that
implementation detail/hack to userspace. Maybe that means direct
calls between IMA and EVM, maybe that means preserving some of the
common integrity code hidden from userspace, maybe that means adding
functionality to the LSM layer, maybe that means something else?
Let's think on this to come up with something that we can all accept
as a long term solution instead of just doing the quick and easy
option.
> If you see the diff, the changes compared to v5 that was already
> accepted by Mimi are very straightforward. If the assumption I made that
> in the end the 'ima' LSM could take over the role of the 'integrity'
> LSM, that for me is the preferable option.
I looked at it quickly, but my workflow isn't well suited for patches
as attachments; inline patches (the kernel standard) is preferable.
> Given that the patch set is not doing any design change, but merely
> moving calls and storing pointers elsewhere, that leaves us with the
> option of thinking better what to do next, including like you suggested
> to make IMA and EVM use disjoint metadata.
>
> >> 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?
>
> That would be up to Mimi. Also this does not seem the main focus of the
> patch set.
Yes, it is not part of the original main focus, but it is definitely
relevant to the discussion we are having now. Once again, the most
important thing to me is that we do The Right Thing for the long term
maintenance of the code base; if that means scope creep, I've got no
problem with that.
> >> 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).
>
> Uhm, that would not be possible right away (the BPF LSM is mutable),
> remember that we defined LSM_ORDER_LAST so that an LSM can be always
> enable and placed as last (requested by Mimi)?
To be clear, I can both dislike the bpf-always-last and LSM_ORDER_LAST
concepts while accepting them as necessary evils. I'm willing to
tolerate LSM_ORDER_LAST, but I'm not currently willing to tolerate
LSM_ORDER_REALLY_LAST; that is one step too far right now. I brought
up the BPF LSM simply as an example of ordering that is not enforced
by code, but rather by documentation and convention.
--
paul-moore.com
On 11/30/2023 5:34 PM, Paul Moore wrote:
> On Wed, Nov 29, 2023 at 1:47 PM Roberto Sassu
> <[email protected]> wrote:
>> On 11/29/2023 6:22 PM, Paul Moore wrote:
>>> 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.
>>
>> The right thing in my opinion is to achieve the goal with the minimal
>> set of changes, in the most intuitive way.
>
> Once again, I want to stress that I don't care about the size of the
> change, the number of patches in a patchset, etc. While it's always
> nice to be able to minimize the number of changes in a patch/patchset,
> that is secondary to making sure we are doing the right thing over the
> long term. This is especially important when we are talking about
> things that are user visible.
If we successfully remove the 'integrity' LSM we achieve the goal.
What you say is beyond the scope of this patch set, which is just moving
IMA and EVM to the LSM infrastructure.
Of course we can discuss about nice ideas, how to improve IMA and EVM,
but again this is beyond scope.
>> Until now, there was no solution that could achieve the primary goal of
>> this patch set (moving IMA and EVM to the LSM infrastructure) and, at
>> the same time, achieve the additional goal you set of removing the
>> 'integrity' LSM.
>
> We need to stop thinking about the "integrity" code as a LSM, it isn't
> a LSM. It's a vestigial implementation detail that was necessary back
> when there could only be one LSM active at a time and there was a
> desire to have IMA/EVM active in conjunction with one of the LSMs,
> i.e. Smack, SELinux, etc.
>
> IMA and EVM are (or will be) LSMs, "integrity" is not. I recognize
> that eliminating the need for the "integrity" code is a relatively new
> addition to this effort, but that is only because I didn't properly
> understand the relationship between IMA, EVM, and the "integrity" code
> until recently. The elimination of the shared "integrity" code is
> consistent with promoting IMA and EVM as full LSMs, if there is core
> functionality that cannot be split up into the IMA and/or EVM LSMs
> then we need to look at how to support that without exposing that
> implementation detail/hack to userspace. Maybe that means direct
> calls between IMA and EVM, maybe that means preserving some of the
> common integrity code hidden from userspace, maybe that means adding
> functionality to the LSM layer, maybe that means something else?
> Let's think on this to come up with something that we can all accept
> as a long term solution instead of just doing the quick and easy
> option.
Sorry, once we find the proper way to interface the 'ima' and 'evm' LSM
with the LSM infrastructure, that is all we need to do.
Not changing any internal gives the best guarantee that the behavior
remains unchanged. And the best thing is that we are not doing a hack,
we are just preserving what is there.
Sorry, again, we are not exposing to user space any interface that is
going to change in the future once we refactor the integrity metadata
management. So, given that, I still haven't seen any compelling reason
to do the change you suggested.
>> If you see the diff, the changes compared to v5 that was already
>> accepted by Mimi are very straightforward. If the assumption I made that
>> in the end the 'ima' LSM could take over the role of the 'integrity'
>> LSM, that for me is the preferable option.
>
> I looked at it quickly, but my workflow isn't well suited for patches
> as attachments; inline patches (the kernel standard) is preferable.
Ok, no problem. I send the patches.
>> Given that the patch set is not doing any design change, but merely
>> moving calls and storing pointers elsewhere, that leaves us with the
>> option of thinking better what to do next, including like you suggested
>> to make IMA and EVM use disjoint metadata.
>>
>>>> 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?
>>
>> That would be up to Mimi. Also this does not seem the main focus of the
>> patch set.
>
> Yes, it is not part of the original main focus, but it is definitely
> relevant to the discussion we are having now. Once again, the most
> important thing to me is that we do The Right Thing for the long term
> maintenance of the code base; if that means scope creep, I've got no
> problem with that.
>
>>>> 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).
>>
>> Uhm, that would not be possible right away (the BPF LSM is mutable),
>> remember that we defined LSM_ORDER_LAST so that an LSM can be always
>> enable and placed as last (requested by Mimi)?
>
> To be clear, I can both dislike the bpf-always-last and LSM_ORDER_LAST
> concepts while accepting them as necessary evils. I'm willing to
> tolerate LSM_ORDER_LAST, but I'm not currently willing to tolerate
> LSM_ORDER_REALLY_LAST; that is one step too far right now. I brought
> up the BPF LSM simply as an example of ordering that is not enforced
> by code, but rather by documentation and convention.
Given what Petr found, we don't need an additional order.
Thanks
Roberto
On Thu, 2023-11-30 at 11:34 -0500, Paul Moore wrote:
> On Wed, Nov 29, 2023 at 1:47 PM Roberto Sassu
> <[email protected]> wrote:
> > On 11/29/2023 6:22 PM, Paul Moore wrote:
> > > 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.
> >
> > The right thing in my opinion is to achieve the goal with the minimal
> > set of changes, in the most intuitive way.
>
> Once again, I want to stress that I don't care about the size of the
> change, the number of patches in a patchset, etc. While it's always
> nice to be able to minimize the number of changes in a patch/patchset,
> that is secondary to making sure we are doing the right thing over the
> long term. This is especially important when we are talking about
> things that are user visible.
>
> > Until now, there was no solution that could achieve the primary goal of
> > this patch set (moving IMA and EVM to the LSM infrastructure) and, at
> > the same time, achieve the additional goal you set of removing the
> > 'integrity' LSM.
>
> We need to stop thinking about the "integrity" code as a LSM, it isn't
> a LSM. It's a vestigial implementation detail that was necessary back
> when there could only be one LSM active at a time and there was a
> desire to have IMA/EVM active in conjunction with one of the LSMs,
> i.e. Smack, SELinux, etc.
>
> IMA and EVM are (or will be) LSMs, "integrity" is not. I recognize
> that eliminating the need for the "integrity" code is a relatively new
> addition to this effort, but that is only because I didn't properly
> understand the relationship between IMA, EVM, and the "integrity" code
> until recently. The elimination of the shared "integrity" code is
> consistent with promoting IMA and EVM as full LSMs, if there is core
> functionality that cannot be split up into the IMA and/or EVM LSMs
> then we need to look at how to support that without exposing that
> implementation detail/hack to userspace. Maybe that means direct
> calls between IMA and EVM, maybe that means preserving some of the
> common integrity code hidden from userspace, maybe that means adding
> functionality to the LSM layer, maybe that means something else?
> Let's think on this to come up with something that we can all accept
> as a long term solution instead of just doing the quick and easy
> option.
If the result of this patch set should be that IMA and EVM become
proper LSMs without the shared integrity layer, instead of collapsing
all changes in this patch set, I think we should first verify if IMA
and EVM can be really independent. Once we guarantee that, we can
proceed making the proper LSMs.
These are the changes I have in mind:
1) Fix evm_verifyxattr(), and make it work without integrity_iint_cache
2) Remove the integrity_iint_cache parameter from evm_verifyxattr(),
since the other callers are not going to use it
3) Create an internal function with the original parameters to be used
by IMA
4) Introduce evm_post_path_mknod(), which similarly to
ima_post_path_mknod(), sets IMA_NEW_FILE for new files
5) Add hardcoded call to evm_post_path_mknod() after
ima_post_path_mknod() in security.c
If we think that this is good enough, we proceed with the move of IMA
and EVM functions to the LSM infrastructure (patches v7 19-21).
The next patches are going to be similar to patches v6 22-23, but
unlike those, their goal would be simply to split metadata, not to make
IMA and EVM independent, which at this point has been addressed
separately in the prerequisite patches.
The final patch is to remove the 'integrity' LSM and the integrity
metadata management code, which now is not used anymore.
Would that work?
Thanks
Roberto
> > If you see the diff, the changes compared to v5 that was already
> > accepted by Mimi are very straightforward. If the assumption I made that
> > in the end the 'ima' LSM could take over the role of the 'integrity'
> > LSM, that for me is the preferable option.
>
> I looked at it quickly, but my workflow isn't well suited for patches
> as attachments; inline patches (the kernel standard) is preferable.
>
> > Given that the patch set is not doing any design change, but merely
> > moving calls and storing pointers elsewhere, that leaves us with the
> > option of thinking better what to do next, including like you suggested
> > to make IMA and EVM use disjoint metadata.
> >
> > > > 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?
> >
> > That would be up to Mimi. Also this does not seem the main focus of the
> > patch set.
>
> Yes, it is not part of the original main focus, but it is definitely
> relevant to the discussion we are having now. Once again, the most
> important thing to me is that we do The Right Thing for the long term
> maintenance of the code base; if that means scope creep, I've got no
> problem with that.
>
> > > > 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).
> >
> > Uhm, that would not be possible right away (the BPF LSM is mutable),
> > remember that we defined LSM_ORDER_LAST so that an LSM can be always
> > enable and placed as last (requested by Mimi)?
>
> To be clear, I can both dislike the bpf-always-last and LSM_ORDER_LAST
> concepts while accepting them as necessary evils. I'm willing to
> tolerate LSM_ORDER_LAST, but I'm not currently willing to tolerate
> LSM_ORDER_REALLY_LAST; that is one step too far right now. I brought
> up the BPF LSM simply as an example of ordering that is not enforced
> by code, but rather by documentation and convention.
>
On Mon, 2023-12-04 at 14:26 +0100, Roberto Sassu wrote:
> On Thu, 2023-11-30 at 11:34 -0500, Paul Moore wrote:
> > On Wed, Nov 29, 2023 at 1:47 PM Roberto Sassu
> > <[email protected]> wrote:
> > > On 11/29/2023 6:22 PM, Paul Moore wrote:
> > > > 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.
> > >
> > > The right thing in my opinion is to achieve the goal with the minimal
> > > set of changes, in the most intuitive way.
> >
> > Once again, I want to stress that I don't care about the size of the
> > change, the number of patches in a patchset, etc. While it's always
> > nice to be able to minimize the number of changes in a patch/patchset,
> > that is secondary to making sure we are doing the right thing over the
> > long term. This is especially important when we are talking about
> > things that are user visible.
> >
> > > Until now, there was no solution that could achieve the primary goal of
> > > this patch set (moving IMA and EVM to the LSM infrastructure) and, at
> > > the same time, achieve the additional goal you set of removing the
> > > 'integrity' LSM.
> >
> > We need to stop thinking about the "integrity" code as a LSM, it isn't
> > a LSM. It's a vestigial implementation detail that was necessary back
> > when there could only be one LSM active at a time and there was a
> > desire to have IMA/EVM active in conjunction with one of the LSMs,
> > i.e. Smack, SELinux, etc.
> >
> > IMA and EVM are (or will be) LSMs, "integrity" is not. I recognize
> > that eliminating the need for the "integrity" code is a relatively new
> > addition to this effort, but that is only because I didn't properly
> > understand the relationship between IMA, EVM, and the "integrity" code
> > until recently. The elimination of the shared "integrity" code is
> > consistent with promoting IMA and EVM as full LSMs, if there is core
> > functionality that cannot be split up into the IMA and/or EVM LSMs
> > then we need to look at how to support that without exposing that
> > implementation detail/hack to userspace. Maybe that means direct
> > calls between IMA and EVM, maybe that means preserving some of the
> > common integrity code hidden from userspace, maybe that means adding
> > functionality to the LSM layer, maybe that means something else?
> > Let's think on this to come up with something that we can all accept
> > as a long term solution instead of just doing the quick and easy
> > option.
>
> If the result of this patch set should be that IMA and EVM become
> proper LSMs without the shared integrity layer, instead of collapsing
> all changes in this patch set, I think we should first verify if IMA
> and EVM can be really independent. Once we guarantee that, we can
> proceed making the proper LSMs.
>
> These are the changes I have in mind:
>
> 1) Fix evm_verifyxattr(), and make it work without integrity_iint_cache
> 2) Remove the integrity_iint_cache parameter from evm_verifyxattr(),
> since the other callers are not going to use it
> 3) Create an internal function with the original parameters to be used
> by IMA
> 4) Introduce evm_post_path_mknod(), which similarly to
> ima_post_path_mknod(), sets IMA_NEW_FILE for new files
> 5) Add hardcoded call to evm_post_path_mknod() after
> ima_post_path_mknod() in security.c
>
> If we think that this is good enough, we proceed with the move of IMA
> and EVM functions to the LSM infrastructure (patches v7 19-21).
>
> The next patches are going to be similar to patches v6 22-23, but
> unlike those, their goal would be simply to split metadata, not to make
> IMA and EVM independent, which at this point has been addressed
> separately in the prerequisite patches.
>
> The final patch is to remove the 'integrity' LSM and the integrity
> metadata management code, which now is not used anymore.
>
> Would that work?
Sounds good to me.
Mimi
On Mon, 2023-12-04 at 14:26 +0100, Roberto Sassu wrote:
> On Thu, 2023-11-30 at 11:34 -0500, Paul Moore wrote:
> > On Wed, Nov 29, 2023 at 1:47 PM Roberto Sassu
> > <[email protected]> wrote:
> > > On 11/29/2023 6:22 PM, Paul Moore wrote:
> > > > 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.
> > >
> > > The right thing in my opinion is to achieve the goal with the minimal
> > > set of changes, in the most intuitive way.
> >
> > Once again, I want to stress that I don't care about the size of the
> > change, the number of patches in a patchset, etc. While it's always
> > nice to be able to minimize the number of changes in a patch/patchset,
> > that is secondary to making sure we are doing the right thing over the
> > long term. This is especially important when we are talking about
> > things that are user visible.
> >
> > > Until now, there was no solution that could achieve the primary goal of
> > > this patch set (moving IMA and EVM to the LSM infrastructure) and, at
> > > the same time, achieve the additional goal you set of removing the
> > > 'integrity' LSM.
> >
> > We need to stop thinking about the "integrity" code as a LSM, it isn't
> > a LSM. It's a vestigial implementation detail that was necessary back
> > when there could only be one LSM active at a time and there was a
> > desire to have IMA/EVM active in conjunction with one of the LSMs,
> > i.e. Smack, SELinux, etc.
> >
> > IMA and EVM are (or will be) LSMs, "integrity" is not. I recognize
> > that eliminating the need for the "integrity" code is a relatively new
> > addition to this effort, but that is only because I didn't properly
> > understand the relationship between IMA, EVM, and the "integrity" code
> > until recently. The elimination of the shared "integrity" code is
> > consistent with promoting IMA and EVM as full LSMs, if there is core
> > functionality that cannot be split up into the IMA and/or EVM LSMs
> > then we need to look at how to support that without exposing that
> > implementation detail/hack to userspace. Maybe that means direct
> > calls between IMA and EVM, maybe that means preserving some of the
> > common integrity code hidden from userspace, maybe that means adding
> > functionality to the LSM layer, maybe that means something else?
> > Let's think on this to come up with something that we can all accept
> > as a long term solution instead of just doing the quick and easy
> > option.
>
> If the result of this patch set should be that IMA and EVM become
> proper LSMs without the shared integrity layer, instead of collapsing
> all changes in this patch set, I think we should first verify if IMA
> and EVM can be really independent. Once we guarantee that, we can
> proceed making the proper LSMs.
>
> These are the changes I have in mind:
>
> 1) Fix evm_verifyxattr(), and make it work without integrity_iint_cache
> 2) Remove the integrity_iint_cache parameter from evm_verifyxattr(),
> since the other callers are not going to use it
Ehm, I checked better.
integrity_inode_get() is public too (although it is not exported). So,
a caller (not IMA) could do:
iint = integrity_inode_get(inode);
status = evm_verifyxattr(..., iint);
However, it should not call integrity_inode_free(), which is also in
include/linux/integrity.h, since this is going to be called by
security_inode_free() (currently).
> 3) Create an internal function with the original parameters to be used
> by IMA
> 4) Introduce evm_post_path_mknod(), which similarly to
> ima_post_path_mknod(), sets IMA_NEW_FILE for new files
I just realized that also this is changing the current behavior.
IMA would clear IMA_NEW_FILE in ima_check_last_writer(), while EVM
wouldn't (unless we implement the file_release hook in EVM too).
> 5) Add hardcoded call to evm_post_path_mknod() after
> ima_post_path_mknod() in security.c
>
> If we think that this is good enough, we proceed with the move of IMA
> and EVM functions to the LSM infrastructure (patches v7 19-21).
>
> The next patches are going to be similar to patches v6 22-23, but
> unlike those, their goal would be simply to split metadata, not to make
> IMA and EVM independent, which at this point has been addressed
> separately in the prerequisite patches.
>
> The final patch is to remove the 'integrity' LSM and the integrity
> metadata management code, which now is not used anymore.
>
> Would that work?
We are not making much progress, I'm going to follow any recommendation
that would move this forward.
Thanks
Roberto
> Thanks
>
> Roberto
>
> > > If you see the diff, the changes compared to v5 that was already
> > > accepted by Mimi are very straightforward. If the assumption I made that
> > > in the end the 'ima' LSM could take over the role of the 'integrity'
> > > LSM, that for me is the preferable option.
> >
> > I looked at it quickly, but my workflow isn't well suited for patches
> > as attachments; inline patches (the kernel standard) is preferable.
> >
> > > Given that the patch set is not doing any design change, but merely
> > > moving calls and storing pointers elsewhere, that leaves us with the
> > > option of thinking better what to do next, including like you suggested
> > > to make IMA and EVM use disjoint metadata.
> > >
> > > > > 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?
> > >
> > > That would be up to Mimi. Also this does not seem the main focus of the
> > > patch set.
> >
> > Yes, it is not part of the original main focus, but it is definitely
> > relevant to the discussion we are having now. Once again, the most
> > important thing to me is that we do The Right Thing for the long term
> > maintenance of the code base; if that means scope creep, I've got no
> > problem with that.
> >
> > > > > 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).
> > >
> > > Uhm, that would not be possible right away (the BPF LSM is mutable),
> > > remember that we defined LSM_ORDER_LAST so that an LSM can be always
> > > enable and placed as last (requested by Mimi)?
> >
> > To be clear, I can both dislike the bpf-always-last and LSM_ORDER_LAST
> > concepts while accepting them as necessary evils. I'm willing to
> > tolerate LSM_ORDER_LAST, but I'm not currently willing to tolerate
> > LSM_ORDER_REALLY_LAST; that is one step too far right now. I brought
> > up the BPF LSM simply as an example of ordering that is not enforced
> > by code, but rather by documentation and convention.
> >
On Wed, 2023-12-06 at 14:10 +0100, Roberto Sassu wrote:
> On Mon, 2023-12-04 at 14:26 +0100, Roberto Sassu wrote:
...
> > If the result of this patch set should be that IMA and EVM become
> > proper LSMs without the shared integrity layer, instead of collapsing
> > all changes in this patch set, I think we should first verify if IMA
> > and EVM can be really independent. Once we guarantee that, we can
> > proceed making the proper LSMs.
> >
> > These are the changes I have in mind:
> >
> > 1) Fix evm_verifyxattr(), and make it work without integrity_iint_cache
> > 2) Remove the integrity_iint_cache parameter from evm_verifyxattr(),
> > since the other callers are not going to use it
>
> Ehm, I checked better.
>
> integrity_inode_get() is public too (although it is not exported). So,
> a caller (not IMA) could do:
>
> iint = integrity_inode_get(inode);
> status = evm_verifyxattr(..., iint);
>
> However, it should not call integrity_inode_free(), which is also in
> include/linux/integrity.h, since this is going to be called by
> security_inode_free() (currently).
Calling integrity_inode_free() directly would release the iint early.
As a result, IMA would then need to re-allocate it on next access.
Other than impacting IMA's performance, is this a problem?
> > 3) Create an internal function with the original parameters to be used
> > by IMA
> > 4) Introduce evm_post_path_mknod(), which similarly to
> > ima_post_path_mknod(), sets IMA_NEW_FILE for new files
>
> I just realized that also this is changing the current behavior.
>
> IMA would clear IMA_NEW_FILE in ima_check_last_writer(), while EVM
> wouldn't (unless we implement the file_release hook in EVM too).
True
Mimi
> > 5) Add hardcoded call to evm_post_path_mknod() after
> > ima_post_path_mknod() in security.c
> >
> > If we think that this is good enough, we proceed with the move of IMA
> > and EVM functions to the LSM infrastructure (patches v7 19-21).
> >
> > The next patches are going to be similar to patches v6 22-23, but
> > unlike those, their goal would be simply to split metadata, not to make
> > IMA and EVM independent, which at this point has been addressed
> > separately in the prerequisite patches.
> >
> > The final patch is to remove the 'integrity' LSM and the integrity
> > metadata management code, which now is not used anymore.
> >
> > Would that work?
>
> We are not making much progress, I'm going to follow any recommendation
> that would move this forward.
On Wed, 2023-12-06 at 11:11 -0500, Mimi Zohar wrote:
> On Wed, 2023-12-06 at 14:10 +0100, Roberto Sassu wrote:
> > On Mon, 2023-12-04 at 14:26 +0100, Roberto Sassu wrote:
>
> ...
> > > If the result of this patch set should be that IMA and EVM become
> > > proper LSMs without the shared integrity layer, instead of collapsing
> > > all changes in this patch set, I think we should first verify if IMA
> > > and EVM can be really independent. Once we guarantee that, we can
> > > proceed making the proper LSMs.
> > >
> > > These are the changes I have in mind:
> > >
> > > 1) Fix evm_verifyxattr(), and make it work without integrity_iint_cache
> > > 2) Remove the integrity_iint_cache parameter from evm_verifyxattr(),
> > > since the other callers are not going to use it
> >
> > Ehm, I checked better.
> >
> > integrity_inode_get() is public too (although it is not exported). So,
> > a caller (not IMA) could do:
> >
> > iint = integrity_inode_get(inode);
> > status = evm_verifyxattr(..., iint);
> >
> > However, it should not call integrity_inode_free(), which is also in
> > include/linux/integrity.h, since this is going to be called by
> > security_inode_free() (currently).
Oh, I needed to add a check for the iint here:
void integrity_inode_free(struct inode *inode)
{
struct integrity_iint_cache *iint;
if (!IS_IMA(inode))
return;
iint = integrity_iint_find(inode);
if (!iint) <=== this
return;
integrity_inode_set_iint(inode, NULL);
iint_free(iint);
}
> Calling integrity_inode_free() directly would release the iint early.
> As a result, IMA would then need to re-allocate it on next access.
> Other than impacting IMA's performance, is this a problem?
Uhm, I think the iint could be freed while IMA is using it, for example
in process_measurement().
Roberto
> > > 3) Create an internal function with the original parameters to be used
> > > by IMA
> > > 4) Introduce evm_post_path_mknod(), which similarly to
> > > ima_post_path_mknod(), sets IMA_NEW_FILE for new files
> >
> > I just realized that also this is changing the current behavior.
> >
> > IMA would clear IMA_NEW_FILE in ima_check_last_writer(), while EVM
> > wouldn't (unless we implement the file_release hook in EVM too).
>
> True
>
> Mimi
>
> > > 5) Add hardcoded call to evm_post_path_mknod() after
> > > ima_post_path_mknod() in security.c
> > >
> > > If we think that this is good enough, we proceed with the move of IMA
> > > and EVM functions to the LSM infrastructure (patches v7 19-21).
> > >
> > > The next patches are going to be similar to patches v6 22-23, but
> > > unlike those, their goal would be simply to split metadata, not to make
> > > IMA and EVM independent, which at this point has been addressed
> > > separately in the prerequisite patches.
> > >
> > > The final patch is to remove the 'integrity' LSM and the integrity
> > > metadata management code, which now is not used anymore.
> > >
> > > Would that work?
> >
> > We are not making much progress, I'm going to follow any recommendation
> > that would move this forward.
>