Confirmation of methods for calculating requested pathname.

Confirmation of methods for calculating requested pathname.

[Kentaro Takeda]

The current Linux kernel is not designed to pass vfsmount parameter
that is crucial for pathname-based security including AppArmor and
TOMOYO Linux, to LSM. Though both projects have been proposing
patches to calculate pathname, none of them have been accepted as
you know.

To find the reason for NACK, we examined past proposals and the
threads. And we came to understand that you oppose accessing vfsmount
inside vfs helper functions. Is our understanding correct?

If our understanding is correct, we would like to propose a new
method that does not require modifications to vfs helper functions.
Attached patch is a trial of this method.

vfs helper functions are surrounded by mnt_want_write() and
mnt_drop_write() pairs which receive "struct vfsmount" parameter
since 2.6.26. So, by remembering the absolute pathname of "struct
vfsmount" of the moment, LSM module can calculate an absolute
pathname of the given "struct dentry" parameter inside vfs_*
functions, without passing "struct vfsmount" parameter to vfs_*
functions.

This approach doesn't access vfsmount inside vfs helper functions,
and modification of existing kernel is only in task_struct and
mnt_want/drop_write().

We would like to hear your comments as fs maintainer.

Regards,

---
Subject: Remembering previously referenced vfsmount's pathname.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 fs/Kconfig                |   18 ++++++++++++++++++
 fs/namespace.c            |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/init_task.h |    7 +++++++
 include/linux/sched.h     |    3 +++
 kernel/fork.c             |    8 ++++++++
 5 files changed, 82 insertions(+)

--- linux-2.6.27-rc3.orig/fs/Kconfig
+++ linux-2.6.27-rc3/fs/Kconfig
@@ -2094,4 +2094,22 @@ endif
 source "fs/nls/Kconfig"
 source "fs/dlm/Kconfig"
 
+config REMEMBER_VFSMOUNT_PATH
+	bool "Remember previously referenced vfsmount's pathname."
+	help
+	  The vfs_* functions (e.g. vfs_mkdir) don't receive "struct vfsmount"
+	  parameter, making it impossible to calculate an absolute pathname of
+	  the given "struct dentry" parameter inside vfs_* functions.
+	
+	  If someone wants to get an absolute pathname of the given
+	  "struct dentry" parameter, he/she must calculate it outside
+	  vfs_* functions.
+	
+	  However, vfs_* functions are surrounded by mnt_want_write() and
+	  mnt_drop_write() pairs which receive "struct vfsmount" parameter.
+	  So, by remembering the absolute pathname of "struct vfsmount" of
+	  the moment, he/she can calculate an absolute pathname of the given
+	  "struct dentry" parameter inside vfs_* functions, without passing
+	  "struct vfsmount" parameter to vfs_* functions.
+
 endmenu
--- linux-2.6.27-rc3.orig/fs/namespace.c
+++ linux-2.6.27-rc3/fs/namespace.c
@@ -231,6 +231,41 @@ static inline void use_cpu_writer_for_mo
 	cpu_writer->mnt = mnt;
 }
 
+#ifdef CONFIG_REMEMBER_VFSMOUNT_PATH
+static void forget_vfsmount_path(void)
+{
+	struct task_struct *task = current;
+	kfree(task->current_vfsmount_path);
+	task->current_vfsmount_path = NULL;
+}
+
+static bool remember_vfsmount_path(struct vfsmount *mnt)
+{
+	struct task_struct *task = current;
+	struct path path = { mnt, mnt->mnt_root };
+	char *page;
+	char *ret;
+	if (task->current_vfsmount_path)
+		forget_vfsmount_path();
+	page = kmalloc(PAGE_SIZE, GFP_KERNEL);
+	if (!page)
+		goto out;
+	ret = d_path(&path, page, PAGE_SIZE);
+	if (IS_ERR(ret))
+		goto out_free;
+	ret = kstrdup(ret, GFP_KERNEL);
+	if (!ret)
+		goto out_free;
+	kfree(page);
+	task->current_vfsmount_path = ret;
+	return 1;
+ out_free:
+	kfree(page);
+ out:
+	return 0;
+}
+#endif
+
 /*
  * Most r/o checks on a fs are for operations that take
  * discrete amounts of time, like a write() or unlink().
@@ -253,6 +288,10 @@ int mnt_want_write(struct vfsmount *mnt)
 {
 	int ret = 0;
 	struct mnt_writer *cpu_writer;
+#ifdef CONFIG_REMEMBER_VFSMOUNT_PATH
+	if (!remember_vfsmount_path(mnt))
+		return -ENOMEM;
+#endif
 
 	cpu_writer = &get_cpu_var(mnt_writers);
 	spin_lock(&cpu_writer->lock);
@@ -265,6 +304,10 @@ int mnt_want_write(struct vfsmount *mnt)
 out:
 	spin_unlock(&cpu_writer->lock);
 	put_cpu_var(mnt_writers);
+#ifdef CONFIG_REMEMBER_VFSMOUNT_PATH
+	if (ret)
+		forget_vfsmount_path();
+#endif
 	return ret;
 }
 EXPORT_SYMBOL_GPL(mnt_want_write);
@@ -362,6 +405,9 @@ void mnt_drop_write(struct vfsmount *mnt
 	 * we could theoretically wrap __mnt_writers.
 	 */
 	put_cpu_var(mnt_writers);
