Re: [PATCH v5 2/7] IMA: update process_buffer_measurement to measure buffer hash

[Tushar Sugandhi]

On 2020-11-06 4:11 a.m., Mimi Zohar wrote:
> Hi Tushar,
>
> Below inline are a few additional comments.
>
> > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> > index ae5da9f3339d..4485d87c0aa5 100644
> > --- a/security/integrity/ima/ima_main.c
> > +++ b/security/integrity/ima/ima_main.c
> > @@ -787,12 +787,15 @@ int ima_post_load_data(char *buf, loff_t size,
> >    * @func: IMA hook
> >    * @pcr: pcr to extend the measurement
> >    * @func_data: private data specific to @func, can be NULL.
> > + * @measure_buf_hash: if set to true - will measure hash of the buf,
> > + *                    instead of buf
> >    *
> >    * Based on policy, the buffer is measured into the ima log.
>
> Both the brief and longer function descriptions need to be updated, as
> well as the last argument description.  The last argument should be
> limited to "measure buffer hash".  How it is used could be included in
> the longer function description.  The longer function description would
> include adding the buffer data or the buffer data hash to the IMA
> measurement list and extending the PCR.
>
> For example,
> process_buffer_measurement - measure the buffer data or the buffer data
> hash
Thanks Mimi. Will update the brief and longer descriptions accordingly.
> >    */
> >   void process_buffer_measurement(struct inode *inode, const void *buf, int size,
> >   				const char *eventname, enum ima_hooks func,
> > -				int pcr, const char *func_data)
> > +				int pcr, const char *func_data,
> > +				bool measure_buf_hash)
> >   {
> >   	int ret = 0;
> >   	const char *audit_cause = "ENOMEM";
> > @@ -807,6 +810,8 @@ void process_buffer_measurement(struct inode *inode, const void *buf, int size,
> >   		struct ima_digest_data hdr;
> >   		char digest[IMA_MAX_DIGEST_SIZE];
> >   	} hash = {};
> > +	char digest_hash[IMA_MAX_DIGEST_SIZE];
> > +	int hash_len = hash_digest_size[ima_hash_algo];
> >   	int violation = 0;
> >   	int action = 0;
> >   	u32 secid;
> > @@ -855,6 +860,21 @@ void process_buffer_measurement(struct inode *inode, const void *buf, int size,
> >   		goto out;
> >   	}
> >   
> > +	if (measure_buf_hash) {
> > +		memcpy(digest_hash, hash.hdr.digest, hash_len);
>
> Instead of digest_hash and hash_len, consider naming the variables
> buf_hash and buf_hashlen.
Thanks. Will do.
> > +
> > +		ret = ima_calc_buffer_hash(digest_hash,
> > +					   hash_len,
> > +					   iint.ima_hash);
>
> There's no need for each variable to be on a separate line.
Thanks, will fix.
~Tushar

> thanks,
>
> Mimi
>
> > +		if (ret < 0) {
> > +			audit_cause = "measure_buf_hash_error";
> > +			goto out;
> > +		}
> > +
> > +		event_data.buf = digest_hash;
> > +		event_data.buf_len = hash_len;
> > +	}
> > +
> >   	ret = ima_alloc_init_template(&event_data, &entry, template);
> >   	if (ret < 0) {
> >   		audit_cause = "alloc_entry";