Re: graft_tree/attach_mnt rfc

chrisat_private

* Serge E. Hallyn (hallyn@mpi-cbg.de) wrote:
> I've written a patch (attached) to move the check_sb and post_addmount 
> hooks from graft_tree into attach_mnt.  If we were to actually want to 
> do this, then we probably want to rename these hooks.

Yes, I agree.  I did basically the same in a patch I started before I
went MIA ;-)  I attached the patch just for grins.  It's basically the
same (not as complete), however I opted not to change the attach_mnt
prototype.  Also, I believe for symmetry, we should record the detaches,
otherwise you get a sort of leak in the module's view of a namespace.

> Please notice the need for a new fs/namespace.c function 
> (undo_detach_mnt, which could be done by adding a flag to attach_mnt to 
> skip security checks).

I understand why you did this, but it is clunky and will not go over
well I'm afraid.

> Do people feel this is worth doing?

I do think it is worth doing.  I guess it comes down to which namespace
operations you (and others) need to mediate and how.  do_kern_mount
hooks is clearly sufficient for SELinux, and gives each superblock a
label.  Is attaching a tree to the namespace something that needs to be
mediated, or simply recorded?

> The codepaths which an attach_mnt 
> hook catch, which graft_tree does not, are:
> 
> 1. copy_tree:  The validity of the mountpoints being copied were already 
> checked at prior
> mount.
> 
> (In DTE's case, these checks would simply return immediately, as the
> superblock will already have been set up.)
> 
> 2. do_movemount.  Currently protected by sb_mount.
> 
> 3. pivot_root:  Currently protected by sb_pivotroot.

This one I basically punted on in my patch, I suspect you can reason why ;-)

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

--- namespace.c.1.28	Tue Sep 24 14:55:47 2002
+++ namespace.c.fixup	Mon Aug  5 14:36:02 2002
@@ -91,32 +91,35 @@
 	while (mnt->mnt_parent != mnt)
 		mnt = mnt->mnt_parent;
 	spin_unlock(&dcache_lock);
 	return mnt == current->namespace->root;
 }
 
 static void detach_mnt(struct vfsmount *mnt, struct nameidata *old_nd)
 {
+	/* XXX dcache lock held */
+	security_ops->sb_umount_close(mnt);
 	old_nd->dentry = mnt->mnt_mountpoint;
 	old_nd->mnt = mnt->mnt_parent;
 	mnt->mnt_parent = mnt;
 	mnt->mnt_mountpoint = mnt->mnt_root;
 	list_del_init(&mnt->mnt_child);
 	list_del_init(&mnt->mnt_hash);
 	old_nd->dentry->d_mounted--;
 }
 
 static void attach_mnt(struct vfsmount *mnt, struct nameidata *nd)
 {
 	mnt->mnt_parent = mntget(nd->mnt);
 	mnt->mnt_mountpoint = dget(nd->dentry);
 	list_add(&mnt->mnt_hash, mount_hashtable+hash(nd->mnt, nd->dentry));
 	list_add(&mnt->mnt_child, &nd->mnt->mnt_mounts);
 	nd->dentry->d_mounted++;
+	security_ops->sb_post_addmount(mnt, nd);
 }
 
 static struct vfsmount *next_mnt(struct vfsmount *p, struct vfsmount *root)
 {
 	struct list_head *next = p->mnt_mounts.next;
 	if (next == &p->mnt_mounts) {
 		while (1) {
 			if (p == root)
@@ -336,16 +339,17 @@
 
 	if (atomic_read(&sb->s_active) == 1) {
 		/* last instance - try to be smart */
 		spin_unlock(&dcache_lock);
 		lock_kernel();
 		DQUOT_OFF(sb);
 		acct_auto_close(sb);
 		unlock_kernel();
+		/* XXX can we get this with the detach? */
 		security_ops->sb_umount_close(mnt);
 		spin_lock(&dcache_lock);
 	}
 	retval = -EBUSY;
 	if (atomic_read(&mnt->mnt_count) == 2 || flags & MNT_DETACH) {
 		if (!list_empty(&mnt->mnt_list))
 			umount_tree(mnt);
 		retval = 0;
@@ -435,16 +439,19 @@
 			q = q->mnt_parent;
 		}
 		p = next;
 		nd.mnt = q;
 		nd.dentry = p->mnt_mountpoint;
 		q = clone_mnt(p, p->mnt_root);
 		if (!q)
 			goto Enomem;
+		/* XXX return value is lost */
+		if (security_ops->sb_check_sb(q, &nd))
+			goto Enomem;
 		spin_lock(&dcache_lock);
 		list_add_tail(&q->mnt_list, &res->mnt_list);
 		attach_mnt(q, &nd);
 		spin_unlock(&dcache_lock);
 	}
 	return res;
 Enomem:
 	if (res) {
@@ -481,18 +488,16 @@
 		list_add_tail(&head, &mnt->mnt_list);
 		list_splice(&head, current->namespace->list.prev);
 		mntget(mnt);
 		err = 0;
 	}
 	spin_unlock(&dcache_lock);
 out_unlock:
 	up(&nd->dentry->d_inode->i_sem);
-	if (!err)
-		security_ops->sb_post_addmount(mnt, nd);
 	return err;
 }
 
 /*
  * do loopback mount.
  */
 static int do_loopback(struct nameidata *nd, char *old_name, int recurse)
 {
@@ -601,18 +606,20 @@
 	if (S_ISDIR(nd->dentry->d_inode->i_mode) !=
 	      S_ISDIR(old_nd.dentry->d_inode->i_mode))
 		goto out2;
 
 	err = -ELOOP;
 	for (p = nd->mnt; p->mnt_parent!=p; p = p->mnt_parent)
 		if (p == old_nd.mnt)
 			goto out2;
-	err = 0;
-
+	/* XXX inconsistent wrt. dcache_lock */
+	err = security_ops->sb_check_sb(old_nd.mnt, nd);
+	if (err)
+		goto out2;
 	detach_mnt(old_nd.mnt, &parent_nd);
 	attach_mnt(old_nd.mnt, nd);
 out2:
 	spin_unlock(&dcache_lock);
 out1:
 	up(&nd->dentry->d_inode->i_sem);
 out:
 	up_write(&current->namespace->sem);
@@ -984,16 +991,17 @@
 	} else if (!is_subdir(old_nd.dentry, new_nd.dentry))
 		goto out3;
 	detach_mnt(new_nd.mnt, &parent_nd);
 	detach_mnt(user_nd.mnt, &root_parent);
 	attach_mnt(user_nd.mnt, &old_nd);
 	attach_mnt(new_nd.mnt, &root_parent);
 	spin_unlock(&dcache_lock);
 	chroot_fs_refs(&user_nd, &new_nd);
+	/* XXX this should be obsolete now */
 	security_ops->sb_post_pivotroot(&user_nd, &new_nd);
 	error = 0;
 	path_release(&root_parent);
 	path_release(&parent_nd);
 out2:
 	up(&old_nd.dentry->d_inode->i_sem);
 	up_write(&current->namespace->sem);
 	path_release(&user_nd);
_______________________________________________
linux-security-module mailing list
linux-security-moduleat_private
http://mail.wirex.com/mailman/listinfo/linux-security-module
