Up to now, measuring files and updating the IMA open/read/write
counters associated with the file were done at the same time
in ima_path_check(). An imbalanced counter was an indication
that the file hadn't been measured. Each case needed to be
inspected, resulting in adding either a new ima_counts_get()
or ima_path_check() call (e.g. nfsd, ecryptfs, openAFS).
This patchset separates incrementing the counters from measuring
the file. However, the underlying assumption is that all regular
files are opened via do_filp_open(). Is this assumption correct or,
by incrementing the file counters separately, have we inadvertently
hidden the fact that a file wasn't measured?
Mimi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Signed-off-by: Mimi Zohar <zo...@us.ibm.com>
---
fs/namei.c | 4 +-
include/linux/ima.h | 4 +-
security/integrity/ima/ima_main.c | 234 ++++++++++++++-----------------------
3 files changed, 94 insertions(+), 148 deletions(-)
diff --git a/fs/namei.c b/fs/namei.c
index fabd939..a89ebc3 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1730,7 +1730,7 @@ do_last:
if (nd.root.mnt)
path_put(&nd.root);
if (!IS_ERR(filp)) {
- error = ima_path_check(&filp->f_path, filp->f_mode &
+ error = ima_path_check(filp, filp->f_mode &
(MAY_READ | MAY_WRITE | MAY_EXEC));
if (error) {
fput(filp);
@@ -1791,7 +1791,7 @@ ok:
}
filp = nameidata_to_filp(&nd, open_flag);
if (!IS_ERR(filp)) {
- error = ima_path_check(&filp->f_path, filp->f_mode &
+ error = ima_path_check(filp, filp->f_mode &
(MAY_READ | MAY_WRITE | MAY_EXEC));
if (error) {
fput(filp);
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 99dc6d5..aa55a8f 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -17,7 +17,7 @@ struct linux_binprm;
extern int ima_bprm_check(struct linux_binprm *bprm);
extern int ima_inode_alloc(struct inode *inode);
extern void ima_inode_free(struct inode *inode);
-extern int ima_path_check(struct path *path, int mask);
+extern int ima_path_check(struct file *file, int mask);
extern void ima_file_free(struct file *file);
extern int ima_file_mmap(struct file *file, unsigned long prot);
extern void ima_counts_get(struct file *file);
@@ -38,7 +38,7 @@ static inline void ima_inode_free(struct inode *inode)
return;
}
-static inline int ima_path_check(struct path *path, int mask)
+static inline int ima_path_check(struct file *file, int mask)
{
return 0;
}
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index a89f44d..1ffa54e 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -84,6 +84,36 @@ out:
return found;
}
+/* ima_read_write_check - reflect possible reading/writing errors in the PCR.
+ *
+ * When opening a file for read, if the file is already open for write,
+ * the file could change, resulting in a file measurement error.
+ *
+ * Opening a file for write, if the file is already open for read, results
+ * in a time of measure, time of use (ToMToU) error.
+ *
+ * In either case invalidate the PCR.
+ */
+enum iint_pcr_error { TOMTOU, OPEN_WRITERS };
+static void ima_read_write_check(enum iint_pcr_error error,
+ struct ima_iint_cache *iint,
+ struct inode *inode,
+ const unsigned char *filename)
+{
+ switch (error) {
+ case TOMTOU:
+ if (iint->readcount > 0)
+ ima_add_violation(inode, filename, "invalid_pcr",
+ "ToMToU");
+ break;
+ case OPEN_WRITERS:
+ if (iint->writecount > 0)
+ ima_add_violation(inode, filename, "invalid_pcr",
+ "open_writers");
+ break;
+ }
+}
+
/*
* Update the counts given an fmode_t
*/
@@ -99,6 +129,47 @@ static void ima_inc_counts(struct ima_iint_cache *iint, fmode_t mode)
}
/*
+ * ima_counts_get - increment file counts
+ *
+ * Maintain read/write counters for all files, but only
+ * invalidate the PCR for measured files:
+ * - Opening a file for write when already open for read,
+ * results in a time of measure, time of use (ToMToU) error.
+ * - Opening a file for read when already open for write,
+ * could result in a file measurement error.
+ *
+ */
+void ima_counts_get(struct file *file)
+{
+ struct dentry *dentry = file->f_path.dentry;
+ struct inode *inode = dentry->d_inode;
+ fmode_t mode = file->f_mode;
+ struct ima_iint_cache *iint;
+ int rc;
+
+ if (!ima_initialized || !S_ISREG(inode->i_mode))
+ return;
+ iint = ima_iint_find_get(inode);
+ if (!iint)
+ return;
+ mutex_lock(&iint->mutex);
+ rc = ima_must_measure(iint, inode, MAY_READ, PATH_CHECK);
+ if (rc < 0)
+ goto out;
+
+ if (mode & FMODE_WRITE) {
+ ima_read_write_check(TOMTOU, iint, inode, dentry->d_name.name);
+ goto out;
+ }
+ ima_read_write_check(OPEN_WRITERS, iint, inode, dentry->d_name.name);
+out:
+ ima_inc_counts(iint, file->f_mode);
+ mutex_unlock(&iint->mutex);
+
+ kref_put(&iint->refcount, iint_free);
+}
+
+/*
* Decrement ima counts
*/
static void ima_dec_counts(struct ima_iint_cache *iint, struct inode *inode,
@@ -153,123 +224,6 @@ void ima_file_free(struct file *file)
kref_put(&iint->refcount, iint_free);
}
-/* ima_read_write_check - reflect possible reading/writing errors in the PCR.
- *
- * When opening a file for read, if the file is already open for write,
- * the file could change, resulting in a file measurement error.
- *
- * Opening a file for write, if the file is already open for read, results
- * in a time of measure, time of use (ToMToU) error.
- *
- * In either case invalidate the PCR.
- */
-enum iint_pcr_error { TOMTOU, OPEN_WRITERS };
-static void ima_read_write_check(enum iint_pcr_error error,
- struct ima_iint_cache *iint,
- struct inode *inode,
- const unsigned char *filename)
-{
- switch (error) {
- case TOMTOU:
- if (iint->readcount > 0)
- ima_add_violation(inode, filename, "invalid_pcr",
- "ToMToU");
- break;
- case OPEN_WRITERS:
- if (iint->writecount > 0)
- ima_add_violation(inode, filename, "invalid_pcr",
- "open_writers");
- break;
- }
-}
-
-static int get_path_measurement(struct ima_iint_cache *iint, struct file *file,
- const unsigned char *filename)
-{
- int rc = 0;
-
- ima_inc_counts(iint, file->f_mode);
-
- rc = ima_collect_measurement(iint, file);
- if (!rc)
- ima_store_measurement(iint, file, filename);
- return rc;
-}
-
-/**
- * ima_path_check - based on policy, collect/store measurement.
- * @path: contains a pointer to the path to be measured
- * @mask: contains MAY_READ, MAY_WRITE or MAY_EXECUTE
- *
- * Measure the file being open for readonly, based on the
- * ima_must_measure() policy decision.
- *
- * Keep read/write counters for all files, but only
- * invalidate the PCR for measured files:
- * - Opening a file for write when already open for read,
- * results in a time of measure, time of use (ToMToU) error.
- * - Opening a file for read when already open for write,
- * could result in a file measurement error.
- *
- * Always return 0 and audit dentry_open failures.
- * (Return code will be based upon measurement appraisal.)
- */
-int ima_path_check(struct path *path, int mask)
-{
- struct inode *inode = path->dentry->d_inode;
- struct ima_iint_cache *iint;
- struct file *file = NULL;
- int rc;
-
- if (!ima_initialized || !S_ISREG(inode->i_mode))
- return 0;
- iint = ima_iint_find_get(inode);
- if (!iint)
- return 0;
-
- mutex_lock(&iint->mutex);
-
- rc = ima_must_measure(iint, inode, MAY_READ, PATH_CHECK);
- if (rc < 0)
- goto out;
-
- if ((mask & MAY_WRITE) || (mask == 0))
- ima_read_write_check(TOMTOU, iint, inode,
- path->dentry->d_name.name);
-
- if ((mask & (MAY_WRITE | MAY_READ | MAY_EXEC)) != MAY_READ)
- goto out;
-
- ima_read_write_check(OPEN_WRITERS, iint, inode,
- path->dentry->d_name.name);
- if (!(iint->flags & IMA_MEASURED)) {
- struct dentry *dentry = dget(path->dentry);
- struct vfsmount *mnt = mntget(path->mnt);
-
- file = dentry_open(dentry, mnt, O_RDONLY | O_LARGEFILE,
- current_cred());
- if (IS_ERR(file)) {
- int audit_info = 0;
-
- integrity_audit_msg(AUDIT_INTEGRITY_PCR, inode,
- dentry->d_name.name,
- "add_measurement",
- "dentry_open failed",
- 1, audit_info);
- file = NULL;
- goto out;
- }
- rc = get_path_measurement(iint, file, dentry->d_name.name);
- }
-out:
- mutex_unlock(&iint->mutex);
- if (file)
- fput(file);
- kref_put(&iint->refcount, iint_free);
- return 0;
-}
-EXPORT_SYMBOL_GPL(ima_path_check);
-
static int process_measurement(struct file *file, const unsigned char *filename,
int mask, int function)
{
@@ -297,33 +251,6 @@ out:
return rc;
}
-/*
- * ima_counts_get - increment file counts
- *
- * - for IPC shm and shmat file.
- * - for nfsd exported files.
- *
- * Increment the counts for these files to prevent unnecessary
- * imbalance messages.
- */
-void ima_counts_get(struct file *file)
-{
- struct inode *inode = file->f_dentry->d_inode;
- struct ima_iint_cache *iint;
-
- if (!ima_initialized || !S_ISREG(inode->i_mode))
- return;
- iint = ima_iint_find_get(inode);
- if (!iint)
- return;
- mutex_lock(&iint->mutex);
- ima_inc_counts(iint, file->f_mode);
- mutex_unlock(&iint->mutex);
-
- kref_put(&iint->refcount, iint_free);
-}
-EXPORT_SYMBOL_GPL(ima_counts_get);
-
/**
* ima_file_mmap - based on policy, collect/store measurement.
* @file: pointer to the file to be measured (May be NULL)
@@ -369,6 +296,25 @@ int ima_bprm_check(struct linux_binprm *bprm)
return 0;
}
+/**
+ * ima_path_check - based on policy, collect/store measurement.
+ * @file: pointer to the file to be measured
+ * @mask: contains MAY_READ, MAY_WRITE or MAY_EXECUTE
+ *
+ * Measure files based on the ima_must_measure() policy decision.
+ *
+ * Always return 0 and audit dentry_open failures.
+ * (Return code will be based upon measurement appraisal.)
+ */
+int ima_path_check(struct file *file, int mask)
+{
+ int rc;
+
+ rc = process_measurement(file, file->f_dentry->d_name.name,
+ mask, PATH_CHECK);
+ return 0;
+}
+
static int __init init_ima(void)
{
int error;
--
1.6.0.6
Um...
a) where's the nfsd part?
b) will that work if we open file with O_WRONLY?
nfsd side of things is non-trivial. Note that you have that thing called
an awful lot; nfsd_permission() is called by fh_verify(). For which
operations do you really want it to happen? Should it just migrate to
nfsd_open()?
Files opened O_WRONLY are not measured.
> nfsd side of things is non-trivial. Note that you have that thing called
> an awful lot; nfsd_permission() is called by fh_verify(). For which
> operations do you really want it to happen? Should it just migrate to
> nfsd_open()?
The IMA counters are updated in alloc_file() and __dentry_open().
__dentry_open() is called from a couple of places:
lookup_instantiate_filp(), nameidata_to_filp() and dentry_open. Of
these calls, files are only being measured in the nameidata_to_filp()
path. So yes, the current ima_path_check() needs to be moved to after
the dentry_open() in nfsd_open(), and also added after each of the other
dentry_open() and lookup_instantiate_filp() calls. Otherwise the
counters will be correct, but the files will not be measured.
Eric's proposed patches had two calls to ima_path_check(). Was sure
looking forward to not having to add ima_path_check() calls all over.
Mimi
> The IMA counters are updated in alloc_file() and __dentry_open().
> __dentry_open() is called from a couple of places:
> lookup_instantiate_filp(), nameidata_to_filp() and dentry_open. Of
> these calls, files are only being measured in the nameidata_to_filp()
> path. So yes, the current ima_path_check() needs to be moved to after
> the dentry_open() in nfsd_open(), and also added after each of the other
> dentry_open() and lookup_instantiate_filp() calls. Otherwise the
> counters will be correct, but the files will not be measured.
Wrong. lookup_instantiate_filp() is followed by do_filp_open() ones.
So no, we don't need to add there. As for other dentry_open(), I'm
not at all convinced that we *want* ima_path_check() done for all
of those; it should be decided on per-call basis and it's not a trivial
decision.
Please, see if it's OK with you in this form; other calls of dentry_open()
are separate story, we'll have to see which ones should and which ones
should not get ima_path_check(). Do you have any other problems with that
one? If not, it's going to migrate into for-linus and into the mainline;
this stuff is definitely 2.6.33 fodder.
I'll look at the patches, but in the meantime we also needed the
following patch.
commit 8262bb85da4a71c4ff8c9b22e03aff11f8427b6d
Author: Eric Paris <epa...@redhat.com>
Date: Wed Dec 9 15:29:01 2009 -0500
ima: initialize ima before inodes can be allocated
ima wants to create an inode information struct (iint) when inodes are
allocated. This means that at least the part of ima which does this
allocation (the allocation is filled with information later) should
before any inodes are created. To accomplish this we split the ima
initialization routine placing the kmem cache allocator inside a
security_initcall() function. Since this makes use of radix trees we also
need to make sure that is initialized before security_initcall().
Signed-off-by: Eric Paris <epa...@redhat.com>
Acked-by: Mimi Zohar <zo...@linux.vnet.ibm.com>
Index: security-testing-2.6/init/main.c
===================================================================
--- security-testing-2.6.orig/init/main.c
+++ security-testing-2.6/init/main.c
@@ -663,9 +663,9 @@ asmlinkage void __init start_kernel(void
proc_caches_init();
buffer_init();
key_init();
+ radix_tree_init();
security_init();
vfs_caches_init(totalram_pages);
- radix_tree_init();
signals_init();
/* rootfs populating might need page-writeback */
page_writeback_init();
Index: security-testing-2.6/security/integrity/ima/ima.h
===================================================================
--- security-testing-2.6.orig/security/integrity/ima/ima.h
+++ security-testing-2.6/security/integrity/ima/ima.h
@@ -65,7 +65,6 @@ void integrity_audit_msg(int audit_msgno
const char *cause, int result, int info);
/* Internal IMA function definitions */
-void ima_iintcache_init(void);
int ima_init(void);
void ima_cleanup(void);
int ima_fs_init(void);
Index: security-testing-2.6/security/integrity/ima/ima_iint.c
===================================================================
--- security-testing-2.6.orig/security/integrity/ima/ima_iint.c
+++ security-testing-2.6/security/integrity/ima/ima_iint.c
@@ -52,9 +52,6 @@ int ima_inode_alloc(struct inode *inode)
struct ima_iint_cache *iint = NULL;
int rc = 0;
- if (!ima_initialized)
- return 0;
-
iint = kmem_cache_alloc(iint_cache, GFP_NOFS);
if (!iint)
return -ENOMEM;
@@ -118,8 +115,6 @@ void ima_inode_free(struct inode *inode)
{
struct ima_iint_cache *iint;
- if (!ima_initialized)
- return;
spin_lock(&ima_iint_lock);
iint = radix_tree_delete(&ima_iint_store, (unsigned long)inode);
spin_unlock(&ima_iint_lock);
@@ -141,9 +136,11 @@ static void init_once(void *foo)
kref_set(&iint->refcount, 1);
}
-void __init ima_iintcache_init(void)
+static int __init ima_iintcache_init(void)
{
iint_cache =
kmem_cache_create("iint_cache", sizeof(struct ima_iint_cache), 0,
SLAB_PANIC, init_once);
+ return 0;
}
+security_initcall(ima_iintcache_init);
Index: security-testing-2.6/security/integrity/ima/ima_main.c
===================================================================
--- security-testing-2.6.orig/security/integrity/ima/ima_main.c
+++ security-testing-2.6/security/integrity/ima/ima_main.c
@@ -319,7 +319,6 @@ static int __init init_ima(void)
{
int error;
- ima_iintcache_init();
error = ima_init();
ima_initialized = 1;
return error;
FWIW, I'd take radix_tree_init() much earlier than that; it's a common
infrastructure and if we have it early it'll mean less headache.
AFAICS, it can go right after mm_init(). Objections, anyone?
Other than that, I'm OK with that one.
On Tue, Jan 26, 2010 at 12:59:07PM -0500, Mimi Zohar wrote:
> I still don't see that the
> calls to do_filp_open() in the lookup_instantiate_filp(), but will take
> a closer look once these patches are out. Also not sure if there is
> anything in the alloc_file() path.
OK... lookup_instantiate_filp() is a god-awful mess, so it's OK to be
confused by it - its authors definitely had been ;-)
The thing is, currently lookup_instantiate_filp() is called *only* from
call chains starting in do_filp_open(). And place right after
nameidata_to_filp() is downstream from both those call chains and
from the call chain leading from do_filp_open() to dentry_open().
Gory and revolting details follow.
What's happening is easier to understand with code massage done in
vfs-2.6.git#untested; basically, the logics of do_filp_open() (in
_very_ obfuscated form in the mainline kernel) is this:
* find parent directory + last component of pathname
* do "open or create file with this name in that directory" actions
* we can
+ get opened struct file. We are done.
+ get ERR_PTR(-error). Fail.
+ be told that it's a symlink
* try to follow it
+ if it fails, fail.
+ if it's a "direct" symlink a-la procfs, we just get to
open the file it points to. It *will* exist, so O_CREAT|O_EXCL => fail,
and O_CREAT alone gets ignored.
+ if it's a normal symlink, we'd just followed it up to
the last component. Now we have new directory and new filename; repeat
the steps above.
That's the high-level overview. The reason for that kind of loop lies in
the messy semantics of O_CREAT on dangling symlinks; nevermind that part
of the horror show for now.
The really interesting part is opening a file in known directory.
Normal filesystems do d_lookup + either ->lookup() or ->d_revalidate()
+ dentry_open() which will call ->open(). For file creation it may become
d_lookup + either ->lookup() or ->d_revalidate() + ->create() + dentry_open().
*However*, NFS really wants to issue a single request to server that would
do lookup + maybe create + open in one step. Atomically. Fair enough,
but they have a nasty problem - which of existing fs methods would do that?
Neither ->d_revalidate() nor ->lookup() are going to get struct file from
their caller, after all...
Now, the sane solution would be to introduce a new method that would do
lookup-or-d_revalidate+maybe-create+open. With normal filesystems defaulting
to the usual sequence. And have its caller pass all we need to it (struct
file, flags, mode, etc.). That, BTW, is where #untested leads to - a large
part of things done in fs/namei.c:do_last() there will eventually bud off
into default instance of that new method.
However, that's *NOT* what had been done. Instead of that the arguments
(struct file *, etc.) are hidden in struct nameidata and a pointer
to nameidata is added to argument lists of ->d_revalidate(), ->lookup(), etc.
And NFS has added "let's talk to server and try atomic open" into those
methods. If that attempt succeeds, they call a helper
(lookup_instantiate_filp()) that fills the struct file indirectly passed
to the damn thing via pointer in nameidata.
Eventually, we emerge from all that mess back into do_filp_open(). There
we look at the struct file that had been refered to from nameidata and
check if it had already been filled. _THEN_ we do dentry_open() in case
file hadn't been already filled by call of lookup_instantiate_filp() issued
by one of fs methods. That's nameidata_to_filp().
So any call of lookup_instantiate_filp() will be followed by nameidata_to_filp()
and that's where we'll pick the results of the former. If there'd been no
such call (we are on a normal fs, or NFS doesn't feel like doing atomic open
for that one), nameidata_to_filp() will be the place that calls dentry_open().
IOW, the point directly after nameidata_to_filp() is where all those paths
converge. And yes, it's a horrible mess. One I'd been slowly massaging into
saner shape for a year already ;-/
Eventually nameidata_to_filp() and lookup_instantiate_filp() will go to hell
and we'll get one or two methods getting unopened struct file, dentry (unhashed
new one or found in dcache) and the rest of open() arguments. Default will
be to do ->lookup() or ->d_revalidate() + possibly ->create() + dentry_open();
NFS will be able to do its atomic open stuff ending with dentry_open().
In either case we'll get success, error or "it's a symlink, deal with it
yourself" and caller will act accordingly. And that'll be the place to
do any immediately-after-open things, including ima_path_check(). With
only do_filp_open() ever calling that method.
Other than missing the export for ima_path_check() for nfs and the
re-ordering not being bisect safe, they look good. I've updated
the following two patches, from Eric's patchset, and would appreciate
your applying them as well:
ima: rename ima_path_check to ima_file_check
ima: rename PATH_CHECK to FILE_CHECK
Posting them separately.
Mimi