introduce new LSM hooks where vfsmount is available.

Al, would you respond to this patch?
James, Andrew and we are waiting for your Acked-by: response.

Regards,

------- Forwarded Message
From: Al Viro <[email protected]>
To: [email protected]
Cc: [email protected], [email protected],[email protected], [email protected],[email protected], [email protected], [email protected],[email protected], [email protected]
Subject: Re: [patch 04/11] vfs: introduce new LSM hooks where vfsmount isavailable.
Date: Sat, 6 Dec 2008 07:10:09 +0000

On Mon, Dec 01, 2008 at 02:34:54PM -0800, [email protected] wrote:

In principle, ACK.

do_sys_ftruncate() calls do_truncate() with "struct file *", whereas do_sys_truncate() and may_open() call do_truncate() with NULL. TOMOYO is not using the last argument, but I'm passing all arguments without modification so that a LSM module can know the last argument if that LSM module needs the last argument.

Regards.

------- End of Forwarded Message

------- Forwarded Message
From: Al Viro <[email protected]>
To: Tetsuo Handa <[email protected]>
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: [patch 04/11] vfs: introduce new LSM hooks where vfsmount is available.
Date: Sat, 6 Dec 2008 18:29:02 +0000

And if such a module ever shows up *and* its authors can give good reasons for checks in that place that apply to ftruncate(2) but not to truncate(2), we can always add such argument. Until then...

Note that similar story with struct nameidata * being passed to fs methods had caused a lot of mess and we are still not through with it. Let's stick to the things that make clear sense...

------- End of Forwarded Message

------- Forwarded Message
From: Kentaro Takeda <[email protected]>
To: [email protected]
Cc: [email protected], [email protected],[email protected], [email protected],[email protected], [email protected], [email protected],[email protected], [email protected],[email protected], [email protected]
Subject: Re: [patch 04/11] vfs: introduce new LSM hooks where vfsmount isavailable.
Date: Mon, 08 Dec 2008 10:33:57 +0900

(added cc:lsm-list, linux-kernel-list)

I see. I removed the last argument from this patch.
I attach the updated version of this patch with updated changelog.

I'm sorry for letting you repeat the same answer.
TOMOYO wants to do DAC checks before MAC checks. Thus, I tried to embed DAC's code copied from VFS helper functions into security_path_*() hooks. But I received a comment that duplicating DAC's code is ugly. Thus, I was searching for a way to do MAC checks at security_inode_*() hooks. To do MAC checks at security_inode_*() hooks, I need to somehow pass vfsmount's pathname, but I now understood that passing something via implicit arguments is not acceptable. Thus, I give up approaches which use implicit arguments.

Regards,

(patch omitted)
------- End of Forwarded Message

------- Forwarded Message
From: Kentaro Takeda <[email protected]>
To: [email protected]
Cc: [email protected], [email protected],[email protected], [email protected],[email protected], [email protected], [email protected],[email protected], [email protected],[email protected], [email protected]
Subject: Re: [patch 04/11] vfs: introduce new LSM hooks where vfsmount isavailable.
Date: Fri, 12 Dec 2008 10:59:39 +0900

Al, please give us Acked-by response.
(patch refreshed for mmotm-2008-12-11-15-20).

Regards,

---
Subject: vfs: introduce new LSM hooks where vfsmount is available.

----- What is this patch for? -----

There are security_inode_*() LSM hooks for attribute-based MAC, but they are not suitable for pathname-based MAC because they don't receive "struct vfsmount" information.

----- How this patch was developed? -----

Two pathname-based MACs, AppArmor and TOMOYO Linux, are trying to merge upstream. But because of "struct vfsmount" problem, they have been unable to merge upstream.

Here are the list of approaches and the reasons of denial.

(1) Not using LSM
http://lwn.net/Articles/277833/

This approach was rejected because security modules should use LSM because the whole idea behind LSM was to have a single set of hooks for all security modules; if every module now adds its own set of hooks, that purpose will have been defeated and the kernel will turn into a big mess of security hooks.

(2) Retrieving "struct vfsmount" from "struct task_struct".
http://lkml.org/lkml/2007/11/5/388

Since "struct task_struct" contains list of "struct vfsmount", "struct vfsmount" which corresponds to "struct dentry" can be retrieved from the list unless "mount --bind" is used.

This approach turned out to cause a critical problem that getting namespace_sem lock from security_inode_*() triggers AB-BA deadlock.

(3) Adding "struct vfsmount" parameter to VFS helper functions.
http://lkml.org/lkml/2008/5/29/207

This approach adds "struct vfsmount" to VFS helper functions (e.g. vfs_mkdir() and vfs_symlink()) and LSM hooks inside VFS helper functions. This approach is helpful for not only AppArmor and TOMOYO Linux 2.x but also SELinux and auditing purpose, for this approach allows existent LSM users to use pathnames in their access control and audit logs.

This approach was rejected by Al Viro, the VFS maintainer, because he thinks individual filesystem should remain "struct vfsmount"-unaware and VFS helper functions should not receive "struct vfsmount".