+#ifdef CONFIG_REMEMBER_VFSMOUNT_PATH
+	forget_vfsmount_path();
+#endif
 }
 EXPORT_SYMBOL_GPL(mnt_drop_write);
 
--- linux-2.6.27-rc3.orig/include/linux/init_task.h
+++ linux-2.6.27-rc3/include/linux/init_task.h
@@ -113,6 +113,12 @@ extern struct group_info init_groups;
 # define CAP_INIT_BSET  CAP_INIT_EFF_SET
 #endif
 
+#ifdef CONFIG_REMEMBER_VFSMOUNT_PATH
+#define INIT_VFSMOUNT_PATH .current_vfsmount_path = NULL,
+#else
+#define INIT_VFSMOUNT_PATH
+#endif
+
 /*
  *  INIT_TASK is used to set up the first task table, touch at
  * your own risk!. Base=0, limit=0x1fffff (=2MB)
@@ -179,6 +185,7 @@ extern struct group_info init_groups;
 	INIT_IDS							\
 	INIT_TRACE_IRQFLAGS						\
 	INIT_LOCKDEP							\
+	INIT_VFSMOUNT_PATH                                              \
 }
 
 
--- linux-2.6.27-rc3.orig/include/linux/sched.h
+++ linux-2.6.27-rc3/include/linux/sched.h
@@ -1300,6 +1300,9 @@ struct task_struct {
 	int latency_record_count;
 	struct latency_record latency_record[LT_SAVECOUNT];
 #endif
+#ifdef CONFIG_REMEMBER_VFSMOUNT_PATH
+	const char *current_vfsmount_path;
+#endif
 };
 
 /*
--- linux-2.6.27-rc3.orig/kernel/fork.c
+++ linux-2.6.27-rc3/kernel/fork.c
@@ -1051,6 +1051,14 @@ static struct task_struct *copy_process(
 	p->blocked_on = NULL; /* not blocked yet */
 #endif
 
+#ifdef CONFIG_REMEMBER_VFSMOUNT_PATH
+	/*
+	 * It seems to me that this function is called between mnt_want_write()
+	 * and mnt_drop_write(). I don't know why.
+	 */
+	p->current_vfsmount_path = NULL;
+#endif
+
 	/* Perform scheduler related setup. Assign this task to a CPU. */
 	sched_fork(p, clone_flags);
 

(repost) Confirmation of methods for calculating requested pathname.

[Kentaro Takeda]

Al, could you answer the following question?


The current Linux kernel is not designed to pass vfsmount parameter
that is crucial for pathname-based security including AppArmor and
TOMOYO Linux, to LSM. Though both projects have been proposing
patches to calculate pathname, none of them have been accepted as
you know.

To find the reason for NACK, we examined past proposals and the
threads. And we came to understand that you oppose accessing vfsmount
inside vfs helper functions. Is our understanding correct?

If our understanding is correct, we would like to propose a new
method that does not require modifications to vfs helper functions.
Attached patch is a trial of this method.

vfs helper functions are surrounded by mnt_want_write() and
mnt_drop_write() pairs which receive "struct vfsmount" parameter
since 2.6.26. So, by remembering the absolute pathname of "struct
vfsmount" of the moment, LSM module can calculate an absolute
pathname of the given "struct dentry" parameter inside vfs_*
functions, without passing "struct vfsmount" parameter to vfs_*
functions.

This approach doesn't access vfsmount inside vfs helper functions,
and modification of existing kernel is only in task_struct and
mnt_want/drop_write().

We would like to hear your comments as fs maintainer.

Regards,

---
Subject: Remembering previously referenced vfsmount's pathname.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 fs/Kconfig                |   18 ++++++++++++++++++
 fs/namespace.c            |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/init_task.h |    7 +++++++
 include/linux/sched.h     |    3 +++
 kernel/fork.c             |    8 ++++++++
 5 files changed, 82 insertions(+)

--- linux-2.6.27-rc5.orig/fs/Kconfig
+++ linux-2.6.27-rc5/fs/Kconfig
@@ -2093,4 +2093,22 @@ endif
 source "fs/nls/Kconfig"
 source "fs/dlm/Kconfig"
 
