Re: [PATCH] IMA: Handle early boot data measurement

From: Mimi Zohar <zohar@linux.ibm.com>
To: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>,
	stephen.smalley.work@gmail.com, casey@schaufler-ca.com
Cc: tyhicks@linux.microsoft.com, tusharsu@linux.microsoft.com,
	sashal@kernel.org, jmorris@namei.org,
	linux-integrity@vger.kernel.org, selinux@vger.kernel.org,
	linux-security-module@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] IMA: Handle early boot data measurement
Date: Tue, 25 Aug 2020 13:42:13 -0400	[thread overview]
Message-ID: <49f8a616d80344c539b512f8b83590ea281ee54d.camel@linux.ibm.com> (raw)
In-Reply-To: <307617de-b42d-ac52-6e9e-9e0d16bbc20e@linux.microsoft.com>

On Tue, 2020-08-25 at 08:46 -0700, Lakshmi Ramasubramanian wrote:
> On 8/25/20 8:40 AM, Mimi Zohar wrote:
> > On Fri, 2020-08-21 at 16:12 -0700, Lakshmi Ramasubramanian wrote:
> > > The current implementation of early boot measurement in
> > > the IMA subsystem is very specific to asymmetric keys. It does not
> > > handle early boot measurement of data from other subsystems such as
> > > Linux Security Module (LSM), Device-Mapper, etc. As a result data,
> > > provided by these subsystems during system boot are not measured by IMA.
> > > 
> > > Update the early boot key measurement to handle any early boot data.
> > > Refactor the code from ima_queue_keys.c to a new file ima_queue_data.c.
> > > Rename the kernel configuration CONFIG_IMA_QUEUE_EARLY_BOOT_KEYS to
> > > CONFIG_IMA_QUEUE_EARLY_BOOT_DATA so it can be used for enabling any
> > > early boot data measurement. Since measurement of asymmetric keys is
> > > the first consumer of early boot measurement, this kernel configuration
> > > is enabled if IMA_MEASURE_ASYMMETRIC_KEYS and SYSTEM_TRUSTED_KEYRING are
> > > both enabled.
> > > 
> > > Update the IMA hook ima_measure_critical_data() to utilize early boot
> > > measurement support.
> > 
> > Please limit the changes in this patch to renaming the functions and/or
> > files.  For example, adding "measure_payload_hash" should be a separate
> > patch, not hidden here.
> > 
> 
> Thanks for the feedback Mimi.
> 
> I'll split this into 2 patches:
> 
> PATCH 1: Rename files + rename CONFIG
> PATCH 2: Update IMA hook to utilize early boot data measurement.

I'm referring to introducing the "measure_payload_hash" flag.  I assume
this is to indicate whether the buffer should be hashed or not.  

Example 1: ima_alloc_key_entry() and ima_alloc_data_entry(0 comparison
> -static struct ima_key_entry *ima_alloc_key_entry(struct key *keyring,
> -                                                const void *payload,
> -                                                size_t payload_len)
> -{


> +static struct ima_data_entry *ima_alloc_data_entry(const char *event_name,
> +                                                  const void *payload,
> +                                                  size_t payload_len,
> +                                                  const char *event_data,
> +                                                  enum ima_hooks func,
> +                                                  bool measure_payload_hash)  <====
> +{

Example 2:

diff --git a/security/integrity/ima/ima_asymmetric_keys.c b/security/integrity/ima/ima_asymmetric_keys.c
index a74095793936..65423754765f 100644
--- a/security/integrity/ima/ima_asymmetric_keys.c
+++ b/security/integrity/ima/ima_asymmetric_keys.c
@@ -37,9 +37,10 @@ void ima_post_key_create_or_update(struct key *keyring, struct key *key,
        if (!payload || (payload_len == 0))
                return;
 
-       if (ima_should_queue_key())
-               queued = ima_queue_key(keyring, payload, payload_len);
-
+       if (ima_should_queue_data())
+               queued = ima_queue_data(keyring->description, payload,
+                                       payload_len, keyring->description,
+                                       KEY_CHECK, false);   <===
        if (queued)
                return;

But in general, as much as possible function and file name changes
should be done independently of other changes.

thanks,

Mimi

