> On Dec 17, 2024, at 3:16 PM, Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
>
> On Tue, Dec 17, 2024 at 5:47 PM Song Liu <songliubraving@xxxxxxxx> wrote:
>>
>> If we use lsm= to control ima and evm, we will need the following
>> changes in ordered_lsm_parse(). We still need supporting logic
>> in ima and evm side, so that ima and evm are only initialized
>> when they are in lsm=.
>>
>> Does this sound the right way forward?
>
> Have you tested it? What happens? There is value in going through
> the testing process, especially if you haven't played much with the
> LSM code.
Yes, I tested both the original patches and the "lsm=xx" version.
>
> I'd also want to see a comment line in both places explaining why it
> is necessary to mark the LSM as enabled prior to actually adding it to
> @ordered_lsms. Something along the lines of only parsing the
> parameter once should be sufficient.
Please see below for the explanation. I will add different words in
the actual comments so they make more sense as comments
>
>> diff --git i/security/security.c w/security/security.c
>> index 09664e09fec9..00271be3b0c1 100644
>> --- i/security/security.c
>> +++ w/security/security.c
>> @@ -365,6 +365,9 @@ static void __init ordered_lsm_parse(const char *order, const char *origin)
>> if (strcmp(lsm->name, name) == 0) {
>> if (lsm->order == LSM_ORDER_MUTABLE)
>> append_ordered_lsm(lsm, origin);
>> + else if (lsm->order == LSM_ORDER_LAST)
>> + set_enabled(lsm, true);
We need a flag here, saying we want to enable the lsm. We cannot do
append_ordered_lsm() yet, otherwise, it will not be "last".
>> +
>> found = true;
>> }
>> }
>> @@ -386,7 +389,7 @@ static void __init ordered_lsm_parse(const char *order, const char *origin)
>>
>> /* LSM_ORDER_LAST is always last. */
>> for (lsm = __start_lsm_info; lsm < __end_lsm_info; lsm++) {
>> - if (lsm->order == LSM_ORDER_LAST)
>> + if (lsm->order == LSM_ORDER_LAST && is_enabled(lsm))
>> append_ordered_lsm(lsm, " last");
Before this change, lsm with order==LSM_ORDER_LAST is always considered
enabled, which is a bug (if I understand you and Casey correctly).
To fix this, we need a flag from above saying we actually want to enable
it.
I personally think it is fine to use set_enabled() to set the flag.
But I don't have a strong preference, we can add a different flag.
Does this make sense?
Thanks,
Song