Al Viro also suggested to move existing security_inode_*() to out of VFS helper functions so that security_inode_*() can receive "struct vfsmount" without modifying VFS helper functions, but this suggestion was opposed by Stephen Smalley because changing the order of permission checks (i.e. MAC checks before DAC checks) is not acceptable.

(4) Passing "struct vfsmount" via "struct task_struct".
http://lkml.org/lkml/2007/11/16/157

Since we didn't understand the reason why accessing "struct vfsmount" from LSM hooks inside VFS helper functions is not acceptable, we thought the reason why VFS helper functions don't receive "struct vfsmount" is the amount of modifications needed to do so. Thus, we proposed to pass "struct vfsmount" via "struct task_struct" so that modifications remain minimal.

This approach was rejected because this is an abuse of "struct task_struct".

(5) Remembering pathname of "struct vfsmount" via "struct task_struct".
http://lkml.org/lkml/2008/8/19/16

Since pathname of a "struct dentry" up to the mount point can be calculated without "struct vfsmount", absolute pathname of a "struct dentry" can be calculated if "struct task_struct" can remember absolute pathname of a "struct vfsmount" which corresponds to "struct dentry".

As we now understand that Al Viro is opposing to access "struct vfsmount" from LSM hooks inside VFS helper functions, we gave up delivering "struct vfsmount" itself to LSM hooks inside VFS helper functions.

Kernel 2.6.26 introduced read-only bind mount feature, and hooks for that feature (i.e. mnt_want_write() and mnt_drop_write()) were inserted around VFS helper functions call. Since mnt_want_write() receives "struct vfsmount" which corresponds to "struct dentry" that will be passed to subsequent VFS helper functions call, we associated pathname of "struct vfsmount" with "struct task_struct" instead of associating "struct vfsmount" itself.

This approach was rejected because of the kludge and of having the effect of vfs_mkdir() et.al. dependent on anything except the arguments it's getting.

(6) Introducing new LSM hooks where vfsmount is available. (this patch)

We came to the conclusion that adding new LSM hooks which receive all necessary arguments (e.g. "struct vfsmount" and "struct dentry") outside VFS helper functions is the only acceptable approach.

This approach has less impact to existing LSM module and no impact to VFS helper functions.

---

 fs/namei.c               |   36 ++++++++++++
 fs/open.c                |    5 +
 include/linux/security.h |  137 +++++++++++++++++++++++++++++++++++++++++++++++
 net/unix/af_unix.c       |    4 +
 security/Kconfig         |    9 +++
 security/capability.c    |   57 +++++++++++++++++++
 security/security.c      |   66 ++++++++++++++++++++++
 7 files changed, 314 insertions(+)

