Re: [RFC PATCH] ima: verify mprotect change is consistent with mmap policy

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

On Mon, 2020-05-04 at 15:51 -0700, Lakshmi Ramasubramanian wrote:
> On 5/4/20 2:17 PM, Mimi Zohar wrote:
> 
> Hi Mimi,
> 
> > +int ima_file_mprotect(struct vm_area_struct *vma, unsigned long prot)
> > +{
> > +	struct ima_template_desc *template;
> > +	struct inode *inode;
> > +	int result = 0;
> > +	int action;
> > +	u32 secid;
> > +	int pcr;
> > +
> > +	if (vma->vm_file && (prot & PROT_EXEC) && !(vma->vm_flags & VM_EXEC)) {
> 
> Just a suggestion:
> Maybe you could do the negative of the above check and return, so that 
> the block within the if statement doesn't have to be indented.

Good suggestion.

> 
> > +		inode = file_inode(vma->vm_file);
> > +
> > +		security_task_getsecid(current, &secid);
> > +		action = ima_get_action(inode, current_cred(), secid, MAY_EXEC,
> > +					MMAP_CHECK, &pcr, &template, 0);
> > +
> > +		if (action & IMA_APPRAISE_SUBMASK)
> > +			result = -EPERM;
> > +
> > +		if ((action & IMA_APPRAISE_SUBMASK) || (action & IMA_MEASURE)) {
> 
> action is checked for IMA_APPRAISE_SUBMASK bits in the previous if 
> statement. Does it need to be checked again in the above if statement?

Agreed, the code should be cleaned up here too.  In either the
measurement or the appraisal case, mprotect modifying the execute mmap
flag should be audited, but only in the appraisal case is the request
denied.

Mimi

> 
> > +			struct file *file = vma->vm_file;
> > +			char *pathbuf = NULL;
> > +			const char *pathname;
> > +			char filename[NAME_MAX];
> > +
> > +			pathname = ima_d_path(&file->f_path, &pathbuf,
> > +					      filename);
> > +			integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode,
> > +					    pathname, "collect_data",
> > +					    "failed-mprotect", result, 0);
> > +
> > +			if (pathbuf)
> > +				__putname(pathbuf);
> > +		}
> > +	}
> > +	return result;
> > +}
> 
> thanks,
>   -lakshmi
>