[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