Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH 1/5 v4] added a new ima policy func buffer_check, and
 ima hook to measure the buffer hash into ima
From:   Mimi Zohar <zohar@linux.ibm.com>
To:     Prakhar Srivastava <prsriva02@gmail.com>,
        linux-integrity@vger.kernel.org,
        linux-secuirty-module@vger.kernel.org, linux-kernel@vger.kernel.org
Cc:     ebiederm@xmission.com, vgoyal@redhat.com, nayna@linux.ibm.com,
        nramas@microsoft.com, prsriva@microsoft.com
Date:   Mon, 06 May 2019 08:13:17 -0400
In-Reply-To: <20190503222523.6294-2-prsriva02@gmail.com>
References: <20190503222523.6294-1-prsriva02@gmail.com>
         <20190503222523.6294-2-prsriva02@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 8bit
Message-Id: <1557144797.14288.93.camel@linux.ibm.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, 2019-05-03 at 15:25 -0700, Prakhar Srivastava wrote:
> From: Prakhar Srivastava <prsriva02@gmail.com>
> 
> This change adds a new ima policy func buffer_check, and ima hook to
>  measure the buffer hash into ima log.

This patch description says "what" you're during without the
motivation.  Please write an appropriate patch description, starting
with some background information.  Refer to section "2) Describe your
changes" of Documentation/process/submitting-patches.rst.

<snip>

> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 357edd140c09..3db3f3966ac7 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -576,6 +576,95 @@ int ima_load_data(enum kernel_load_data_id id)
>  	return 0;
>  }
>  
> +/*
> + * process_buffer_measurement - Measure the buffer passed to ima log.
> + * (Instead of using the file hash the buffer hash is used).
> + * @buff - The buffer that needs to be added to the log

"buf" with a single 'f' is the commonly used variable name.

> + * @size - size of buffer(in bytes)
> + * @eventname - this is eventname used for the various buffers
> + * that can be measured.

This patch set introduces measuring the boot command line.  There
aren't any other buffers being measured.  Please remove the reference
to other buffers.

> + *
> + * The buffer passed is added to the ima logs.
> + * If the sig template is used, then the sig field contains the buffer.

It doesn't look like this particular patch adds the boot command line
string to the measurement list sig field.  Please remove this comment.

I've previously said you can't overload the sig field for storing the
boot command line string.  Please define a new template field.

> + *
> + * On success return 0.
> + * On error cases surface errors from ima calls.
> + */
> +static int process_buffer_measurement(const void *buff, int size,
> +				const char *eventname, const struct cred *cred,
> +				u32 secid)
> +{
> +	int ret = -EINVAL;
> +	struct ima_template_entry *entry = NULL;
> +	struct integrity_iint_cache tmp_iint, *iint = &tmp_iint;
> +	struct ima_event_data event_data = {iint, NULL, NULL,
> +					    NULL, 0, NULL};
> +	struct {
> +		struct ima_digest_data hdr;
> +		char digest[IMA_MAX_DIGEST_SIZE];
> +	} hash;
> +	int violation = 0;
> +	int pcr = CONFIG_IMA_MEASURE_PCR_IDX;
> +
> +	if (!buff || size ==  0 || !eventname)
> +		goto err_out;

There is only one caller to this function.  This can never happen.
 Please remove this test.

> +
> +	if (ima_get_action(NULL, cred, secid, 0, BUFFER_CHECK, &pcr)
> +		!= IMA_MEASURE)
> +		goto err_out;

The IMA policy defines what should and shouldn't be measured.  Not
having a rule to measure the boot command line can not be considered
an error.

> +
> +	memset(iint, 0, sizeof(*iint));
> +	memset(&hash, 0, sizeof(hash));
> +
> +	event_data.filename = eventname;
> +
> +	iint->ima_hash = &hash.hdr;
> +	iint->ima_hash->algo = ima_hash_algo;
> +	iint->ima_hash->length = hash_digest_size[ima_hash_algo];
> +
> +	ret = ima_calc_buffer_hash(buff, size, iint->ima_hash);
> +	if (ret < 0)
> +		goto err_out;
> +
> +	ret = ima_alloc_init_template(&event_data, &entry);
> +	if (ret < 0)
> +		goto err_out;
> +
> +	ret = ima_store_template(entry, violation, NULL,
> +					buff, pcr);
> +	if (ret < 0) {
> +		ima_free_template_entry(entry);
> +		goto err_out;
> +	}
> +
> +	return 0;
> +
> +err_out:
> +	pr_err("Error in adding buffer measure: %d\n", ret);
> +	return ret;

Please remove.


> +}
> +
> +/**
> + * ima_buffer_check - based on policy, collect & store buffer measurement
> + * @buf: pointer to buffer
> + * @size: size of buffer
> + * @eventname: event name identifier
> + *
> + * Buffers can only be measured, not appraised.  The buffer identifier
> + * is used as the measurement list entry name (eg. boot_cmdline).
> + */
> +void ima_buffer_check(const void *buf, int size, const char *eventname)
> +{
> +	u32 secid;
> +
> +	if (buf && size != 0 && eventname) {
> +		security_task_getsecid(current, &secid);
> +		process_buffer_measurement(buf, size, eventname,
> +				current_cred(), secid);
> +	}
> +}
> +
> +
>  static int __init init_ima(void)
>  {
>  	int error;
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index e0cc323f948f..b12551ed191c 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -291,6 +291,12 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
>  {
>  	int i;
>  
> +	// Incase of BUFFER_CHECK, Inode is NULL

Comments use the /* ... */ syntax.  Please refer to section "8)
Commenting" of Documentation/process/coding-style.rst.

Mimi

> +	if (!inode) {
> +		if ((rule->flags & IMA_FUNC) && (rule->func == func))
> +			return true;
> +		return false;
> +	}
>  	if ((rule->flags & IMA_FUNC) &&
>  	    (rule->func != func && func != POST_SETATTR))
>  		return false;