Re: [PATCH v9 08/23] ima: Move measurement list related variables into ima_namespace

[Stefan Berger <stefanb@linux.ibm.com>]

On 1/27/22 16:48, Mimi Zohar wrote:
> On Wed, 2022-01-26 at 17:23 -0500, Stefan Berger wrote:
>> On 1/26/22 04:21, Christian Brauner wrote:
>>> On Tue, Jan 25, 2022 at 05:46:30PM -0500, Stefan Berger wrote:
>>>> From: Stefan Berger <stefanb@linux.ibm.com>
>>>>
>>>> Move measurement list related variables into the ima_namespace. This way
>>>> a front-end like securityfs can show the measurement list inside an IMA
>>>> namespace.
>>>>
>>>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>>>> ---
>>>>    security/integrity/ima/ima.h             |  5 +++--
>>>>    security/integrity/ima/ima_fs.c          |  6 ++++--
>>>>    security/integrity/ima/ima_init_ima_ns.c |  5 +++++
>>>>    security/integrity/ima/ima_kexec.c       | 12 ++++++-----
>>>>    security/integrity/ima/ima_queue.c       | 27 +++++++++++-------------
>>>>    5 files changed, 31 insertions(+), 24 deletions(-)
>>>>
>>>> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
>>>> index 340a59174670..45706836a77b 100644
>>>> --- a/security/integrity/ima/ima.h
>>>> +++ b/security/integrity/ima/ima.h
>>>> @@ -106,7 +106,6 @@ struct ima_queue_entry {
>>>>    	struct list_head later;		/* place in ima_measurements list */
>>>>    	struct ima_template_entry *entry;
>>>>    };
>>>> -extern struct list_head ima_measurements;	/* list of all measurements */
>>>>    
>>>>    /* Some details preceding the binary serialized measurement list */
>>>>    struct ima_kexec_hdr {
>>>> @@ -136,6 +135,8 @@ struct ima_namespace {
>>>>    	struct ima_rule_entry *arch_policy_entry;
>>>>    
>>>>    	struct ima_h_table ima_htable;
>>>> +	struct list_head ima_measurements;	/* list of all measurements */
>>>> +	unsigned long binary_runtime_size;	/* used by init_ima_ns */
>>>>    } __randomize_layout;
>>> Moving this into struct imans seems sane to me but I'm not going to ack
>>> it because I don't have enough knowledge to guarantee that this code
>>> will only run for init_ima_ns. I'll leave that to Mimi.
> Moving the ima_measurements to the ima_namespace is needed for
> namespacing the IMA measurement list (next stage).  I think moving it
> now is fine.
And besides that we can now already how an empty measurement list 
without having to special case the code.
>
>> The code modifying binary_runtime_size may do this for all IMA
>> namespaces but the esulting value of binary_runtime_size may only
>> matter in init_ima_ns (not 100% sure, but Mimi seems to say so). Moving
>> it into ima_namespace rather than special-casing the code keeps the code
>> readable.
> Right, there are other changes like moving the binary_runtime_size to
> the namespace, which aren't needed, but are being done, I guess, for
> aesthetics.  The binary_runtime_size is only used for allocating the
> memory needed for carrying the measurement list across kexec.  Anything
> related to carrying or restoring the measurement list across kexec is
> limited to ima_init_ns.
>
>> There are also some case in the code that may do something like this:
>>
>> if (ns == &init_ima_ns)
>>
>>       foo = xyz;
>>
>> Those will go away when foo is moved into the namespace and then it
>> becomes ns->foo = xyz, which is much saner for readability but
>> unavoidable for some variables at this stage.
> Since binary_runtime_size is limited to the ima_init_ns, should it be
> conditional?

Moving it into ima_namespace keeps the code clean from these kind of 
constructs above. I would move it... The less of these constructs are 
there the less confusing it is, IMO. But ymmv.



>
> thanks,
>
> Mimi
>