[kernel] r22472 - in dists/sid/linux/debian: . patches patches/bugfix/all

Ben Hutchings benh at moszumanska.debian.org
Mon Apr 6 17:24:48 UTC 2015


Author: benh
Date: Mon Apr  6 17:24:48 2015
New Revision: 22472

Log:
Btrfs: make xattr replace operations atomic (CVE-2014-9710)

Added:
   dists/sid/linux/debian/patches/bugfix/all/btrfs-make-xattr-replace-operations-atomic.patch
Modified:
   dists/sid/linux/debian/changelog
   dists/sid/linux/debian/patches/series

Modified: dists/sid/linux/debian/changelog
==============================================================================
--- dists/sid/linux/debian/changelog	Mon Apr  6 17:06:28 2015	(r22471)
+++ dists/sid/linux/debian/changelog	Mon Apr  6 17:24:48 2015	(r22472)
@@ -180,6 +180,7 @@
   * Revert "quota: Store maximum space limit in bytes" to avoid ABI change
   * IB/core: Prevent integer overflow in ib_umem_get address arithmetic
     (CVE-2014-8159)
+  * Btrfs: make xattr replace operations atomic (CVE-2014-9710)
 
  -- Ian Campbell <ijc at debian.org>  Wed, 18 Mar 2015 21:07:15 +0000
 

