Fix untangling ima mess, part 2 with counters

The "Untangling ima mess, part 2 with counters" patch messed up the counters. Based on conversations with Al Viro, this patch streamlines ima_path_check() by removing the counter maintaince. The counters are now updated independently, from measuring the file, in __dentry_open() and alloc_file() by calling ima_counts_get(). ima_path_check() is called from nfsd and do_filp_open().

---
 fs/namei.c                        |    4 +-
 include/linux/ima.h               |    4 +-
 security/integrity/ima/ima_main.c |  234 ++++++++++++++-----------------------
 3 files changed, 94 insertions(+), 148 deletions(-)

index fabd939..a89ebc3 100644
@@ -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);

index 99dc6d5..aa55a8f 100644
@@ -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;
 }

1.6.0.6

The "Untangling ima mess, part 2 with counters" patch not only messed up the counters, but also doesn't measure files which should be measured. The "Untangling ima mess ..." patchset, applied some of Eric's patches, but not all, leaving inodes allocated before late_initcall() not allocated/measured. (8262bb85da ima: initialize ima before inodes can be allocated)

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

The "Untangling ima mess, part 2 with counters" patch did not measure all files, which should have been measured.

---
 fs/namei.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

index a89ebc3..e1f15e8 100644
@@ -1730,7 +1730,7 @@ do_last:
 		if (nd.root.mnt)
 			path_put(&nd.root);
 		if (!IS_ERR(filp)) {
-			error = ima_path_check(filp, filp->f_mode &
+			error = ima_path_check(filp, acc_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, filp->f_mode &
+		error = ima_path_check(filp, acc_mode &
 			       (MAY_READ | MAY_WRITE | MAY_EXEC));
 		if (error) {
 			fput(filp);

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.

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

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.

OK, this stuff is in for-next, with the following changes:
* nfsd call has been moved to nfsd_open()
* patches reordered
* masking irrelevant bits (i.e. leaving only MAY_{READ,WRITE,EXEC})
has been taken to ima_path_check(); all callers do it and it's safer that way anyway.

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 <[email protected]>
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().

Index: 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
===================================================================
@@ -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
===================================================================
@@ -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
===================================================================
@@ -319,7 +319,6 @@ static int __init init_ima(void)
 {
 	int error;
 
-	ima_iintcache_init();
 	error = ima_init();
 	ima_initialized = 1;
 	return error;

[fsdevel Cc'd; see the horror story/semi-rant below]

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