Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <546afc0c-8d86-e471-49cd-8665cea643fb@linux.ibm.com>
Date:   Tue, 18 Jan 2022 13:25:38 -0500
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.3.0
Subject: Re: [PATCH v8 10/19] ima: Implement hierarchical processing of file
 accesses
Content-Language: en-US
To:     Christian Brauner <brauner@kernel.org>,
        Stefan Berger <stefanb@linux.vnet.ibm.com>
Cc:     linux-integrity@vger.kernel.org, zohar@linux.ibm.com,
        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: <20220104170416.1923685-1-stefanb@linux.vnet.ibm.com>
 <20220104170416.1923685-11-stefanb@linux.vnet.ibm.com>
 <20220114112114.tu4f56bm7tewzfmj@wittgenstein>
From:   Stefan Berger <stefanb@linux.ibm.com>
In-Reply-To: <20220114112114.tu4f56bm7tewzfmj@wittgenstein>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 1/14/22 06:21, Christian Brauner wrote:
> On Tue, Jan 04, 2022 at 12:04:07PM -0500, Stefan Berger wrote:
>> From: Stefan Berger <stefanb@linux.ibm.com>
>>
>> Implement hierarchical processing of file accesses in IMA namespaces by
>> walking the list of user namespaces towards the root. This way file
>> accesses can be audited in an IMA namespace and also be evaluated against
>> the IMA policies of parent IMA namespaces.
>>
>> __process_measurement() returns either 0 or -EACCES. For hierarchical
>> processing remember the -EACCES returned by this function but continue
>> to the parent user namespace. At the end either return 0 or -EACCES
>> if an error occurred in one of the IMA namespaces.
>>
>> Currently the ima_ns pointer of the user_namespace is always NULL except
>> at the init_user_ns, so test ima_ns for NULL pointer and skip the call to
>> __process_measurement() if it is NULL. Once IMA namespacing is fully
>> enabled, the pointer may also be NULL due to late initialization of the
>> IMA namespace.
>>
>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>> ---
>>   include/linux/ima.h               |  6 +++++
>>   security/integrity/ima/ima_main.c | 37 +++++++++++++++++++++++++++----
>>   2 files changed, 39 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/linux/ima.h b/include/linux/ima.h
>> index b6ab66a546ae..fcee2a51bb87 100644
>> --- a/include/linux/ima.h
>> +++ b/include/linux/ima.h
>> @@ -65,6 +65,12 @@ static inline const char * const *arch_get_ima_policy(void)
>>   }
>>   #endif
>>   
>> +static inline struct user_namespace
>> +*ima_ns_to_user_ns(struct ima_namespace *ns)
>> +{
>> +	return current_user_ns();
>> +}
>> +
>>   #else
>>   static inline enum hash_algo ima_get_current_hash_algo(void)
>>   {
>> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
>> index 621685d4eb95..51b0ef1cebbe 100644
>> --- a/security/integrity/ima/ima_main.c
>> +++ b/security/integrity/ima/ima_main.c
>> @@ -200,10 +200,10 @@ void ima_file_free(struct file *file)
>>   	ima_check_last_writer(iint, inode, file);
>>   }
>>   
>> -static int process_measurement(struct ima_namespace *ns,
>> -			       struct file *file, const struct cred *cred,
>> -			       u32 secid, char *buf, loff_t size, int mask,
>> -			       enum ima_hooks func)
>> +static int __process_measurement(struct ima_namespace *ns,
>> +				 struct file *file, const struct cred *cred,
>> +				 u32 secid, char *buf, loff_t size, int mask,
>> +				 enum ima_hooks func)
>>   {
>>   	struct inode *inode = file_inode(file);
>>   	struct integrity_iint_cache *iint = NULL;
>> @@ -395,6 +395,35 @@ static int process_measurement(struct ima_namespace *ns,
>>   	return 0;
>>   }
>>   
>> +static int process_measurement(struct ima_namespace *ns,
>> +			       struct file *file, const struct cred *cred,
>> +			       u32 secid, char *buf, loff_t size, int mask,
>> +			       enum ima_hooks func)
>> +{
>> +	struct user_namespace *user_ns = ima_ns_to_user_ns(ns);
>> +	int ret = 0;
>> +
>> +	while (user_ns) {
>> +		ns = ima_ns_from_user_ns(user_ns);
>> +		if (ns) {
>> +			int rc;
>> +
>> +			rc = __process_measurement(ns, file, cred, secid, buf,
>> +						   size, mask, func);
>> +			switch (rc) {
>> +			case -EACCES:
>> +				/* return this error at the end but continue */
>> +				ret = -EACCES;
>> +				break;
> This seems risky. Every error not -EACCES will be counted as a success.
> It doesn't look like __process_measurement() will return anything else
> but I would still place a WARN_ON() or WARN_ON_ONCE() in there to make
> that assumption explicit.
>
> Right now it looks like your only error condition is -EACCES and non-ima
> cracks like me need to read through __process_measurement() to figure
> out that that's ok. With a WARN_ON* in there I'd not have needed to bother.
>
> switch (rc) {
> case -EACCES:
> 	/* return this error at the end but continue */
> 	ret = -EACCES;
> 	break
> default:
> 	WARN_ON_ONCE(true);
              ret = -EINVAL;
> }
>
> or sm similar.


Agreed. To be on the safe side I would add a ret = -EINVAL to it for the 
unhandled case as shown above.