+config REMEMBER_VFSMOUNT_PATH
+	bool "Remember previously referenced vfsmount's pathname."
+	help
+	  The vfs_* functions (e.g. vfs_mkdir) don't receive "struct vfsmount"
+	  parameter, making it impossible to calculate an absolute pathname of
+	  the given "struct dentry" parameter inside vfs_* functions.
+	
+	  If someone wants to get an absolute pathname of the given
+	  "struct dentry" parameter, he/she must calculate it outside
+	  vfs_* functions.
+	
+	  However, vfs_* functions are surrounded by mnt_want_write() and
+	  mnt_drop_write() pairs which receive "struct vfsmount" parameter.
+	  So, by remembering the absolute pathname of "struct vfsmount" of
+	  the moment, he/she can calculate an absolute pathname of the given
+	  "struct dentry" parameter inside vfs_* functions, without passing
+	  "struct vfsmount" parameter to vfs_* functions.
+
 endmenu
--- linux-2.6.27-rc5.orig/fs/namespace.c
+++ linux-2.6.27-rc5/fs/namespace.c
@@ -231,6 +231,41 @@ static inline void use_cpu_writer_for_mo
 	cpu_writer->mnt = mnt;
 }
 
+#ifdef CONFIG_REMEMBER_VFSMOUNT_PATH
+static void forget_vfsmount_path(void)
+{
+	struct task_struct *task = current;
+	kfree(task->current_vfsmount_path);
+	task->current_vfsmount_path = NULL;
+}
+
+static bool remember_vfsmount_path(struct vfsmount *mnt)
+{
+	struct task_struct *task = current;
+	struct path path = { mnt, mnt->mnt_root };
+	char *page;
+	char *ret;
+	if (task->current_vfsmount_path)
+		forget_vfsmount_path();
+	page = kmalloc(PAGE_SIZE, GFP_KERNEL);
+	if (!page)
+		goto out;
+	ret = d_path(&path, page, PAGE_SIZE);
+	if (IS_ERR(ret))
+		goto out_free;
+	ret = kstrdup(ret, GFP_KERNEL);
+	if (!ret)
+		goto out_free;
+	kfree(page);
+	task->current_vfsmount_path = ret;
+	return 1;
+ out_free:
+	kfree(page);
+ out:
+	return 0;
+}
+#endif
+
 /*
  * Most r/o checks on a fs are for operations that take
  * discrete amounts of time, like a write() or unlink().
@@ -253,6 +288,10 @@ int mnt_want_write(struct vfsmount *mnt)
 {
 	int ret = 0;
 	struct mnt_writer *cpu_writer;
+#ifdef CONFIG_REMEMBER_VFSMOUNT_PATH
+	if (!remember_vfsmount_path(mnt))
+		return -ENOMEM;
+#endif
 
 	cpu_writer = &get_cpu_var(mnt_writers);
 	spin_lock(&cpu_writer->lock);
@@ -265,6 +304,10 @@ int mnt_want_write(struct vfsmount *mnt)
 out:
 	spin_unlock(&cpu_writer->lock);
 	put_cpu_var(mnt_writers);
+#ifdef CONFIG_REMEMBER_VFSMOUNT_PATH
+	if (ret)
+		forget_vfsmount_path();
+#endif
 	return ret;
 }
 EXPORT_SYMBOL_GPL(mnt_want_write);
@@ -362,6 +405,9 @@ void mnt_drop_write(struct vfsmount *mnt
 	 * we could theoretically wrap __mnt_writers.
 	 */
 	put_cpu_var(mnt_writers);
+#ifdef CONFIG_REMEMBER_VFSMOUNT_PATH
+	forget_vfsmount_path();
+#endif
 }
 EXPORT_SYMBOL_GPL(mnt_drop_write);
 
--- linux-2.6.27-rc5.orig/include/linux/init_task.h
+++ linux-2.6.27-rc5/include/linux/init_task.h
@@ -113,6 +113,12 @@ extern struct group_info init_groups;
 # define CAP_INIT_BSET  CAP_INIT_EFF_SET
 #endif
 
+#ifdef CONFIG_REMEMBER_VFSMOUNT_PATH
+#define INIT_VFSMOUNT_PATH .current_vfsmount_path = NULL,
+#else
+#define INIT_VFSMOUNT_PATH
+#endif
+
 /*
  *  INIT_TASK is used to set up the first task table, touch at
  * your own risk!. Base=0, limit=0x1fffff (=2MB)
@@ -179,6 +185,7 @@ extern struct group_info init_groups;
 	INIT_IDS							\
 	INIT_TRACE_IRQFLAGS						\
 	INIT_LOCKDEP							\
+	INIT_VFSMOUNT_PATH                                              \
 }
 
 
--- linux-2.6.27-rc5.orig/include/linux/sched.h
+++ linux-2.6.27-rc5/include/linux/sched.h
@@ -1301,6 +1301,9 @@ struct task_struct {
 	int latency_record_count;
 	struct latency_record latency_record[LT_SAVECOUNT];
 #endif
+#ifdef CONFIG_REMEMBER_VFSMOUNT_PATH
+	const char *current_vfsmount_path;
+#endif
 };
 
 /*
--- linux-2.6.27-rc5.orig/kernel/fork.c
+++ linux-2.6.27-rc5/kernel/fork.c
@@ -1051,6 +1051,14 @@ static struct task_struct *copy_process(
 	p->blocked_on = NULL; /* not blocked yet */
 #endif
 