@@ -1556,6 +1556,9 @@ int may_open(struct nameidata *nd, int a
 		 * Refuse to truncate files with mandatory locks held on them.
 		 */
 		error = locks_verify_locked(inode);
+		if (!error)
+			error = security_path_truncate(&nd->path, 0,
+					       ATTR_MTIME|ATTR_CTIME|ATTR_OPEN);
 		if (!error) {
 			DQUOT_INIT(inode);
 

@@ -1586,7 +1589,11 @@ static int __open_namei_create(struct na
 
 	if (!IS_POSIXACL(dir->d_inode))
 		mode &= ~current->fs->umask;
+	error = security_path_mknod(&nd->path, path->dentry, mode, 0);
+	if (error)
+		goto out_unlock;
 	error = vfs_create(dir->d_inode, path->dentry, mode, nd);
+out_unlock:
 	mutex_unlock(&dir->d_inode->i_mutex);
 	dput(nd->path.dentry);
 	nd->path.dentry = path->dentry;

@@ -1999,6 +2006,9 @@ asmlinkage long sys_mknodat(int dfd, con
 	error = mnt_want_write(nd.path.mnt);
 	if (error)
 		goto out_dput;
+	error = security_path_mknod(&nd.path, dentry, mode, dev);
+	if (error)
+		goto out_drop_write;
 	switch (mode & S_IFMT) {
 		case 0: case S_IFREG:
 			error = vfs_create(nd.path.dentry->d_inode,dentry,mode,&nd);

@@ -2011,6 +2021,7 @@ asmlinkage long sys_mknodat(int dfd, con
 			error = vfs_mknod(nd.path.dentry->d_inode,dentry,mode,0);
 			break;
 	}
+out_drop_write:
 	mnt_drop_write(nd.path.mnt);
 out_dput:
 	dput(dentry);

@@ -2070,7 +2081,11 @@ asmlinkage long sys_mkdirat(int dfd, con
 	error = mnt_want_write(nd.path.mnt);
 	if (error)
 		goto out_dput;
+	error = security_path_mkdir(&nd.path, dentry, mode);
+	if (error)
+		goto out_drop_write;
 	error = vfs_mkdir(nd.path.dentry->d_inode, dentry, mode);
+out_drop_write:
 	mnt_drop_write(nd.path.mnt);
 out_dput:
 	dput(dentry);

@@ -2180,7 +2195,11 @@ static long do_rmdir(int dfd, const char
 	error = mnt_want_write(nd.path.mnt);
 	if (error)
 		goto exit3;
+	error = security_path_rmdir(&nd.path, dentry);
+	if (error)
+		goto exit4;
 	error = vfs_rmdir(nd.path.dentry->d_inode, dentry);
+exit4:
 	mnt_drop_write(nd.path.mnt);
 exit3:
 	dput(dentry);

@@ -2265,7 +2284,11 @@ static long do_unlinkat(int dfd, const c
 		error = mnt_want_write(nd.path.mnt);
 		if (error)
 			goto exit2;
+		error = security_path_unlink(&nd.path, dentry);
+		if (error)
+			goto exit3;
 		error = vfs_unlink(nd.path.dentry->d_inode, dentry);
+exit3:
 		mnt_drop_write(nd.path.mnt);
 	exit2:
 		dput(dentry);

@@ -2346,7 +2369,11 @@ asmlinkage long sys_symlinkat(const char
 	error = mnt_want_write(nd.path.mnt);
 	if (error)
 		goto out_dput;
+	error = security_path_symlink(&nd.path, dentry, from);
+	if (error)
+		goto out_drop_write;
 	error = vfs_symlink(nd.path.dentry->d_inode, dentry, from);
+out_drop_write:
 	mnt_drop_write(nd.path.mnt);
 out_dput:
 	dput(dentry);

@@ -2443,7 +2470,11 @@ asmlinkage long sys_linkat(int olddfd, c
 	error = mnt_want_write(nd.path.mnt);
 	if (error)
 		goto out_dput;
+	error = security_path_link(old_path.dentry, &nd.path, new_dentry);
+	if (error)
+		goto out_drop_write;
 	error = vfs_link(old_path.dentry, nd.path.dentry->d_inode, new_dentry);
+out_drop_write:
 	mnt_drop_write(nd.path.mnt);
 out_dput:
 	dput(new_dentry);

@@ -2677,8 +2708,13 @@ asmlinkage long sys_renameat(int olddfd,
 	error = mnt_want_write(oldnd.path.mnt);
 	if (error)
 		goto exit5;
+	error = security_path_rename(&oldnd.path, old_dentry,
+				     &newnd.path, new_dentry);
+	if (error)
+		goto exit6;
 	error = vfs_rename(old_dir->d_inode, old_dentry,
 				   new_dir->d_inode, new_dentry);
+exit6:
 	mnt_drop_write(oldnd.path.mnt);
 exit5:
 	dput(new_dentry);

@@ -272,6 +272,8 @@ static long do_sys_truncate(const char _
 		goto put_write_and_out;
 
 	error = locks_verify_truncate(inode, NULL, length);
+	if (!error)
+		error = security_path_truncate(&path, length, 0);
 	if (!error) {
 		DQUOT_INIT(inode);
 		error = do_truncate(path.dentry, length, 0, NULL);

@@ -329,6 +331,9 @@ static long do_sys_ftruncate(unsigned in
 
 	error = locks_verify_truncate(inode, file, length);
 	if (!error)
+		error = security_path_truncate(&file->f_path, length,
+					       ATTR_MTIME|ATTR_CTIME);
+	if (!error)
 		error = do_truncate(dentry, length, ATTR_MTIME|ATTR_CTIME, file);
 out_putf:
 	fput(file);

@@ -836,7 +836,11 @@ static int unix_bind(struct socket *sock
 		err = mnt_want_write(nd.path.mnt);
 		if (err)
 			goto out_mknod_dput;
+		err = security_path_mknod(&nd.path, dentry, mode, 0);
+		if (err)
+			goto out_mknod_drop_write;
 		err = vfs_mknod(nd.path.dentry->d_inode, dentry, mode, 0);
+out_mknod_drop_write:
 		mnt_drop_write(nd.path.mnt);
 		if (err)
 			goto out_mknod_dput;

@@ -81,6 +81,15 @@ config SECURITY_NETWORK_XFRM
 	  IPSec.
 	  If you are unsure how to answer this question, answer N.
 
+config SECURITY_PATH
+	bool "Security hooks for pathname based access control"
+	depends on SECURITY
+	help
+	  This enables the security hooks for pathname based access control.
+	  If enabled, a security module can use these hooks to
+	  implement pathname based access controls.
+	  If you are unsure how to answer this question, answer N.
+
 config SECURITY_FILE_CAPABILITIES
 	bool "File POSIX Capabilities"
 	default n

------- End of Forwarded Message

In queue. FWIW, I'm about to post the current audit patchset to linux-audit, push that crap to audit git and then I'm through with that turd. And _finally_ can get to VFS queue sorting.

"That crap" == audit patches for .29-rc1, that is.

I see, thank you for informing us.
We'll wait for your work.

Regards,