linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [AppArmor 19/45] Add struct vfsmount parameters to vfs_rename()
       [not found]       ` <9iE8n-3zk-1@gated-at.bofh.it>
@ 2007-11-02 10:14         ` Bodo Eggert
  2007-11-02 12:28           ` Peter Zijlstra
  0 siblings, 1 reply; 7+ messages in thread
From: Bodo Eggert @ 2007-11-02 10:14 UTC (permalink / raw)
  To: Al Viro, John Johansen, akpm, linux-kernel,
	linux-security-module, Tony Jones, Andreas Gruenbacher

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.


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [AppArmor 19/45] Add struct vfsmount parameters to vfs_rename()
  2007-11-02 10:14         ` [AppArmor 19/45] Add struct vfsmount parameters to vfs_rename() Bodo Eggert
@ 2007-11-02 12:28           ` Peter Zijlstra
  0 siblings, 0 replies; 7+ messages in thread
From: Peter Zijlstra @ 2007-11-02 12:28 UTC (permalink / raw)
  To: 7eggert
  Cc: Al Viro, John Johansen, akpm, linux-kernel,
	linux-security-module, Tony Jones, Andreas Gruenbacher

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.


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [AppArmor 19/45] Add struct vfsmount parameters to vfs_rename()
  2007-10-26 18:23     ` John Johansen
@ 2007-10-26 20:33       ` Al Viro
  0 siblings, 0 replies; 7+ messages in thread
From: Al Viro @ 2007-10-26 20:33 UTC (permalink / raw)
  To: John Johansen
  Cc: akpm, linux-kernel, linux-security-module, Tony Jones,
	Andreas Gruenbacher

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.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [AppArmor 19/45] Add struct vfsmount parameters to vfs_rename()
  2007-10-26  7:37   ` Al Viro
@ 2007-10-26 18:23     ` John Johansen
  2007-10-26 20:33       ` Al Viro
  0 siblings, 1 reply; 7+ messages in thread
From: John Johansen @ 2007-10-26 18:23 UTC (permalink / raw)
  To: Al Viro
  Cc: jjohansen, akpm, linux-kernel, linux-security-module, Tony Jones,
	Andreas Gruenbacher

[-- 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 --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [AppArmor 19/45] Add struct vfsmount parameters to vfs_rename()
  2007-10-26  6:40 ` [AppArmor 19/45] Add struct vfsmount parameters to vfs_rename() jjohansen
@ 2007-10-26  7:37   ` Al Viro
  2007-10-26 18:23     ` John Johansen
  0 siblings, 1 reply; 7+ messages in thread
From: Al Viro @ 2007-10-26  7:37 UTC (permalink / raw)
  To: jjohansen
  Cc: akpm, linux-kernel, linux-security-module, Tony Jones,
	Andreas Gruenbacher

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...

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [AppArmor 19/45] Add struct vfsmount parameters to vfs_rename()
  2007-10-26  6:40 [AppArmor 00/45] AppArmor security module overview jjohansen
@ 2007-10-26  6:40 ` jjohansen
  2007-10-26  7:37   ` Al Viro
  0 siblings, 1 reply; 7+ messages in thread
From: jjohansen @ 2007-10-26  6:40 UTC (permalink / raw)
  To: akpm
  Cc: linux-kernel, linux-security-module, Tony Jones,
	Andreas Gruenbacher, John Johansen

[-- 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.

-- 


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [AppArmor 19/45] Add struct vfsmount parameters to vfs_rename()
  2007-05-14 11:06 [AppArmor 00/45] AppArmor security module overview jjohansen
@ 2007-05-14 11:06 ` jjohansen
  0 siblings, 0 replies; 7+ messages in thread
From: jjohansen @ 2007-05-14 11:06 UTC (permalink / raw)
  To: linux-kernel
  Cc: linux-security-module, linux-fsdevel, Tony Jones,
	Andreas Gruenbacher, John Johansen

[-- 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.

-- 

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2007-11-02 12:28 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <9irkY-83N-5@gated-at.bofh.it>
     [not found] ` <9iruz-7b-23@gated-at.bofh.it>
     [not found]   ` <9irXD-Jf-25@gated-at.bofh.it>
     [not found]     ` <9iC6y-nT-1@gated-at.bofh.it>
     [not found]       ` <9iE8n-3zk-1@gated-at.bofh.it>
2007-11-02 10:14         ` [AppArmor 19/45] Add struct vfsmount parameters to vfs_rename() Bodo Eggert
2007-11-02 12:28           ` Peter Zijlstra
2007-10-26  6:40 [AppArmor 00/45] AppArmor security module overview jjohansen
2007-10-26  6:40 ` [AppArmor 19/45] Add struct vfsmount parameters to vfs_rename() jjohansen
2007-10-26  7:37   ` Al Viro
2007-10-26 18:23     ` John Johansen
2007-10-26 20:33       ` Al Viro
  -- strict thread matches above, loose matches on Subject: below --
2007-05-14 11:06 [AppArmor 00/45] AppArmor security module overview jjohansen
2007-05-14 11:06 ` [AppArmor 19/45] Add struct vfsmount parameters to vfs_rename() jjohansen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).