+#ifdef CONFIG_REMEMBER_VFSMOUNT_PATH
+	/*
+	 * It seems to me that this function is called between mnt_want_write()
+	 * and mnt_drop_write(). I don't know why.
+	 */
+	p->current_vfsmount_path = NULL;
+#endif
+
 	/* Perform scheduler related setup. Assign this task to a CPU. */
 	sched_fork(p, clone_flags);
 

Re: (repost) Confirmation of methods for calculating requested pathname.

[Alexey Dobriyan]

On Tue, Sep 02, 2008 at 01:31:50PM +0900, Kentaro Takeda wrote:
> Al, could you answer the following question?
> 
> 
> The current Linux kernel is not designed to pass vfsmount parameter
> that is crucial for pathname-based security including AppArmor and
> TOMOYO Linux, to LSM. Though both projects have been proposing
> patches to calculate pathname, none of them have been accepted as
> you know.
> 
> To find the reason for NACK, we examined past proposals and the
> threads. And we came to understand that you oppose accessing vfsmount
> inside vfs helper functions. Is our understanding correct?
> 
> If our understanding is correct, we would like to propose a new
> method that does not require modifications to vfs helper functions.
> Attached patch is a trial of this method.
> 
> vfs helper functions are surrounded by mnt_want_write() and
> mnt_drop_write() pairs which receive "struct vfsmount" parameter
> since 2.6.26. So, by remembering the absolute pathname of "struct
> vfsmount" of the moment, LSM module can calculate an absolute
> pathname of the given "struct dentry" parameter inside vfs_*
> functions, without passing "struct vfsmount" parameter to vfs_*
> functions.
> 
> This approach doesn't access vfsmount inside vfs helper functions,
> and modification of existing kernel is only in task_struct and
> mnt_want/drop_write().

I don't know what Al Viro will say, but this is incredibly ugly.
Even more ugly than previous patches.

Re: (repost) Confirmation of methods for calculating requested pathname.

[Jamie Lokier]

Kentaro Takeda wrote:
> If our understanding is correct, we would like to propose a new
> method that does not require modifications to vfs helper functions.
> Attached patch is a trial of this method.
> 
> vfs helper functions are surrounded by mnt_want_write() and
> mnt_drop_write() pairs which receive "struct vfsmount" parameter
> since 2.6.26. So, by remembering the absolute pathname of "struct
> vfsmount" of the moment, LSM module can calculate an absolute
> pathname of the given "struct dentry" parameter inside vfs_*
> functions, without passing "struct vfsmount" parameter to vfs_*
> functions.
> 
> This approach doesn't access vfsmount inside vfs helper functions,
> and modification of existing kernel is only in task_struct and
> mnt_want/drop_write().

This is exactly the same as passing vfsmount through vfs helper functions,
except the parameter is "hidden" inside struct task.

Although this seems to violate the idea that the vfs helper call
chains (including functions they call) aren't affected by the mount
point, to be frank they are affected by the mount point _already_ - by
virtue of mnt_get_write()/mnt_drop_write() existing.

Ironically, the _current_ way that vfs helper call chains are affected
by mount point is for security: read-only mount points are basic security.

So I don't understand the objection to using path mount point in
security decisions, given that it already is to some extent.

This patch seems ugly at first because of hidden parameters in struct
task.  On the other hand, mnt_get_write() already does a similar thing
(and the implementation is uglier imho, though clever), and it is accepted.

-- Jamie

Re: (repost) Confirmation of methods for calculating requested pathname.

[Kentaro Takeda]

> I don't know what Al Viro will say, but this is incredibly ugly.
> Even more ugly than previous patches.
Which approach did you mean by the "previous patches"?

* introduce security_path_* (new LSM hooks)
http://marc.info/?l=linux-security-module&m=120962139014606&w=2

* pass struct path * to vfs helper functions and existing LSM hooks
http://lkml.org/lkml/2008/5/29/217

Regards,

Re: (repost) Confirmation of methods for calculating requested pathname.

[Serge E. Hallyn]

Quoting Kentaro Takeda (takedakn@nttdata.co.jp):
> Al, could you answer the following question?
> 
> 
> The current Linux kernel is not designed to pass vfsmount parameter
> that is crucial for pathname-based security including AppArmor and
> TOMOYO Linux, to LSM. Though both projects have been proposing
> patches to calculate pathname, none of them have been accepted as
> you know.
> 
> To find the reason for NACK, we examined past proposals and the
> threads. And we came to understand that you oppose accessing vfsmount
> inside vfs helper functions. Is our understanding correct?
> 
> If our understanding is correct, we would like to propose a new
> method that does not require modifications to vfs helper functions.
> Attached patch is a trial of this method.
> 
> vfs helper functions are surrounded by mnt_want_write() and
> mnt_drop_write() pairs which receive "struct vfsmount" parameter

