Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <1f83ec246cb6356c340b379ab00e43f0b5bba0ae.camel@linux.ibm.com>
Subject: Re: [PATCH v5 3/7] IMA: add hook to measure critical data
From:   Mimi Zohar <zohar@linux.ibm.com>
To:     Tushar Sugandhi <tusharsu@linux.microsoft.com>,
        stephen.smalley.work@gmail.com, casey@schaufler-ca.com,
        agk@redhat.com, snitzer@redhat.com, gmazyland@gmail.com,
        paul@paul-moore.com
Cc:     tyhicks@linux.microsoft.com, sashal@kernel.org, jmorris@namei.org,
        nramas@linux.microsoft.com, linux-integrity@vger.kernel.org,
        selinux@vger.kernel.org, linux-security-module@vger.kernel.org,
        linux-kernel@vger.kernel.org, dm-devel@redhat.com
Date:   Fri, 06 Nov 2020 08:24:18 -0500
In-Reply-To: <20201101222626.6111-4-tusharsu@linux.microsoft.com>
References: <20201101222626.6111-1-tusharsu@linux.microsoft.com>
         <20201101222626.6111-4-tusharsu@linux.microsoft.com>
Content-Type: text/plain; charset="ISO-8859-15"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Tushar,

On Sun, 2020-11-01 at 14:26 -0800, Tushar Sugandhi wrote:
> Currently, IMA does not provide a generic function for kernel subsystems
> to measure their critical data. Examples of critical data in this context
> could be kernel in-memory r/o structures, hash of the memory structures,
> or data that represents a linux kernel subsystem state change. The 
> critical data, if accidentally or maliciously altered, can compromise
> the integrity of the system.

Start out with what IMA does do (e.g. measures files and more recently
buffer data).  Afterwards continue with kernel integrity critical data
should also be measured.  Please include a definition of kernel
integrity critical data here, as well as in the cover letter.

> 
> A generic function provided by IMA to measure critical data would enable
> various subsystems with easier and faster on-boarding to use IMA
> infrastructure and would also avoid code duplication.

By definition LSM and IMA hooks are generic with callers in appropriate
places in the kernel.   This paragraph is redundant.

> 
> Add a new IMA func CRITICAL_DATA and a corresponding IMA hook
> ima_measure_critical_data() to support measuring critical data for 
> various kernel subsystems. 

Instead of using the word "add", it would be more appropriate to use
the word "define".   Define a new IMA hook named
ima_measure_critical_data to measure kernel integrity critical data.  
Please also update the Subject line as well.  "ima: define an IMA hook
to measure kernel integrity critical data".

> 
> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
> ---
> 
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 4485d87c0aa5..6e1b11dcba53 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -921,6 +921,44 @@ void ima_kexec_cmdline(int kernel_fd, const void *buf, int size)
>  	fdput(f);
>  }
>  
> +/**
> + * ima_measure_critical_data - measure kernel subsystem data
> + * critical to integrity of the kernel

Please change this to "measure kernel integrity critical data".

> + * @event_data_source: name of the data source being measured;
> + * typically it should be the name of the kernel subsystem that is sending
> + * the data for measurement

Including "data_source" here isn't quite right.  "data source" should
only be added in the first patch which uses it, not here.   When adding
it please shorten the field description to "kernel data source".   The
longer explanation can be included in the longer function description.

> + * @event_name: name of an event from the kernel subsystem that is sending
> + * the data for measurement

As this is being passed to process_buffer_measurement(), this should be
the same or similar to the existing definition.

> + * @buf: pointer to buffer containing data to measure
> + * @buf_len: length of buffer(in bytes)
> + * @measure_buf_hash: if set to true - will measure hash of the buf,
> + *                    instead of buf

 kernel doc requires a single line.  In this case, please shorten the
argument definition to "measure buffer data or buffer data hash".   The
details can be included in the longer function description.

> + *
> + * A given kernel subsystem (event_data_source) may send
> + * data (buf) to be measured when the data or the subsystem state changes.
> + * The state/data change can be described by event_name.
> + * Examples of critical data (buf) could be kernel in-memory r/o structures,
> + * hash of the memory structures, or data that represents subsystem
> + * state change.
> + * measure_buf_hash can be used to save space, if the data being measured
> + * is too large.
> + * The data (buf) can only be measured, not appraised.
> + */

Please remove this longer function description, replacing it something
more appropriate.  The subsequent patch that introduces the "data
source" parameter would expand the description.

thanks,

Mimi

> +void ima_measure_critical_data(const char *event_data_source,
> +			       const char *event_name,
> +			       const void *buf, int buf_len,
> +			       bool measure_buf_hash)
> +{
> +	if (!event_name || !event_data_source || !buf || !buf_len) {
> +		pr_err("Invalid arguments passed to %s().\n", __func__);
> +		return;
> +	}
> +
> +	process_buffer_measurement(NULL, buf, buf_len, event_name,
> +				   CRITICAL_DATA, 0, event_data_source,
> +				   measure_buf_hash);
> +}
> +
>  static int __init init_ima(void)
>  {
>  	int error;