Re: [PATCH v2] ima: drop vla in ima_audit_measurement()

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

On Thu, 2018-03-08 at 15:15 -0700, Tycho Andersen wrote:
> Hi Mimi,
> 
> On Thu, Mar 08, 2018 at 05:05:40PM -0500, Mimi Zohar wrote:
> > On Thu, 2018-03-08 at 14:45 -0700, Tycho Andersen wrote:
> > > Hi Mimi,
> > > 
> > > On Thu, Mar 08, 2018 at 03:36:14PM -0500, Mimi Zohar wrote:
> > > > On Thu, 2018-03-08 at 13:23 -0700, Tycho Andersen wrote:
> > > > 
> > > > >  /*
> > > > > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> > > > > index 2cfb0c714967..356faae6f09c 100644
> > > > > --- a/security/integrity/ima/ima_main.c
> > > > > +++ b/security/integrity/ima/ima_main.c
> > > > > @@ -288,8 +288,11 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
> > > > >  					      xattr_value, xattr_len, opened);
> > > > >  		inode_unlock(inode);
> > > > >  	}
> > > > > -	if (action & IMA_AUDIT)
> > > > > -		ima_audit_measurement(iint, pathname);
> > > > > +	if (action & IMA_AUDIT) {
> > > > > +		rc = ima_audit_measurement(iint, pathname);
> > > > > +		if (rc < 0)
> > > > > +			goto out_locked;
> > > > > +	}
> > > > > 
> > > > >  	if ((file->f_flags & O_DIRECT) && (iint->flags & IMA_PERMIT_DIRECTIO))
> > > > >  		rc = 0;
> > > > 
> > > > Only when IMA-appraisal is enforcing file data integrity should
> > > > process_measurement() ever fail.  Other errors can be logged/audited.
> > > 
> > > Ok, so previously in ima_audit_measurement() when allocation failed,
> > > there was nothing logged. If we just keep this behavior like below,
> > > does that look good?
> > 
> > Before the IMA locking change that were just upstreamed, there were
> > problems with measuring/appraising files that were opened with the
> > O_DIRECT flag.  Unless the IMA policy specified permit_directio, the
> > measurement/appraisal failed.  With the new locking, opening files
> > with the O_DIRECTIO flag shouldn't be a problem.  It just needs to be
> > fully tested before removing this code.
> > 
> > On failure, the code below tests the ima_audit_measurement() result
> > and skips the IMA_PERMIT_DIRECTIO test.  Unless I'm missing something,
> > I don't see the point.
> 
> It skips the IMA_PERMIT_DIRECTIO test because it's already going to
> fail: we're in enforce mode and we got an allocation failure and so we
> can't audit this access (note: there is another allocation failure in
> ima_audit_measurement() which is still ignored after this patch, so
> maybe ignoring failures is ok; seems like it's not, though

By the time we get here, we've already verified the file's integrity,
if it is in policy.  At this point, we're attempting to add the file
hash to the audit log.  If for some reason the audit fails, there's
not much we can do.

> I'm not sure I really understand the rest of your message though. Can
> you suggest what the patch should do here? Should we just ignore all
> failures as before?

I would leave it as it is.

Mimi