I thought Al and others (Stephen?) had made it clear that the thing to do was
add new lsm hooks there.  So whereas inode_permission takes only an inode and
ends up calling security_inode_permission, you would add a
security_path_permission() or somesuch before or after the call to
inode_permission(), where as you've noted the path is available.  You're
*close* to doing the right thing by having a helper who is called at the right
place catch the vfsmount, but you refuse to send a patch doing exactly what
has been suggested.

-serge

Re: (repost) Confirmation of methods for calculating requested pathname.

[Tetsuo Handa]

Hello.

Serge E. Hallyn wrote:
> I thought Al and others (Stephen?) had made it clear that the thing to do was
> add new lsm hooks there.
Thank you for a hint.
I have a question to Al Viro: Is it acceptable for you to introduce
new LSM hooks (i.e. security_path_*()) where struct vfsmount is available?
If it is acceptable, I'd like to start from the following patch.

Regards.
-----
Subject: Introduce new LSM hooks.

This patch allows LSM to check permission using "struct vfsmount"
without passing "struct vfsmount" to VFS helper functions.

Signed-off-by: Kentaro Takeda <takedakn@nttdata.co.jp>
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Signed-off-by: Toshiharu Harada <haradats@nttdata.co.jp>
---
 fs/namei.c               |   37 ++++++++++++
 fs/open.c                |    5 +
 include/linux/security.h |  135 +++++++++++++++++++++++++++++++++++++++++++++++
 net/unix/af_unix.c       |    4 +
 security/capability.c    |   53 ++++++++++++++++++
 security/security.c      |   63 +++++++++++++++++++++
 6 files changed, 297 insertions(+)

--- linux-2.6.27-rc5.orig/fs/namei.c
+++ linux-2.6.27-rc5/fs/namei.c
@@ -1585,6 +1585,10 @@ 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,
+						       NULL);
 		if (!error) {
 			DQUOT_INIT(inode);
 
@@ -1615,7 +1619,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;
@@ -2018,6 +2026,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);
@@ -2030,6 +2041,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);
@@ -2089,7 +2101,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);
@@ -2196,7 +2212,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);
@@ -2278,7 +2298,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);
@@ -2359,7 +2383,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);
@@ -2456,7 +2484,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);
@@ -2688,8 +2720,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);
--- linux-2.6.27-rc5.orig/fs/open.c
+++ linux-2.6.27-rc5/fs/open.c
@@ -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, NULL);
 	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, file);
+	if (!error)
 		error = do_truncate(dentry, length, ATTR_MTIME|ATTR_CTIME, file);
 out_putf:
 	fput(file);
--- linux-2.6.27-rc5.orig/include/linux/security.h
+++ linux-2.6.27-rc5/include/linux/security.h
@@ -345,17 +345,37 @@ static inline void security_free_mnt_opt
  *	@dir contains the inode structure of the parent directory of the new link.
  *	@new_dentry contains the dentry structure for the new link.
  *	Return 0 if permission is granted.
+ * @path_link:
+ *	Check permission before creating a new hard link to a file.
+ *	@old_dentry contains the dentry structure for an existing link
+ *	to the file.
+ *	@new_dir contains the path structure of the parent directory of
+ *	the new link.
+ *	@new_dentry contains the dentry structure for the new link.
+ *	Return 0 if permission is granted.
  * @inode_unlink:
  *	Check the permission to remove a hard link to a file.
  *	@dir contains the inode structure of parent directory of the file.
  *	@dentry contains the dentry structure for file to be unlinked.
  *	Return 0 if permission is granted.
+ * @path_unlink:
+ *	Check the permission to remove a hard link to a file.
+ *	@dir contains the path structure of parent directory of the file.
+ *	@dentry contains the dentry structure for file to be unlinked.
+ *	Return 0 if permission is granted.
  * @inode_symlink:
  *	Check the permission to create a symbolic link to a file.
  *	@dir contains the inode structure of parent directory of the symbolic link.
  *	@dentry contains the dentry structure of the symbolic link.
  *	@old_name contains the pathname of file.
  *	Return 0 if permission is granted.
+ * @path_symlink:
+ *	Check the permission to create a symbolic link to a file.
+ *	@dir contains the path structure of parent directory of
+ *	the symbolic link.
+ *	@dentry contains the dentry structure of the symbolic link.
+ *	@old_name contains the pathname of file.
+ *	Return 0 if permission is granted.
  * @inode_mkdir:
  *	Check permissions to create a new directory in the existing directory
  *	associated with inode strcture @dir.
@@ -363,11 +383,25 @@ static inline void security_free_mnt_opt
  *	@dentry contains the dentry structure of new directory.
  *	@mode contains the mode of new directory.
  *	Return 0 if permission is granted.
