Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <95a9eb64-654b-8d34-12c7-1dd7666e3420@linux.ibm.com>
Date:   Thu, 17 Feb 2022 15:59:31 -0500
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: [PATCH v10 11/27] ima: Move ima_lsm_policy_notifier into
 ima_namespace
Content-Language: en-US
To:     Mimi Zohar <zohar@linux.ibm.com>, linux-integrity@vger.kernel.org
Cc:     serge@hallyn.com, christian.brauner@ubuntu.com,
        containers@lists.linux.dev, dmitry.kasatkin@gmail.com,
        ebiederm@xmission.com, krzysztof.struczynski@huawei.com,
        roberto.sassu@huawei.com, mpeters@redhat.com, lhinds@redhat.com,
        lsturman@redhat.com, puiterwi@redhat.com, jejb@linux.ibm.com,
        jamjoom@us.ibm.com, linux-kernel@vger.kernel.org,
        paul@paul-moore.com, rgb@redhat.com,
        linux-security-module@vger.kernel.org, jmorris@namei.org
References: <20220201203735.164593-1-stefanb@linux.ibm.com>
 <20220201203735.164593-12-stefanb@linux.ibm.com>
 <62f946ec160296b6b20bee98986b2bafb8427718.camel@linux.ibm.com>
From:   Stefan Berger <stefanb@linux.ibm.com>
In-Reply-To: <62f946ec160296b6b20bee98986b2bafb8427718.camel@linux.ibm.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 2/17/22 15:30, Mimi Zohar wrote:
> On Tue, 2022-02-01 at 15:37 -0500, Stefan Berger wrote:
>
> The builtin IMA policy rules are broad and may be constrained by
> loading a custom policy, which could be defined in terms of LSM labels.
> When an LSM policy is loaded, existing LSM labels might be affected or
> even removed.  In either case, IMA policy rules based on LSM
> labels, need to reflect these changes.  If an LSM label is removed,
> instead of deleting the IMA policy rule based on the LSM label, the IMA
> policy rule is made inactive.

Ok, so I take this paragraph for the patch description.


>
>> Move the ima_lsm_policy_notifier into the ima_namespace. Each IMA
>> namespace can now register its own LSM policy change notifier callback.
>> The policy change notifier for the init_ima_ns still remains in init_ima()
>> and therefore handle the registration of the callback for all other
>> namespaces in init_ima_namespace().
>>
>> Suppress the kernel warning 'rule for LSM <label> is undefined` for all
>> other IMA namespaces than the init_ima_ns.
> Instead of ignoring the warnings totally, perhaps use either the
> "ratelimited" or "once" function options for non init_ima_ns.  It would
> be nice if these functions could be namespace aware, so that each
> affected IMA namespace would contain at least one warning.

The problem is that any user can now repeatedly create user namespaces 
and with that IMA namespaces and cause the kernel log to fill up with 
these messages and also flood the audit log -- I guess one could 
describe it as an unwanted side-effect. I am afraid that for as long as 
the kernel log is not namespaced it's probably best to just turn them 
off for non-init_ima_ns.


>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
Thanks.
>
>> ---
>> v10:
>>   - Only call pr_warn('rule for LSM <label> is undefined`) for init_ima_ns
>> ---
>>   security/integrity/ima/ima.h             |  2 ++
>>   security/integrity/ima/ima_init_ima_ns.c | 14 ++++++++++++++
>>   security/integrity/ima/ima_main.c        |  6 +-----
>>   security/integrity/ima/ima_policy.c      | 16 ++++++++++------
>>   4 files changed, 27 insertions(+), 11 deletions(-)
>>
>> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
>> index 94c6e3a4d666..fb6bd054d899 100644
>> --- a/security/integrity/ima/ima.h
>> +++ b/security/integrity/ima/ima.h
>> @@ -144,6 +144,8 @@ struct ima_namespace {
>>   	int valid_policy;
>>   
>>   	struct dentry *ima_policy;
>> +
>> +	struct notifier_block ima_lsm_policy_notifier;
>>   } __randomize_layout;
>>   extern struct ima_namespace init_ima_ns;
>>   
>> diff --git a/security/integrity/ima/ima_init_ima_ns.c b/security/integrity/ima/ima_init_ima_ns.c
>> index 425eed1c6838..1cba545ae477 100644
>> --- a/security/integrity/ima/ima_init_ima_ns.c
>> +++ b/security/integrity/ima/ima_init_ima_ns.c
>> @@ -10,6 +10,8 @@
>>   
>>   static int ima_init_namespace(struct ima_namespace *ns)
>>   {
>> +	int rc;
>> +
> There's no right or wrong, but the newer IMA code uses 'ret'.


I don't mind to change it. I will take your Reviewed-by, though :-)


>
>>   	INIT_LIST_HEAD(&ns->ima_default_rules);
>>   	INIT_LIST_HEAD(&ns->ima_policy_rules);
>>   	INIT_LIST_HEAD(&ns->ima_temp_rules);