| Message ID | 20180308214547.kdeoeozugxffzumn@smitten (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. Mimi > Thanks! > > Tycho > > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c > index 356faae6f09c..4e699bc7adc5 100644 > --- a/security/integrity/ima/ima_main.c > +++ b/security/integrity/ima/ima_main.c > @@ -289,9 +289,13 @@ static int process_measurement(struct file *file, char *buf, loff_t size, > inode_unlock(inode); > } > if (action & IMA_AUDIT) { > - rc = ima_audit_measurement(iint, pathname); > - if (rc < 0) > + int ret; > + > + ret = ima_audit_measurement(iint, pathname); > + if (ret < 0 && ima_appraise & IMA_APPRAISE_ENFORCE) { > + rc = ret; > goto out_locked; > + } > } > > if ((file->f_flags & O_DIRECT) && (iint->flags & IMA_PERMIT_DIRECTIO)) >
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). 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? Tycho
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
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 356faae6f09c..4e699bc7adc5 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -289,9 +289,13 @@ static int process_measurement(struct file *file, char *buf, loff_t size, inode_unlock(inode); } if (action & IMA_AUDIT) { - rc = ima_audit_measurement(iint, pathname); - if (rc < 0) + int ret; + + ret = ima_audit_measurement(iint, pathname); + if (ret < 0 && ima_appraise & IMA_APPRAISE_ENFORCE) { + rc = ret; goto out_locked; + } } if ((file->f_flags & O_DIRECT) && (iint->flags & IMA_PERMIT_DIRECTIO))