+ * @path_mkdir:
+ *	Check permissions to create a new directory in the existing directory
+ *	associated with path strcture @path.
+ *	@dir containst the path structure of parent of the directory
+ *	to be created.
+ *	@dentry contains the dentry structure of new directory.
+ *	@mode contains the mode of new directory.
+ *	Return 0 if permission is granted.
  * @inode_rmdir:
  *	Check the permission to remove a directory.
  *	@dir contains the inode structure of parent of the directory to be removed.
  *	@dentry contains the dentry structure of directory to be removed.
  *	Return 0 if permission is granted.
+ * @path_rmdir:
+ *	Check the permission to remove a directory.
+ *	@dir contains the path structure of parent of the directory to be
+ *	removed.
+ *	@dentry contains the dentry structure of directory to be removed.
+ *	Return 0 if permission is granted.
  * @inode_mknod:
  *	Check permissions when creating a special file (or a socket or a fifo
  *	file created via the mknod system call).  Note that if mknod operation
@@ -378,6 +412,15 @@ static inline void security_free_mnt_opt
  *	@mode contains the mode of the new file.
  *	@dev contains the device number.
  *	Return 0 if permission is granted.
+ * @path_mknod:
+ *	Check permissions when creating a file. Note that this hook is called
+ *	even if mknod operation is being done for a regular file.
+ *	@dir contains the path structure of parent of the new file.
+ *	@dentry contains the dentry structure of the new file.
+ *	@mode contains the mode of the new file.
+ *	@dev contains the undecoded device number. Use new_decode_dev() to get
+ *	the decoded device number.
+ *	Return 0 if permission is granted.
  * @inode_rename:
  *	Check for permission to rename a file or directory.
  *	@old_dir contains the inode structure for parent of the old link.
@@ -385,6 +428,13 @@ static inline void security_free_mnt_opt
  *	@new_dir contains the inode structure for parent of the new link.
  *	@new_dentry contains the dentry structure of the new link.
  *	Return 0 if permission is granted.
+ * @path_rename:
+ *	Check for permission to rename a file or directory.
+ *	@old_dir contains the path structure for parent of the old link.
+ *	@old_dentry contains the dentry structure of the old link.
+ *	@new_dir contains the path structure for parent of the new link.
+ *	@new_dentry contains the dentry structure of the new link.
+ *	Return 0 if permission is granted.
  * @inode_readlink:
  *	Check the permission to read the symbolic link.
  *	@dentry contains the dentry structure for the file link.
@@ -413,6 +463,13 @@ static inline void security_free_mnt_opt
  *	@dentry contains the dentry structure for the file.
  *	@attr is the iattr structure containing the new file attributes.
  *	Return 0 if permission is granted.
+ * @path_truncate:
+ *	Check permission before truncating a file.
+ *	@path contains the path structure for the file.
+ *	@length is the new length of the file.
+ *	@time_attrs is the flags passed to do_truncate().
+ *	@filp is the file structure (may be NULL).
+ *	Return 0 if permission is granted.
  * @inode_getattr:
  *	Check permission before obtaining file attributes.
  *	@mnt is the vfsmount where the dentry was looked up
@@ -1350,6 +1407,20 @@ struct security_operations {
 				   struct super_block *newsb);
 	int (*sb_parse_opts_str) (char *options, struct security_mnt_opts *opts);
 
