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

Stefan Berger
2022-01-28T14:25:30Z

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
>