linux-kernel - Re: [RFC 0/2] ima: evm: Add kernel cmdline options to disable IMA/EVM

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6E598674-720E-40CE-B3F2-B480323C1926@fb.com>
Date: Tue, 17 Dec 2024 23:33:29 +0000
From: Song Liu <songliubraving@...a.com>
To: Paul Moore <paul@...l-moore.com>
CC: Song Liu <songliubraving@...a.com>,
        Casey Schaufler
	<casey@...aufler-ca.com>, Song Liu <song@...nel.org>,
        "linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
        "linux-integrity@...r.kernel.org" <linux-integrity@...r.kernel.org>,
        "linux-security-module@...r.kernel.org"
	<linux-security-module@...r.kernel.org>,
        "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>,
        "roberto.sassu@...wei.com"
	<roberto.sassu@...wei.com>,
        "dmitry.kasatkin@...il.com"
	<dmitry.kasatkin@...il.com>,
        "eric.snowberg@...cle.com"
	<eric.snowberg@...cle.com>,
        "jmorris@...ei.org" <jmorris@...ei.org>,
        "serge@...lyn.com" <serge@...lyn.com>,
        Kernel Team <kernel-team@...a.com>,
        "brauner@...nel.org" <brauner@...nel.org>,
        "jack@...e.cz" <jack@...e.cz>,
        "viro@...iv.linux.org.uk" <viro@...iv.linux.org.uk>
Subject: Re: [RFC 0/2] ima: evm: Add kernel cmdline options to disable IMA/EVM



> On Dec 17, 2024, at 3:16 PM, Paul Moore <paul@...l-moore.com> wrote:
> 
> On Tue, Dec 17, 2024 at 5:47 PM Song Liu <songliubraving@...a.com> 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