linux-kernel - Re: [PATCH v8 03/19] ima: Move policy related variables into ima

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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Wed, 19 Jan 2022 08:32:24 -0500
From:   Stefan Berger <stefanb@...ux.ibm.com>
To:     Mimi Zohar <zohar@...ux.ibm.com>,
        Stefan Berger <stefanb@...ux.vnet.ibm.com>,
        linux-integrity@...r.kernel.org
Cc:     serge@...lyn.com, christian.brauner@...ntu.com,
        containers@...ts.linux.dev, dmitry.kasatkin@...il.com,
        ebiederm@...ssion.com, krzysztof.struczynski@...wei.com,
        roberto.sassu@...wei.com, mpeters@...hat.com, lhinds@...hat.com,
        lsturman@...hat.com, puiterwi@...hat.com, jejb@...ux.ibm.com,
        jamjoom@...ibm.com, linux-kernel@...r.kernel.org,
        paul@...l-moore.com, rgb@...hat.com,
        linux-security-module@...r.kernel.org, jmorris@...ei.org
Subject: Re: [PATCH v8 03/19] ima: Move policy related variables into
 ima_namespace


On 1/13/22 15:26, Mimi Zohar wrote:
> Hi Stefan,
>
> On Tue, 2022-01-04 at 12:04 -0500, Stefan Berger wrote:
>> From: Stefan Berger <stefanb@...ux.ibm.com>
>>
>> Move variables related to the IMA policy into the ima_namespace. This way
>> the IMA policy of an IMA namespace can be set and displayed using a
>> front-end like SecurityFS.
>>
>> Implement ima_ns_from_file() to get the IMA namespace via the user
>> namespace of the SecurityFS superblock that a file belongs to.
>>
>> To get the current ima_namespace use get_current_ns() when a function
>> that is related to a policy rule is called. In other cases where functions
>> are called due file attribute modifications, use init_ima_ns, since these
>> functions are related to IMA appraisal and changes to file attributes are
>> only relevant to the init_ima_ns until IMA namespaces also support IMA
>> appraisal. In ima_file_free() use init_ima_ns since in this case flags
>> related to file measurements may be affected, which is not supported in
>> IMA namespaces, yet.
>>
>> Signed-off-by: Stefan Berger <stefanb@...ux.ibm.com>
> Please split this patch into "ima: pass through ima namespace", or some
> other name,  and "ima: Move policy related variables into

What is supposed to happen in the 'ima: pass through ima namespace' 
patch? What part of this patch do you expect to see there and what do 
you want to see deferred to a 2nd patch?


> ima_namespace".  The other option is to combine the "pass through ima
> namespace" with the 2nd patch, like Christian's example.

You mean I should merge this patch with the 2nd?

I am a bit confused as to what you are proposing. The first option would 
create 2 patches out of this one, the 2nd option would merge this one 
with the 2nd patch.

The equivalent of Christian's 2nd patch would be to merge this patch 
into the 2nd. So then let's do that?


>
>> ---
>>   security/integrity/ima/ima.h                 |  49 ++++---
>>   security/integrity/ima/ima_api.c             |   8 +-
>>   security/integrity/ima/ima_appraise.c        |  28 ++--
>>   security/integrity/ima/ima_asymmetric_keys.c |   4 +-
>>   security/integrity/ima/ima_fs.c              |  16 ++-
>>   security/integrity/ima/ima_init.c            |   8 +-
>>   security/integrity/ima/ima_init_ima_ns.c     |   6 +
>>   security/integrity/ima/ima_main.c            |  83 +++++++----
>>   security/integrity/ima/ima_policy.c          | 142 ++++++++++---------
>>   security/integrity/ima/ima_queue_keys.c      |  11 +-
>>   10 files changed, 213 insertions(+), 142 deletions(-)
>>
>> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
>> index c4af3275f015..0b3dc9425076 100644
>> --- a/security/integrity/ima/ima.h
>> +++ b/security/integrity/ima/ima.h
>> @@ -20,6 +20,7 @@
>>   #include <linux/hash.h>
>>   #include <linux/tpm.h>
>>   #include <linux/audit.h>
>> +#include <linux/user_namespace.h>
>>   #include <crypto/hash_info.h>
>>   
>>   #include "../integrity.h"
>> @@ -43,9 +44,6 @@ enum tpm_pcrs { TPM_PCR0 = 0, TPM_PCR8 = 8, TPM_PCR10 = 10 };
>>   
>>   #define NR_BANKS(chip) ((chip != NULL) ? chip->nr_allocated_banks : 0)
>>   
>> -/* current content of the policy */
>> -extern int ima_policy_flag;
>> -
>>   /* bitset of digests algorithms allowed in the setxattr hook */
>>   extern atomic_t ima_setxattr_allowed_hash_algorithms;
>>   
>> @@ -120,6 +118,14 @@ struct ima_kexec_hdr {
>>   };
>>   
>>   struct ima_namespace {
>> +	struct list_head ima_default_rules;
>> +	/* ns's policy rules */
> Thank you for adding comments.  Why is the ima_default_rules not
> considered "ns's policy rules"?   Will this come later or is it limited
> to init_ima_ns?
Let me move the comment up.
>
>> +	struct list_head ima_policy_rules;
>> +	struct list_head ima_temp_rules;
>> +	/* Pointer to ns's current policy */
>> +	struct list_head __rcu *ima_rules;
> Since "Pointer to ns's current policy" only refers to ima_rules, append
> it to the variable definition.


Ok, I will move it onto the same line then.


>
>> +	/* current content of the policy */
>> +	int ima_policy_flag;
> Similarly here append the comment to the variable definition.

Will do. Thanks.


>
>>   } __randomize_layout;
>>   extern struct ima_namespace init_ima_ns;
> thanks,
>
> Mimi
>
>