Re: [PATCH v8 10/19] ima: Implement hierarchical processing of file accesses

Stefan Berger
2022-01-18T18:25:55Z

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.