+	int (*path_unlink) (struct path *dir, struct dentry *dentry);
+	int (*path_mkdir) (struct path *dir, struct dentry *dentry, int mode);
+	int (*path_rmdir) (struct path *dir, struct dentry *dentry);
+	int (*path_mknod) (struct path *dir, struct dentry *dentry, int mode,
+			   unsigned int dev);
+	int (*path_truncate) (struct path *path, loff_t length,
+			      unsigned int time_attrs, struct file *filp);
+	int (*path_symlink) (struct path *dir, struct dentry *dentry,
+			     const char *old_name);
+	int (*path_link) (struct dentry *old_dentry, struct path *new_dir,
+			  struct dentry *new_dentry);
+	int (*path_rename) (struct path *old_dir, struct dentry *old_dentry,
+			    struct path *new_dir, struct dentry *new_dentry);
+
 	int (*inode_alloc_security) (struct inode *inode);
 	void (*inode_free_security) (struct inode *inode);
 	int (*inode_init_security) (struct inode *inode, struct inode *dir,
@@ -1618,6 +1689,20 @@ void security_sb_clone_mnt_opts(const st
 				struct super_block *newsb);
 int security_sb_parse_opts_str(char *options, struct security_mnt_opts *opts);
 
+int security_path_unlink(struct path *dir, struct dentry *dentry);
+int security_path_mkdir(struct path *dir, struct dentry *dentry, int mode);
+int security_path_rmdir(struct path *dir, struct dentry *dentry);
+int security_path_mknod(struct path *dir, struct dentry *dentry, int mode,
+			unsigned int dev);
+int security_path_truncate(struct path *path, loff_t length,
+			   unsigned int time_attrs, struct file *filp);
+int security_path_symlink(struct path *dir, struct dentry *dentry,
+			  const char *old_name);
+int security_path_link(struct dentry *old_dentry, struct path *new_dir,
+		       struct dentry *new_dentry);
+int security_path_rename(struct path *old_dir, struct dentry *old_dentry,
+			 struct path *new_dir, struct dentry *new_dentry);
+
 int security_inode_alloc(struct inode *inode);
 void security_inode_free(struct inode *inode);
 int security_inode_init_security(struct inode *inode, struct inode *dir,
@@ -1949,6 +2034,56 @@ static inline int security_sb_parse_opts
 	return 0;
 }
 
+static inline int security_path_unlink(struct path *dir, struct dentry *dentry)
+{
+	return 0;
+}
+
+static inline int security_path_mkdir(struct path *dir, struct dentry *dentry,
+				      int mode)
+{
+	return 0;
+}
+
+static inline int security_path_rmdir(struct path *dir, struct dentry *dentry)
+{
+	return 0;
+}
+
+static inline int security_path_mknod(struct path *dir, struct dentry *dentry,
+				      int mode, unsigned int dev)
+{
+	return 0;
+}
+
+static inline int security_path_truncate(struct path *path, loff_t length,
+					 unsigned int time_attrs,
+					 struct file *filp)
+{
+	return 0;
+}
+
+static inline int security_path_symlink(struct path *dir, struct dentry *dentry,
+					const char *old_name)
+{
+	return 0;
+}
+
+static inline int security_path_link(struct dentry *old_dentry,
+				     struct path *new_dir,
+				     struct dentry *new_dentry)
+{
+	return 0;
+}
+
+static inline int security_path_rename(struct path *old_dir,
+				       struct dentry *old_dentry,
+				       struct path *new_dir,
+				       struct dentry *new_dentry)
+{
+	return 0;
+}
+
 static inline int security_inode_alloc(struct inode *inode)
 {
 	return 0;
--- linux-2.6.27-rc5.orig/net/unix/af_unix.c
+++ linux-2.6.27-rc5/net/unix/af_unix.c
@@ -827,7 +827,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;
--- linux-2.6.27-rc5.orig/security/capability.c
+++ linux-2.6.27-rc5/security/capability.c
@@ -268,6 +268,51 @@ static void cap_inode_getsecid(const str
 	*secid = 0;
 }
 
+static int cap_path_mknod(struct path *dir, struct dentry *dentry, int mode,
+			  unsigned int dev)
+{
+	return 0;
+}
+
+static int cap_path_mkdir(struct path *dir, struct dentry *dentry, int mode)
+{
+	return 0;
+}
+
+static int cap_path_rmdir(struct path *dir, struct dentry *dentry)
+{
+	return 0;
+}
+
+static int cap_path_unlink(struct path *dir, struct dentry *dentry)
+{
+	return 0;
+}
+
+static int cap_path_symlink(struct path *dir, struct dentry *dentry,
+			    const char *old_name)
+{
+	return 0;
+}
+
+static int cap_path_link(struct dentry *old_dentry, struct path *new_dir,
+			 struct dentry *new_dentry)
+{
+	return 0;
+}
+
+static int cap_path_rename(struct path *old_path, struct dentry *old_dentry,
+			   struct path *new_path, struct dentry *new_dentry)
+{
+	return 0;
+}
+
+static int cap_path_truncate(struct path *path, loff_t length,
+			     unsigned int time_attrs, struct file *filp)
+{
+	return 0;
+}
+
 static int cap_file_permission(struct file *file, int mask)
 {
 	return 0;
@@ -877,6 +922,14 @@ void security_fixup_ops(struct security_
 	set_to_cap_if_null(ops, inode_setsecurity);
 	set_to_cap_if_null(ops, inode_listsecurity);
 	set_to_cap_if_null(ops, inode_getsecid);
+	set_to_cap_if_null(ops, path_mknod);
+	set_to_cap_if_null(ops, path_mkdir);
+	set_to_cap_if_null(ops, path_rmdir);
+	set_to_cap_if_null(ops, path_unlink);
+	set_to_cap_if_null(ops, path_symlink);
+	set_to_cap_if_null(ops, path_link);
+	set_to_cap_if_null(ops, path_rename);
+	set_to_cap_if_null(ops, path_truncate);
 	set_to_cap_if_null(ops, file_permission);
 	set_to_cap_if_null(ops, file_alloc_security);
 	set_to_cap_if_null(ops, file_free_security);
--- linux-2.6.27-rc5.orig/security/security.c
+++ linux-2.6.27-rc5/security/security.c
@@ -358,6 +358,69 @@ int security_inode_init_security(struct 
 }
 EXPORT_SYMBOL(security_inode_init_security);
 
+int security_path_mknod(struct path *path, struct dentry *dentry, int mode,
+			unsigned int dev)
+{
+	if (unlikely(IS_PRIVATE(path->dentry->d_inode)))
+		return 0;
+	return security_ops->path_mknod(path, dentry, mode, dev);
+}
+
+int security_path_mkdir(struct path *path, struct dentry *dentry, int mode)
+{
+	if (unlikely(IS_PRIVATE(path->dentry->d_inode)))
+		return 0;
+	return security_ops->path_mkdir(path, dentry, mode);
+}
+
+int security_path_rmdir(struct path *path, struct dentry *dentry)
+{
+	if (unlikely(IS_PRIVATE(path->dentry->d_inode)))
+		return 0;
+	return security_ops->path_rmdir(path, dentry);
+}
+
+int security_path_unlink(struct path *path, struct dentry *dentry)
+{
+	if (unlikely(IS_PRIVATE(path->dentry->d_inode)))
+		return 0;
+	return security_ops->path_unlink(path, dentry);
+}
+
+int security_path_symlink(struct path *path, struct dentry *dentry,
+			  const char *old_name)
+{
+	if (unlikely(IS_PRIVATE(path->dentry->d_inode)))
+		return 0;
+	return security_ops->path_symlink(path, dentry, old_name);
+}
+
+int security_path_link(struct dentry *old_dentry, struct path *new_dir,
+		       struct dentry *new_dentry)
+{
+	if (unlikely(IS_PRIVATE(old_dentry->d_inode)))
+		return 0;
+	return security_ops->path_link(old_dentry, new_dir, new_dentry);
+}
+
+int security_path_rename(struct path *old_dir, struct dentry *old_dentry,
+			 struct path *new_dir, struct dentry *new_dentry)
+{
+	if (unlikely(IS_PRIVATE(old_dentry->d_inode) ||
+		     (new_dentry->d_inode && IS_PRIVATE(new_dentry->d_inode))))
+		return 0;
+	return security_ops->path_rename(old_dir, old_dentry, new_dir,
+					 new_dentry);
+}
+
+int security_path_truncate(struct path *path, loff_t length,
+			   unsigned int time_attrs, struct file *filp)
+{
+	if (unlikely(IS_PRIVATE(path->dentry->d_inode)))
+		return 0;
+	return security_ops->path_truncate(path, length, time_attrs, filp);
+}
+
 int security_inode_create(struct inode *dir, struct dentry *dentry, int mode)
 {
 	if (unlikely(IS_PRIVATE(dir)))

Re: (repost) Confirmation of methods for calculating requested pathname.

[Stephen Smalley]

On Tue, 2008-09-02 at 08:11 -0500, Serge E. Hallyn wrote:
> Quoting Kentaro Takeda (takedakn@nttdata.co.jp):
> > Al, could you answer the following question?
> > 
> > 
> > The current Linux kernel is not designed to pass vfsmount parameter
> > that is crucial for pathname-based security including AppArmor and
> > TOMOYO Linux, to LSM. Though both projects have been proposing
> > patches to calculate pathname, none of them have been accepted as
> > you know.
> > 
> > To find the reason for NACK, we examined past proposals and the
> > threads. And we came to understand that you oppose accessing vfsmount
> > inside vfs helper functions. Is our understanding correct?
> > 
> > If our understanding is correct, we would like to propose a new
> > method that does not require modifications to vfs helper functions.
> > Attached patch is a trial of this method.
> > 
> > vfs helper functions are surrounded by mnt_want_write() and
> > mnt_drop_write() pairs which receive "struct vfsmount" parameter
> 
> I thought Al and others (Stephen?) had made it clear that the thing to do was
> add new lsm hooks there.  So whereas inode_permission takes only an inode and
> ends up calling security_inode_permission, you would add a
> security_path_permission() or somesuch before or after the call to
> inode_permission(), where as you've noted the path is available.  You're
> *close* to doing the right thing by having a helper who is called at the right
> place catch the vfsmount, but you refuse to send a patch doing exactly what
> has been suggested.

No, that idea seemingly died because both Al and Miklos thought it was
wrong to add new security hooks to the same code path (vs. moving the
existing ones to the callers), but I was opposed to moving the existing
hooks as they are presently exactly where SELinux needs them, and moving
them to the callers raises concerns both with ensuring invocation on all
code paths (possible, but an ongoing maintenance concern) and with
performing DAC checks first where possible (which I think is also a
concern for TOMOYO et al).  Miklos' vfs path API showed more promise but
was rejected.

-- 
Stephen Smalley
National Security Agency

Re: (repost) Confirmation of methods for calculating requested pathname.

[Miklos Szeredi]

On Tue, 02 Sep 2008, Stephen Smalley wrote:
> No, that idea seemingly died because both Al and Miklos thought it was
> wrong to add new security hooks to the same code path (vs. moving the
> existing ones to the callers),

I do think that duplicating hooks is the wrong approach, but it does
have the minimal impact of all the solutions, and the duplication can
be consolidated later.  So in the end this might be the easiest way
forward.

What Al is violently opposed to is removing the vfs_foo() API which
separates the "VFS core", which doesn't know anything abount mounts,
from the rest of the VFS.

But as Jamie pointed out, we've already sneaked in vfsmounts via the
r/o bind mounts patches.  I don't see why we would need to strictly
separate the mount namespace handling from the VFS core.

Miklos