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

From: Mimi Zohar <zohar@linux.ibm.com>
To: Stefan Berger <stefanb@linux.ibm.com>,
	Christian Brauner <brauner@kernel.org>,
	Stefan Berger <stefanb@linux.vnet.ibm.com>
Cc: linux-integrity@vger.kernel.org, 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
Subject: Re: [PATCH v9 08/23] ima: Move measurement list related variables into ima_namespace
Date: Thu, 27 Jan 2022 16:48:01 -0500	[thread overview]
Message-ID: <fc0fda8455140dce4f5af25fa8deef66ba931a95.camel@linux.ibm.com> (raw)
In-Reply-To: <9ca21852-17ee-fc99-4764-300cd5199810@linux.ibm.com>

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.

> 
> 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?

thanks,

Mimi