Re: [AppArmor 19/45] Add struct vfsmount parameters to vfs_rename()

Re: [AppArmor 19/45] Add struct vfsmount parameters to vfs_rename()

[Bodo Eggert]

Al Viro <viro@ftp.linux.org.uk> wrote:
> On Fri, Oct 26, 2007 at 11:23:53AM -0700, John Johansen wrote:

>> In the current code, both vfsmounts are always identical, and so one of
>> the two should go, agreed.
>> 
>> The thought behind passing both vfsmounts was that they could differ but
>> point to the same super_block, in which case renames would still be
>> possible at least from a filesystem point of view. The essential
>> restriction here is that both files must be on the same device; the vfs
>> restriction of not allowing cross-mount renames is arbitrary.
> 
> It's called "access control".  Pathname-based one, BTW.  And yes, it's
> 100% deliberate.

I doubt anybody uses bind mounts & co instead of symlinks in order to
prevent rename() while still allowing to move files by either copying
or by using the source file in the bound directory. At least I expected
bind mounted directories to behave like symlinked ones, minus the problems
of symlinks. 

Since this feature only protects you from rename(src/foo,dst/foo) if
1) There is no way to access src and dst in the same mount space
2) src and dst are writebale by the attacker
3) Unlinking src/foo is OK
4) Renaming src/foo is OK as long as it's within the same mount as foo
5) Symlinking src/foo to dst/foo is OK
6) Creating dst/foo having a different owner is OK
7) Having dst/foo with the original content and owner from src/foo is _not_ OK
8) Moon crashes on earth
, I'd rather like to have a fast mv.

>> Cross-mount renames are not allowed currently, and granted, they may not
>> be very useful, either.
> 
> <raised brows>
> Excuse me, but IIRC LSM was supposed to _add_ restrictions, not to remove
> existing security checks.

Security checks as in "we built a steel door into that Chinese paper wall"?

As far as I understand, the restriction would not be removed by the LSM
explicitely allowing it, but by the fixed vfs then being able to handle
cross-mountpoint-renames. Maybe yo'll want to keep the ability for the users
who use bind mounts in order to not allow rename() ... both of them.-)

/me prepares for the impact of a large round object on earth.

Re: [AppArmor 19/45] Add struct vfsmount parameters to vfs_rename()

[Peter Zijlstra]

On Fri, 2007-11-02 at 11:14 +0100, Bodo Eggert wrote:

> /me prepares for the impact of a large round object on earth.

/me ponders the implications of the impact of a 2 dimensional object
colliding with a 3 dimensional object in real life.

[AppArmor 00/45] AppArmor security module overview

[jjohansen]

-- 

[AppArmor 19/45] Add struct vfsmount parameters to vfs_rename()

[jjohansen]

