Re: [PATCH v5 1/4] ima: always measure and audit files in policy

[Mimi Zohar <zohar@linux.vnet.ibm.com>]

On Fri, 2017-08-11 at 12:18 +0200, Christoph Hellwig wrote:
> > +	i_version = file_inode(file)->i_version;
> 
> This probably wants a comment that i_version might be unreliable
> unless the file system supports the change attribute.
> 
> > +	result = (!buf) ?  ima_calc_file_hash(file, &hash.hdr) :
> > +		ima_calc_buffer_hash(buf, size, &hash.hdr);
> 
> Please write this like proper C code:
> 
> 	if (buf)
> 		result = ima_calc_buffer_hash(buf, size, &hash.hdr);
> 	else
> 		result = ima_calc_file_hash(file, &hash.hdr);

Sure

> 
> > +++ b/security/integrity/ima/ima_crypto.c
> > @@ -441,6 +441,16 @@ int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
> >  	loff_t i_size;
> >  	int rc;
> >  
> > +	/*
> > +	 * O_DIRECT not supported for buffered read. For consistency,
> > +	 * don't support O_DIRECT on DAX either.
> > +	 */
> 
> I can't parse this - O_DIRECT is the opposite of a buffered I/O, including
> reads.

Right. I'm trying to differentiate between a file opened with the
O_DIRECT flag on a filesystem mounted with/without DAX.  For
consistency, it was recommended to fail both.

> 
> > +	if ((rc == 0) && (action & IMA_APPRAISE_SUBMASK))
> 
> no need for the first set of inner braces.

thanks,

Mimi