On Thu, 2015-12-17 at 23:03 +0100, Luis R. Rodriguez wrote:
> On Tue, Dec 08, 2015 at 01:01:23PM -0500, Mimi Zohar wrote:
> > diff --git a/security/integrity/iint.c b/security/integrity/iint.c
> > index 8a45576..4d149c9 100644
> > --- a/security/integrity/iint.c
> > +++ b/security/integrity/iint.c
> > @@ -222,6 +223,11 @@ int integrity_read_file(const char *path, char **data)
> >             return rc;
> >     }
> >  
> > +   if (!S_ISREG(file_inode(file)->i_mode)) {
> > +           rc = -EACCES;
> > +           goto out;
> > +   }
> > +
> >     size = i_size_read(file_inode(file));
> >     if (size <= 0)
> >             goto out;
> 
> This hunk seems to be unrelated to this patch? If so can it be split out?

Yes, sure.   Up to now, 'cat' was used to load the IMA policy.   A lot
of the problems related to opening and reading a file were hidden.  So
besides making sure that only a regular file is opened, what other
things should we be checking?   For example,  do we permit the kernel to
read NFS mounted files?   Should the kernel be limited to opening only
local files?   Answering these questions becomes important as we move to
a single kernel file read function.

> > @@ -232,13 +238,18 @@ int integrity_read_file(const char *path, char **data)
> >             goto out;
> >     }
> >  
> > -   rc = integrity_kernel_read(file, 0, buf, size);
> > +   rc = ima_read_and_process_file(file, read_func, buf, size);
> > +   if (rc == -EOPNOTSUPP) {
> > +           rc = integrity_kernel_read(file, 0, buf, size);
> > +           if (rc > 0 && rc != size)
> > +                   rc = -EIO;
> > +   }
> >     if (rc < 0)
> >             kfree(buf);
> > -   else if (rc != size)
> > -           rc = -EIO;
> > -   else
> > +   else {
> > +           rc = size;
> >             *data = buf;
> > +   }
> >  out:
> >     fput(file);
> >     return rc;
> > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> > index 548b258..40a24c3 100644
> > --- a/security/integrity/ima/ima.h
> > +++ b/security/integrity/ima/ima.h
> > @@ -180,6 +180,7 @@ int ima_policy_show(struct seq_file *m, void *v);
> >  #define IMA_APPRAISE_LOG   0x04
> >  #define IMA_APPRAISE_MODULES       0x08
> >  #define IMA_APPRAISE_FIRMWARE      0x10
> > +#define IMA_APPRAISE_POLICY        0x20
> >  
> >  #ifdef CONFIG_IMA_APPRAISE
> >  int ima_appraise_measurement(int func, struct integrity_iint_cache *iint,
> > diff --git a/security/integrity/ima/ima_appraise.c 
> > b/security/integrity/ima/ima_appraise.c
> > index b83049b..1e1a759 100644
> > --- a/security/integrity/ima/ima_appraise.c
> > +++ b/security/integrity/ima/ima_appraise.c
> > @@ -79,6 +79,7 @@ enum integrity_status ima_get_cache_status(struct 
> > integrity_iint_cache *iint,
> >     case FIRMWARE_CHECK:
> >     case KEXEC_CHECK:
> >     case INITRAMFS_CHECK:
> > +   case POLICY_CHECK:
> >             return iint->ima_read_status;
> >     case FILE_CHECK:
> >     default:
> 
> Hrm this uses an int for the func.
> 
> > @@ -102,6 +103,7 @@ static void ima_set_cache_status(struct 
> > integrity_iint_cache *iint,
> >     case FIRMWARE_CHECK:
> >     case KEXEC_CHECK:
> >     case INITRAMFS_CHECK:
> > +   case POLICY_CHECK:
> >             iint->ima_read_status = status;
> >             break;
> >     case FILE_CHECK:
> 
> This uses an enum.
> 
> > @@ -126,6 +128,7 @@ static void ima_cache_flags(struct integrity_iint_cache 
> > *iint, int func)
> >     case FIRMWARE_CHECK:
> >     case KEXEC_CHECK:
> >     case INITRAMFS_CHECK:
> > +   case POLICY_CHECK:
> >             iint->flags |= (IMA_READ_APPRAISED | IMA_APPRAISED);
> >             break;
> >     case FILE_CHECK:
> 
> This uses an enum.
> 
> All of these have a common set of funcs that will do similar things, what 
> about
> just OR'ing them up in one place? That would make future additions one line
> instead of 3.

Ok

Mimi

--
To unsubscribe from this list: send the line "unsubscribe 
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to