Added: dists/sid/linux/debian/patches/bugfix/all/btrfs-make-xattr-replace-operations-atomic.patch
==============================================================================
--- /dev/null	00:00:00 1970	(empty, because file is newly added)
+++ dists/sid/linux/debian/patches/bugfix/all/btrfs-make-xattr-replace-operations-atomic.patch	Mon Apr  6 17:24:48 2015	(r22472)
@@ -0,0 +1,289 @@
+From: Filipe Manana <fdmanana at suse.com>
+Date: Sun, 9 Nov 2014 08:38:39 +0000
+Subject: Btrfs: make xattr replace operations atomic
+Origin: https://git.kernel.org/linus/5f5bc6b1e2d5a6f827bc860ef2dc5b6f365d1339
+
+Replacing a xattr consists of doing a lookup for its existing value, delete
+the current value from the respective leaf, release the search path and then
+finally insert the new value. This leaves a time window where readers (getxattr,
+listxattrs) won't see any value for the xattr. Xattrs are used to store ACLs,
+so this has security implications.
+
+This change also fixes 2 other existing issues which were:
+
+*) Deleting the old xattr value without verifying first if the new xattr will
+   fit in the existing leaf item (in case multiple xattrs are packed in the
+   same item due to name hash collision);
+
+*) Returning -EEXIST when the flag XATTR_CREATE is given and the xattr doesn't
+   exist but we have have an existing item that packs muliple xattrs with
+   the same name hash as the input xattr. In this case we should return ENOSPC.
+
+A test case for xfstests follows soon.
+
+Thanks to Alexandre Oliva for reporting the non-atomicity of the xattr replace
+implementation.
+
+Reported-by: Alexandre Oliva <oliva at gnu.org>
+Signed-off-by: Filipe Manana <fdmanana at suse.com>
+Signed-off-by: Chris Mason <clm at fb.com>
+---
+ fs/btrfs/ctree.c    |   2 +-
+ fs/btrfs/ctree.h    |   5 ++
+ fs/btrfs/dir-item.c |  10 ++--
+ fs/btrfs/xattr.c    | 150 ++++++++++++++++++++++++++++++++--------------------
+ 4 files changed, 102 insertions(+), 65 deletions(-)
+
+--- a/fs/btrfs/ctree.c
++++ b/fs/btrfs/ctree.c
+@@ -2929,7 +2929,7 @@ done:
+ 	 */
+ 	if (!p->leave_spinning)
+ 		btrfs_set_path_blocking(p);
+-	if (ret < 0)
++	if (ret < 0 && !p->skip_release_on_error)
+ 		btrfs_release_path(p);
+ 	return ret;
+ }
+--- a/fs/btrfs/ctree.h
++++ b/fs/btrfs/ctree.h
+@@ -611,6 +611,7 @@ struct btrfs_path {
+ 	unsigned int leave_spinning:1;
+ 	unsigned int search_commit_root:1;
+ 	unsigned int need_commit_sem:1;
++	unsigned int skip_release_on_error:1;
+ };
+ 
+ /*
+@@ -3695,6 +3696,10 @@ struct btrfs_dir_item *btrfs_lookup_xatt
+ int verify_dir_item(struct btrfs_root *root,
+ 		    struct extent_buffer *leaf,
+ 		    struct btrfs_dir_item *dir_item);
++struct btrfs_dir_item *btrfs_match_dir_item_name(struct btrfs_root *root,
++						 struct btrfs_path *path,
++						 const char *name,
++						 int name_len);
+ 
+ /* orphan.c */
+ int btrfs_insert_orphan_item(struct btrfs_trans_handle *trans,
+--- a/fs/btrfs/dir-item.c
++++ b/fs/btrfs/dir-item.c
+@@ -21,10 +21,6 @@
+ #include "hash.h"
+ #include "transaction.h"
+ 
+-static struct btrfs_dir_item *btrfs_match_dir_item_name(struct btrfs_root *root,
+-			      struct btrfs_path *path,
+-			      const char *name, int name_len);
+-
+ /*
+  * insert a name into a directory, doing overflow properly if there is a hash
+  * collision.  data_size indicates how big the item inserted should be.  On
+@@ -383,9 +379,9 @@ struct btrfs_dir_item *btrfs_lookup_xatt
+  * this walks through all the entries in a dir item and finds one
+  * for a specific name.
+  */
+-static struct btrfs_dir_item *btrfs_match_dir_item_name(struct btrfs_root *root,
+-			      struct btrfs_path *path,
+-			      const char *name, int name_len)
++struct btrfs_dir_item *btrfs_match_dir_item_name(struct btrfs_root *root,
++						 struct btrfs_path *path,
++						 const char *name, int name_len)
+ {
+ 	struct btrfs_dir_item *dir_item;
+ 	unsigned long name_ptr;
+--- a/fs/btrfs/xattr.c
++++ b/fs/btrfs/xattr.c
+@@ -29,6 +29,7 @@
+ #include "xattr.h"
+ #include "disk-io.h"
+ #include "props.h"
++#include "locking.h"
+ 
+ 
+ ssize_t __btrfs_getxattr(struct inode *inode, const char *name,
+@@ -91,7 +92,7 @@ static int do_setxattr(struct btrfs_tran
+ 		       struct inode *inode, const char *name,
+ 		       const void *value, size_t size, int flags)
+ {
+-	struct btrfs_dir_item *di;
++	struct btrfs_dir_item *di = NULL;
+ 	struct btrfs_root *root = BTRFS_I(inode)->root;
+ 	struct btrfs_path *path;
+ 	size_t name_len = strlen(name);
+@@ -103,84 +104,119 @@ static int do_setxattr(struct btrfs_tran
+ 	path = btrfs_alloc_path();
+ 	if (!path)
+ 		return -ENOMEM;
++	path->skip_release_on_error = 1;
++
++	if (!value) {
++		di = btrfs_lookup_xattr(trans, root, path, btrfs_ino(inode),
++					name, name_len, -1);
++		if (!di && (flags & XATTR_REPLACE))
++			ret = -ENODATA;
++		else if (di)
++			ret = btrfs_delete_one_dir_name(trans, root, path, di);
++		goto out;
++	}
+ 
++	/*
++	 * For a replace we can't just do the insert blindly.
++	 * Do a lookup first (read-only btrfs_search_slot), and return if xattr
++	 * doesn't exist. If it exists, fall down below to the insert/replace
++	 * path - we can't race with a concurrent xattr delete, because the VFS
++	 * locks the inode's i_mutex before calling setxattr or removexattr.
++	 */
+ 	if (flags & XATTR_REPLACE) {
+-		di = btrfs_lookup_xattr(trans, root, path, btrfs_ino(inode), name,
+-					name_len, -1);
+-		if (IS_ERR(di)) {
+-			ret = PTR_ERR(di);
+-			goto out;
+-		} else if (!di) {
++		ASSERT(mutex_is_locked(&inode->i_mutex));
++		di = btrfs_lookup_xattr(NULL, root, path, btrfs_ino(inode),
++					name, name_len, 0);
++		if (!di) {
+ 			ret = -ENODATA;
+ 			goto out;
+ 		}
+-		ret = btrfs_delete_one_dir_name(trans, root, path, di);
+-		if (ret)
+-			goto out;
+ 		btrfs_release_path(path);
++		di = NULL;
++	}
+ 
++	ret = btrfs_insert_xattr_item(trans, root, path, btrfs_ino(inode),
++				      name, name_len, value, size);
++	if (ret == -EOVERFLOW) {
+ 		/*
+-		 * remove the attribute
++		 * We have an existing item in a leaf, split_leaf couldn't
++		 * expand it. That item might have or not a dir_item that
++		 * matches our target xattr, so lets check.
+ 		 */
+-		if (!value)
+-			goto out;
+-	} else {
+-		di = btrfs_lookup_xattr(NULL, root, path, btrfs_ino(inode),
+-					name, name_len, 0);
+-		if (IS_ERR(di)) {
+-			ret = PTR_ERR(di);
++		ret = 0;
++		btrfs_assert_tree_locked(path->nodes[0]);
++		di = btrfs_match_dir_item_name(root, path, name, name_len);
++		if (!di && !(flags & XATTR_REPLACE)) {
++			ret = -ENOSPC;
+ 			goto out;
+ 		}
+-		if (!di && !value)
+-			goto out;
+-		btrfs_release_path(path);
++	} else if (ret == -EEXIST) {
++		ret = 0;
++		di = btrfs_match_dir_item_name(root, path, name, name_len);
++		ASSERT(di); /* logic error */
++	} else if (ret) {
++		goto out;
+ 	}
+ 
+-again:
+-	ret = btrfs_insert_xattr_item(trans, root, path, btrfs_ino(inode),
+-				      name, name_len, value, size);
+-	/*
+-	 * If we're setting an xattr to a new value but the new value is say
+-	 * exactly BTRFS_MAX_XATTR_SIZE, we could end up with EOVERFLOW getting
+-	 * back from split_leaf.  This is because it thinks we'll be extending
+-	 * the existing item size, but we're asking for enough space to add the
+-	 * item itself.  So if we get EOVERFLOW just set ret to EEXIST and let
+-	 * the rest of the function figure it out.
+-	 */
+-	if (ret == -EOVERFLOW)
++	if (di && (flags & XATTR_CREATE)) {
+ 		ret = -EEXIST;
++		goto out;
++	}
+ 
+-	if (ret == -EEXIST) {
+-		if (flags & XATTR_CREATE)
+-			goto out;
++	if (di) {
+ 		/*
+-		 * We can't use the path we already have since we won't have the
+-		 * proper locking for a delete, so release the path and
+-		 * re-lookup to delete the thing.
++		 * We're doing a replace, and it must be atomic, that is, at
++		 * any point in time we have either the old or the new xattr
++		 * value in the tree. We don't want readers (getxattr and
++		 * listxattrs) to miss a value, this is specially important
++		 * for ACLs.
+ 		 */
+-		btrfs_release_path(path);
+-		di = btrfs_lookup_xattr(trans, root, path, btrfs_ino(inode),
+-					name, name_len, -1);
+-		if (IS_ERR(di)) {
+-			ret = PTR_ERR(di);
+-			goto out;
+-		} else if (!di) {
+-			/* Shouldn't happen but just in case... */
+-			btrfs_release_path(path);
+-			goto again;
++		const int slot = path->slots[0];
++		struct extent_buffer *leaf = path->nodes[0];
++		const u16 old_data_len = btrfs_dir_data_len(leaf, di);
++		const u32 item_size = btrfs_item_size_nr(leaf, slot);
++		const u32 data_size = sizeof(*di) + name_len + size;
++		struct btrfs_item *item;
++		unsigned long data_ptr;
++		char *ptr;
++
++		if (size > old_data_len) {
++			if (btrfs_leaf_free_space(root, leaf) <
++			    (size - old_data_len)) {
++				ret = -ENOSPC;
++				goto out;
++			}
+ 		}
+ 
+-		ret = btrfs_delete_one_dir_name(trans, root, path, di);
+-		if (ret)
+-			goto out;
++		if (old_data_len + name_len + sizeof(*di) == item_size) {
++			/* No other xattrs packed in the same leaf item. */
++			if (size > old_data_len)
++				btrfs_extend_item(root, path,
++						  size - old_data_len);
++			else if (size < old_data_len)
++				btrfs_truncate_item(root, path, data_size, 1);
++		} else {
++			/* There are other xattrs packed in the same item. */
++			ret = btrfs_delete_one_dir_name(trans, root, path, di);
++			if (ret)
++				goto out;
++			btrfs_extend_item(root, path, data_size);
++		}
+ 
++		item = btrfs_item_nr(slot);
++		ptr = btrfs_item_ptr(leaf, slot, char);
++		ptr += btrfs_item_size(leaf, item) - data_size;
++		di = (struct btrfs_dir_item *)ptr;
++		btrfs_set_dir_data_len(leaf, di, size);
++		data_ptr = ((unsigned long)(di + 1)) + name_len;
++		write_extent_buffer(leaf, value, data_ptr, size);
++		btrfs_mark_buffer_dirty(leaf);
++	} else {
+ 		/*
+-		 * We have a value to set, so go back and try to insert it now.
++		 * Insert, and we had space for the xattr, so path->slots[0] is
++		 * where our xattr dir_item is and btrfs_insert_xattr_item()
++		 * filled it.
+ 		 */
+-		if (value) {
+-			btrfs_release_path(path);
+-			goto again;
+-		}
+ 	}
+ out:
+ 	btrfs_free_path(path);

Modified: dists/sid/linux/debian/patches/series
==============================================================================
--- dists/sid/linux/debian/patches/series	Mon Apr  6 17:06:28 2015	(r22471)
+++ dists/sid/linux/debian/patches/series	Mon Apr  6 17:24:48 2015	(r22472)
@@ -556,3 +556,4 @@
 debian/usb-avoid-abi-change-in-3.16.7-ckt8.patch
 
 bugfix/all/ib-core-prevent-integer-overflow-in-ib_umem_get.patch
+bugfix/all/btrfs-make-xattr-replace-operations-atomic.patch



More information about the Kernel-svn-changes mailing list