[-- Attachment #1: vfs-rename.diff --]
[-- Type: text/plain, Size: 4748 bytes --]

The vfsmount will be passed down to the LSM hook so that LSMs can compute
pathnames.

Signed-off-by: Tony Jones <tonyj@suse.de>
Signed-off-by: Andreas Gruenbacher <agruen@suse.de>
Signed-off-by: John Johansen <jjohansen@suse.de>

---
 fs/ecryptfs/inode.c |    7 ++++++-
 fs/namei.c          |   19 ++++++++++++-------
 fs/nfsd/vfs.c       |    3 ++-
 include/linux/fs.h  |    2 +-
 4 files changed, 21 insertions(+), 10 deletions(-)

--- a/fs/ecryptfs/inode.c
+++ b/fs/ecryptfs/inode.c
@@ -589,19 +589,24 @@ ecryptfs_rename(struct inode *old_dir, s
 {
 	int rc;
 	struct dentry *lower_old_dentry;
+	struct vfsmount *lower_old_mnt;
 	struct dentry *lower_new_dentry;
+	struct vfsmount *lower_new_mnt;
 	struct dentry *lower_old_dir_dentry;
 	struct dentry *lower_new_dir_dentry;
 
 	lower_old_dentry = ecryptfs_dentry_to_lower(old_dentry);
+	lower_old_mnt = ecryptfs_dentry_to_lower_mnt(old_dentry);
 	lower_new_dentry = ecryptfs_dentry_to_lower(new_dentry);
+	lower_new_mnt = ecryptfs_dentry_to_lower_mnt(new_dentry);
 	dget(lower_old_dentry);
 	dget(lower_new_dentry);
 	lower_old_dir_dentry = dget_parent(lower_old_dentry);
 	lower_new_dir_dentry = dget_parent(lower_new_dentry);
 	lock_rename(lower_old_dir_dentry, lower_new_dir_dentry);
 	rc = vfs_rename(lower_old_dir_dentry->d_inode, lower_old_dentry,
-			lower_new_dir_dentry->d_inode, lower_new_dentry);
+			lower_old_mnt, lower_new_dir_dentry->d_inode,
+			lower_new_dentry, lower_new_mnt);
 	if (rc)
 		goto out_lock;
 	fsstack_copy_attr_all(new_dir, lower_new_dir_dentry->d_inode);
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2513,7 +2513,8 @@ asmlinkage long sys_link(const char __us
  *	   locking].
  */
 static int vfs_rename_dir(struct inode *old_dir, struct dentry *old_dentry,
-			  struct inode *new_dir, struct dentry *new_dentry)
+			  struct vfsmount *old_mnt, struct inode *new_dir,
+			  struct dentry *new_dentry, struct vfsmount *new_mnt)
 {
 	int error = 0;
 	struct inode *target;
@@ -2556,7 +2557,8 @@ static int vfs_rename_dir(struct inode *
 }
 
 static int vfs_rename_other(struct inode *old_dir, struct dentry *old_dentry,
-			    struct inode *new_dir, struct dentry *new_dentry)
+			    struct vfsmount *old_mnt, struct inode *new_dir,
+			    struct dentry *new_dentry, struct vfsmount *new_mnt)
 {
 	struct inode *target;
 	int error;
@@ -2584,7 +2586,8 @@ static int vfs_rename_other(struct inode
 }
 
 int vfs_rename(struct inode *old_dir, struct dentry *old_dentry,
-	       struct inode *new_dir, struct dentry *new_dentry)
+	        struct vfsmount *old_mnt, struct inode *new_dir,
+	        struct dentry *new_dentry, struct vfsmount *new_mnt)
 {
 	int error;
 	int is_dir = S_ISDIR(old_dentry->d_inode->i_mode);
@@ -2613,9 +2616,11 @@ int vfs_rename(struct inode *old_dir, st
 	old_name = fsnotify_oldname_init(old_dentry->d_name.name);
 
 	if (is_dir)
-		error = vfs_rename_dir(old_dir,old_dentry,new_dir,new_dentry);
+		error = vfs_rename_dir(old_dir, old_dentry, old_mnt,
+				       new_dir, new_dentry, new_mnt);
 	else
-		error = vfs_rename_other(old_dir,old_dentry,new_dir,new_dentry);
+		error = vfs_rename_other(old_dir, old_dentry, old_mnt,
+					 new_dir, new_dentry, new_mnt);
 	if (!error) {
 		const char *new_name = old_dentry->d_name.name;
 		fsnotify_move(old_dir, new_dir, old_name, new_name, is_dir,
@@ -2690,8 +2695,8 @@ static int do_rename(int olddfd, const c
 	error = mnt_want_write(oldnd.mnt);
 	if (error)
 		goto exit5;
-	error = vfs_rename(old_dir->d_inode, old_dentry,
-				   new_dir->d_inode, new_dentry);
+	error = vfs_rename(old_dir->d_inode, old_dentry, oldnd.mnt,
+			   new_dir->d_inode, new_dentry, newnd.mnt);
 	mnt_drop_write(oldnd.mnt);
 exit5:
 	dput(new_dentry);
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -1694,7 +1694,8 @@ nfsd_rename(struct svc_rqst *rqstp, stru
 	if (host_err)
 		goto out_dput_new;
 
-	host_err = vfs_rename(fdir, odentry, tdir, ndentry);
+	host_err = vfs_rename(fdir, odentry, ffhp->fh_export->ex_mnt,
+			      tdir, ndentry, tfhp->fh_export->ex_mnt);
 	if (!host_err && EX_ISSYNC(tfhp->fh_export)) {
 		host_err = nfsd_sync_dir(tdentry);
 		if (!host_err)
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1080,7 +1080,7 @@ extern int vfs_symlink(struct inode *, s
 extern int vfs_link(struct dentry *, struct vfsmount *, struct inode *, struct dentry *, struct vfsmount *);
 extern int vfs_rmdir(struct inode *, struct dentry *, struct vfsmount *);
 extern int vfs_unlink(struct inode *, struct dentry *, struct vfsmount *);
-extern int vfs_rename(struct inode *, struct dentry *, struct inode *, struct dentry *);
+extern int vfs_rename(struct inode *, struct dentry *, struct vfsmount *, struct inode *, struct dentry *, struct vfsmount *);
 
 /*
  * VFS dentry helper functions.

-- 

Re: [AppArmor 19/45] Add struct vfsmount parameters to vfs_rename()

[Al Viro]

On Thu, Oct 25, 2007 at 11:40:43PM -0700, jjohansen@suse.de wrote:
> The vfsmount will be passed down to the LSM hook so that LSMs can compute
> pathnames.

You know, you really are supposed to understand the code you are modifying...
Quiz: what are those vfsmounts and how are they related?

Al, carefully abstaining from saying what he really thinks of LSM and its
users...

Re: [AppArmor 19/45] Add struct vfsmount parameters to vfs_rename()

[John Johansen]

[-- Attachment #1: Type: text/plain, Size: 1081 bytes --]

On Fri, Oct 26, 2007 at 08:37:49AM +0100, Al Viro wrote:
> On Thu, Oct 25, 2007 at 11:40:43PM -0700, jjohansen@suse.de wrote:
> > The vfsmount will be passed down to the LSM hook so that LSMs can compute
> > pathnames.
> 
> You know, you really are supposed to understand the code you are modifying...
> Quiz: what are those vfsmounts and how are they related?
> 

In the current code, both vfsmounts are always identical, and so one of
the two should go, agreed.

The thought behind passing both vfsmounts was that they could differ but
point to the same super_block, in which case renames would still be
possible at least from a filesystem point of view. The essential
restriction here is that both files must be on the same device; the vfs
restriction of not allowing cross-mount renames is arbitrary.
Cross-mount renames are not allowed currently, and granted, they may not
be very useful, either.

> Al, carefully abstaining from saying what he really thinks of LSM and its
> users...

As always, it's a pleasure to see the genuine Viro charm at play.

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [AppArmor 19/45] Add struct vfsmount parameters to vfs_rename()

[Al Viro]

On Fri, Oct 26, 2007 at 11:23:53AM -0700, John Johansen wrote:

> In the current code, both vfsmounts are always identical, and so one of
> the two should go, agreed.
> 
> The thought behind passing both vfsmounts was that they could differ but
> point to the same super_block, in which case renames would still be
> possible at least from a filesystem point of view. The essential
> restriction here is that both files must be on the same device; the vfs
> restriction of not allowing cross-mount renames is arbitrary.

It's called "access control".  Pathname-based one, BTW.  And yes, it's
100% deliberate.

> Cross-mount renames are not allowed currently, and granted, they may not
> be very useful, either.

<raised brows>
Excuse me, but IIRC LSM was supposed to _add_ restrictions, not to remove
existing security checks.

[AppArmor 00/45] AppArmor security module overview

[jjohansen]

lkml-explanatory.txt

-- 

[AppArmor 19/45] Add struct vfsmount parameters to vfs_rename()

[jjohansen]

[-- Attachment #1: vfs-rename.diff --]
[-- Type: text/plain, Size: 4708 bytes --]

The vfsmount will be passed down to the LSM hook so that LSMs can compute
pathnames.

Signed-off-by: Tony Jones <tonyj@suse.de>
Signed-off-by: Andreas Gruenbacher <agruen@suse.de>
Signed-off-by: John Johansen <jjohansen@suse.de>

---
 fs/ecryptfs/inode.c |    7 ++++++-
 fs/namei.c          |   19 ++++++++++++-------
 fs/nfsd/vfs.c       |    3 ++-
 include/linux/fs.h  |    2 +-
 4 files changed, 21 insertions(+), 10 deletions(-)

--- a/fs/ecryptfs/inode.c
+++ b/fs/ecryptfs/inode.c
@@ -598,19 +598,24 @@ ecryptfs_rename(struct inode *old_dir, s
 {
 	int rc;
 	struct dentry *lower_old_dentry;
+	struct vfsmount *lower_old_mnt;
 	struct dentry *lower_new_dentry;
+	struct vfsmount *lower_new_mnt;
 	struct dentry *lower_old_dir_dentry;
 	struct dentry *lower_new_dir_dentry;
 
 	lower_old_dentry = ecryptfs_dentry_to_lower(old_dentry);
+	lower_old_mnt = ecryptfs_dentry_to_lower_mnt(old_dentry);
 	lower_new_dentry = ecryptfs_dentry_to_lower(new_dentry);
+	lower_new_mnt = ecryptfs_dentry_to_lower_mnt(new_dentry);
 	dget(lower_old_dentry);
 	dget(lower_new_dentry);
 	lower_old_dir_dentry = dget_parent(lower_old_dentry);
 	lower_new_dir_dentry = dget_parent(lower_new_dentry);
 	lock_rename(lower_old_dir_dentry, lower_new_dir_dentry);
 	rc = vfs_rename(lower_old_dir_dentry->d_inode, lower_old_dentry,
-			lower_new_dir_dentry->d_inode, lower_new_dentry);
+			lower_old_mnt, lower_new_dir_dentry->d_inode,
+			lower_new_dentry, lower_new_mnt);
 	if (rc)
 		goto out_lock;
 	fsstack_copy_attr_all(new_dir, lower_new_dir_dentry->d_inode, NULL);
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2401,7 +2401,8 @@ asmlinkage long sys_link(const char __us
  *	   locking].
  */
 static int vfs_rename_dir(struct inode *old_dir, struct dentry *old_dentry,
-			  struct inode *new_dir, struct dentry *new_dentry)
+			  struct vfsmount *old_mnt, struct inode *new_dir,
+			  struct dentry *new_dentry, struct vfsmount *new_mnt)
 {
 	int error = 0;
 	struct inode *target;
@@ -2444,7 +2445,8 @@ static int vfs_rename_dir(struct inode *
 }
 
 static int vfs_rename_other(struct inode *old_dir, struct dentry *old_dentry,
-			    struct inode *new_dir, struct dentry *new_dentry)
+			    struct vfsmount *old_mnt, struct inode *new_dir,
+			    struct dentry *new_dentry, struct vfsmount *new_mnt)
 {
 	struct inode *target;
 	int error;
@@ -2472,7 +2474,8 @@ static int vfs_rename_other(struct inode
 }
 
 int vfs_rename(struct inode *old_dir, struct dentry *old_dentry,
-	       struct inode *new_dir, struct dentry *new_dentry)
+	        struct vfsmount *old_mnt, struct inode *new_dir,
+	        struct dentry *new_dentry, struct vfsmount *new_mnt)
 {
 	int error;
 	int is_dir = S_ISDIR(old_dentry->d_inode->i_mode);
@@ -2501,9 +2504,11 @@ int vfs_rename(struct inode *old_dir, st
 	old_name = fsnotify_oldname_init(old_dentry->d_name.name);
 
 	if (is_dir)
-		error = vfs_rename_dir(old_dir,old_dentry,new_dir,new_dentry);
+		error = vfs_rename_dir(old_dir, old_dentry, old_mnt,
+				       new_dir, new_dentry, new_mnt);
 	else
-		error = vfs_rename_other(old_dir,old_dentry,new_dir,new_dentry);
+		error = vfs_rename_other(old_dir, old_dentry, old_mnt,
+					 new_dir, new_dentry, new_mnt);
 	if (!error) {
 		const char *new_name = old_dentry->d_name.name;
 		fsnotify_move(old_dir, new_dir, old_name, new_name, is_dir,
@@ -2575,8 +2580,8 @@ static int do_rename(int olddfd, const c
 	if (new_dentry == trap)
 		goto exit5;
 
-	error = vfs_rename(old_dir->d_inode, old_dentry,
-				   new_dir->d_inode, new_dentry);
+	error = vfs_rename(old_dir->d_inode, old_dentry, oldnd.mnt,
+			   new_dir->d_inode, new_dentry, newnd.mnt);
 exit5:
 	dput(new_dentry);
 exit4:
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -1630,7 +1630,8 @@ nfsd_rename(struct svc_rqst *rqstp, stru
 			host_err = -EPERM;
 	} else
 #endif
-	host_err = vfs_rename(fdir, odentry, tdir, ndentry);
+	host_err = vfs_rename(fdir, odentry, ffhp->fh_export->ex_mnt,
+			      tdir, ndentry, tfhp->fh_export->ex_mnt);
 	if (!host_err && EX_ISSYNC(tfhp->fh_export)) {
 		host_err = nfsd_sync_dir(tdentry);
 		if (!host_err)
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -998,7 +998,7 @@ extern int vfs_symlink(struct inode *, s
 extern int vfs_link(struct dentry *, struct vfsmount *, struct inode *, struct dentry *, struct vfsmount *);
 extern int vfs_rmdir(struct inode *, struct dentry *, struct vfsmount *);
 extern int vfs_unlink(struct inode *, struct dentry *, struct vfsmount *);
-extern int vfs_rename(struct inode *, struct dentry *, struct inode *, struct dentry *);
+extern int vfs_rename(struct inode *, struct dentry *, struct vfsmount *, struct inode *, struct dentry *, struct vfsmount *);
 
 /*
  * VFS dentry helper functions.

--