From a1a7477708ac8ccc042cfe01e663fb24f93fb31b Mon Sep 17 00:00:00 2001 From: Ben Hutchings Date: Mon, 6 Apr 2015 17:24:48 +0000 Subject: [PATCH] Btrfs: make xattr replace operations atomic (CVE-2014-9710) svn path=/dists/sid/linux/; revision=22472 --- debian/changelog | 1 + ...make-xattr-replace-operations-atomic.patch | 289 ++++++++++++++++++ debian/patches/series | 1 + 3 files changed, 291 insertions(+) create mode 100644 debian/patches/bugfix/all/btrfs-make-xattr-replace-operations-atomic.patch diff --git a/debian/changelog b/debian/changelog index aa44ab8b5..d64da0a64 100644 --- a/debian/changelog +++ b/debian/changelog @@ -180,6 +180,7 @@ linux (3.16.7-ckt9-1) UNRELEASED; urgency=medium * 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 Wed, 18 Mar 2015 21:07:15 +0000 diff --git a/debian/patches/bugfix/all/btrfs-make-xattr-replace-operations-atomic.patch b/debian/patches/bugfix/all/btrfs-make-xattr-replace-operations-atomic.patch new file mode 100644 index 000000000..7af01accd --- /dev/null +++ b/debian/patches/bugfix/all/btrfs-make-xattr-replace-operations-atomic.patch @@ -0,0 +1,289 @@ +From: Filipe Manana +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 +Signed-off-by: Filipe Manana +Signed-off-by: Chris Mason +--- + 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); diff --git a/debian/patches/series b/debian/patches/series index 6b2cb22d1..4d7dfa42c 100644 --- a/debian/patches/series +++ b/debian/patches/series @@ -556,3 +556,4 @@ debian/tcp-fix-abi-change-in-3.16.7-ckt7.patch 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