diff mbox

[3/3] btrfs: check namelen before read/memcmp_extent_buffer

Message ID 20170525020908.25830-3-suy.fnst@cn.fujitsu.com (mailing list archive)
State New, archived
Headers show

Commit Message

Su Yue May 25, 2017, 2:09 a.m. UTC 
Reading name using 'read_extent_buffer' and 'memcmp_extent_buffer'
may cause read beyond item boundary if namelen field in dir_item,
inode_ref is corrupted.

Example:
	1. Corrupt one dir_item namelen to be 255.
        2. Run 'ls -lar /mnt/test/ > /dev/null'
dmesg:
[   48.451449] BTRFS info (device vdb1): disk space caching is enabled
[   48.451453] BTRFS info (device vdb1): has skinny extents
[   48.489420] general protection fault: 0000 [#1] SMP
[   48.489571] Modules linked in: ext4 jbd2 mbcache btrfs xor raid6_pq
[   48.489716] CPU: 1 PID: 2710 Comm: ls Not tainted 4.10.0-rc1 #5
[   48.489853] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.10.2-20170228_101828-anatol 04/01/2014
[   48.490008] task: ffff880035df1bc0 task.stack: ffffc90004800000
[   48.490008] RIP: 0010:read_extent_buffer+0xd2/0x190 [btrfs]
[   48.490008] RSP: 0018:ffffc90004803d98 EFLAGS: 00010202
[   48.490008] RAX: 000000000000001b RBX: 000000000000001b RCX: 0000000000000000
[   48.490008] RDX: ffff880079dbf36c RSI: 0005080000000000 RDI: ffff880079dbf368
[   48.490008] RBP: ffffc90004803dc8 R08: ffff880078e8cc48 R09: ffff880000000000
[   48.490008] R10: 0000160000000000 R11: 0000000000001000 R12: ffff880079dbf288
[   48.490008] R13: ffff880078e8ca88 R14: 0000000000000003 R15: ffffc90004803e20
[   48.490008] FS:  00007fef50c60800(0000) GS:ffff88007d400000(0000) knlGS:0000000000000000
[   48.490008] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   48.490008] CR2: 000055f335ac2ff8 CR3: 000000007356d000 CR4: 00000000001406e0
[   48.490008] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[   48.490008] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[   48.490008] Call Trace:
[   48.490008]  btrfs_real_readdir+0x3b7/0x4a0 [btrfs]
[   48.490008]  iterate_dir+0x181/0x1b0
[   48.490008]  SyS_getdents+0xa7/0x150
[   48.490008]  ? fillonedir+0x150/0x150
[   48.490008]  entry_SYSCALL_64_fastpath+0x18/0xad
[   48.490008] RIP: 0033:0x7fef5032546b
[   48.490008] RSP: 002b:00007ffeafcdb830 EFLAGS: 00000206 ORIG_RAX: 000000000000004e
[   48.490008] RAX: ffffffffffffffda RBX: 00007fef5061db38 RCX: 00007fef5032546b
[   48.490008] RDX: 0000000000008000 RSI: 000055f335abaff0 RDI: 0000000000000003
[   48.490008] RBP: 00007fef5061dae0 R08: 00007fef5061db48 R09: 0000000000000000
[   48.490008] R10: 000055f335abafc0 R11: 0000000000000206 R12: 00007fef5061db38
[   48.490008] R13: 0000000000008040 R14: 00007fef5061db38 R15: 000000000000270e
[   48.490008] Code: 48 29 c3 74 5f 4c 89 d8 4c 89 d6 48 29 c8 48 39 d8 48 0f 47 c3 49 03 30 48 c1 fe 06 48 c1 e6 0c 4c 01 ce 48 01 ce 83 f8 08 72 b3 <48> 8b 0e 49 83 c0 08 48 89 0a 89 c1 48 8b 7c 0e f8 48 89 7c 0a
[   48.490008] RIP: read_extent_buffer+0xd2/0x190 [btrfs] RSP: ffffc90004803d98
[   48.499455] ---[ end trace 321920d8e8339505 ]---

Solutions:
1. If read from dir_item, using 'verify_dir_item' to verify namelen.
   And if 'verify_dir_item' failed, returns with error.
2. Otherwise, call 'check_btrfs_namelen' to get 'proper' namelen.
   If the returned value is not equal to original namelen,
   returns with error.

Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
---
 fs/btrfs/dir-item.c   |  5 ++--
 fs/btrfs/export.c     |  7 +++++
 fs/btrfs/inode-item.c | 11 +++++++-
 fs/btrfs/root-tree.c  |  7 +++++
 fs/btrfs/tree-log.c   | 72 ++++++++++++++++++++++++++++++++++++++++++---------
 5 files changed, 87 insertions(+), 15 deletions(-)

Comments

David Sterba May 29, 2017, 3:43 p.m. UTC | #1 
This patch adds the name length verification to many places and in some
of them it looks unnecessary, as the directory item passes sanity checks
already. The verification should always happen when we read the input,
ie from disk, after search_slot etc. Then, it can be considered valid
and does not need the strict checks.

I haven't gone through all, one example is __add_inode_ref. The caller
add_inode_ref uses namelen before it reaches __add_inode_ref (and thus
the sanity checks).

As the checks add error hanlind branch, the entire callgraph of the
function should be verified, so I suggest to split the patch and group
only hunks that touch the same codepaths.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Su Yue May 31, 2017, 2:31 a.m. UTC | #2 
On 05/29/2017 11:43 PM, David Sterba wrote:
> This patch adds the name length verification to many places and in some
> of them it looks unnecessary, as the directory item passes sanity checks
> already. The verification should always happen when we read the input,
> ie from disk, after search_slot etc. Then, it can be considered valid
> and does not need the strict checks.
> 
I have realized the fact many checks in the patch are useless.
> I haven't gone through all, one example is __add_inode_ref. The caller
> add_inode_ref uses namelen before it reaches __add_inode_ref (and thus
> the sanity checks).
> 
> As the checks add error hanlind branch, the entire callgraph of the
> function should be verified, so I suggest to split the patch and group
> only hunks that touch the same codepaths.
> 
OK, I will update and split it.
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/fs/btrfs/dir-item.c b/fs/btrfs/dir-item.c
index 82390f36101c..8c28ff457219 100644
--- a/fs/btrfs/dir-item.c
+++ b/fs/btrfs/dir-item.c
@@ -395,11 +395,12 @@  struct btrfs_dir_item *btrfs_match_dir_item_name(struct btrfs_fs_info *fs_info,
 
 	leaf = path->nodes[0];
 	dir_item = btrfs_item_ptr(leaf, path->slots[0], struct btrfs_dir_item);
-	if (verify_dir_item(fs_info, leaf, path->slots[0], dir_item))
-		return NULL;
 
 	total_len = btrfs_item_size_nr(leaf, path->slots[0]);
 	while (cur < total_len) {
+		if (verify_dir_item(fs_info, leaf, path->slots[0], dir_item))
+			return NULL;
+
 		this_len = sizeof(*dir_item) +
 			btrfs_dir_name_len(leaf, dir_item) +
 			btrfs_dir_data_len(leaf, dir_item);
diff --git a/fs/btrfs/export.c b/fs/btrfs/export.c
index 87144c9f9593..96c24adef54f 100644
--- a/fs/btrfs/export.c
+++ b/fs/btrfs/export.c
@@ -234,6 +234,7 @@  static int btrfs_get_name(struct dentry *parent, char *name,
 	int name_len;
 	int ret;
 	u64 ino;
+	u16 namelen_ret;
 
 	if (!S_ISDIR(dir->i_mode))
 		return -EINVAL;
@@ -282,6 +283,12 @@  static int btrfs_get_name(struct dentry *parent, char *name,
 		name_len = btrfs_inode_ref_name_len(leaf, iref);
 	}
 
+	namelen_ret = btrfs_check_namelen(leaf, path->slots[0], name_ptr,
+					  name_len);
+	if (namelen_ret != name_len) {
+		btrfs_free_path(path);
+		return -EIO;
+	}
 	read_extent_buffer(leaf, name, name_ptr, name_len);
 	btrfs_free_path(path);
 
diff --git a/fs/btrfs/inode-item.c b/fs/btrfs/inode-item.c
index 39c968f80157..c478da7c4784 100644
--- a/fs/btrfs/inode-item.c
+++ b/fs/btrfs/inode-item.c
@@ -32,6 +32,7 @@  static int find_name_in_backref(struct btrfs_path *path, const char *name,
 	u32 item_size;
 	u32 cur_offset = 0;
 	int len;
+	u16 namelen_ret;
 
 	leaf = path->nodes[0];
 	item_size = btrfs_item_size_nr(leaf, path->slots[0]);
@@ -43,6 +44,10 @@  static int find_name_in_backref(struct btrfs_path *path, const char *name,
 		cur_offset += len + sizeof(*ref);
 		if (len != name_len)
 			continue;
+		namelen_ret = btrfs_check_namelen(leaf, path->slots[0],
+						name_ptr, name_len);
+		if (namelen_ret != name_len)
+			break;
 		if (memcmp_extent_buffer(leaf, name, name_ptr, name_len) == 0) {
 			*ref_ret = ref;
 			return 1;
@@ -62,6 +67,7 @@  int btrfs_find_name_in_ext_backref(struct btrfs_path *path, u64 ref_objectid,
 	u32 item_size;
 	u32 cur_offset = 0;
 	int ref_name_len;
+	u16 namelen_ret;
 
 	leaf = path->nodes[0];
 	item_size = btrfs_item_size_nr(leaf, path->slots[0]);
@@ -77,7 +83,10 @@  int btrfs_find_name_in_ext_backref(struct btrfs_path *path, u64 ref_objectid,
 		extref = (struct btrfs_inode_extref *) (ptr + cur_offset);
 		name_ptr = (unsigned long)(&extref->name);
 		ref_name_len = btrfs_inode_extref_name_len(leaf, extref);
-
+		namelen_ret = btrfs_check_namelen(leaf, path->slots[0],
+						name_ptr, name_len);
+		if (namelen_ret != ref_name_len)
+			break;
 		if (ref_name_len == name_len &&
 		    btrfs_inode_extref_parent(leaf, extref) == ref_objectid &&
 		    (memcmp_extent_buffer(leaf, name, name_ptr, name_len) == 0)) {
diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c
index 7d6bc308bf43..a7c657b784d1 100644
--- a/fs/btrfs/root-tree.c
+++ b/fs/btrfs/root-tree.c
@@ -371,6 +371,7 @@  int btrfs_del_root_ref(struct btrfs_trans_handle *trans,
 	unsigned long ptr;
 	int err = 0;
 	int ret;
+	u16 namelen_ret;
 
 	path = btrfs_alloc_path();
 	if (!path)
@@ -390,6 +391,12 @@  int btrfs_del_root_ref(struct btrfs_trans_handle *trans,
 		WARN_ON(btrfs_root_ref_dirid(leaf, ref) != dirid);
 		WARN_ON(btrfs_root_ref_name_len(leaf, ref) != name_len);
 		ptr = (unsigned long)(ref + 1);
+		namelen_ret = btrfs_check_namelen(leaf, path->slots[0],
+						ptr, name_len);
+		if (namelen_ret != name_len) {
+			err = -EIO;
+			goto out;
+		}
 		WARN_ON(memcmp_extent_buffer(leaf, name, ptr, name_len));
 		*sequence = btrfs_root_ref_sequence(leaf, ref);
 
diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index 1930f28edcdd..ed7b0adbc403 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -863,6 +863,9 @@  static noinline int drop_one_dir_item(struct btrfs_trans_handle *trans,
 
 	btrfs_dir_item_key_to_cpu(leaf, di, &location);
 	name_len = btrfs_dir_name_len(leaf, di);
+	if (verify_dir_item(fs_info, leaf, path->slots[0], di))
+		return -EIO;
+
 	name = kmalloc(name_len, GFP_NOFS);
 	if (!name)
 		return -ENOMEM;
@@ -953,6 +956,7 @@  static noinline int backref_in_log(struct btrfs_root *log,
 	int item_size;
 	int ret;
 	int match = 0;
+	u16 namelen_ret;
 
 	path = btrfs_alloc_path();
 	if (!path)
@@ -976,9 +980,11 @@  static noinline int backref_in_log(struct btrfs_root *log,
 	ptr_end = ptr + item_size;
 	while (ptr < ptr_end) {
 		ref = (struct btrfs_inode_ref *)ptr;
+		name_ptr = (unsigned long)(ref + 1);
 		found_name_len = btrfs_inode_ref_name_len(path->nodes[0], ref);
+		namelen_ret = btrfs_check_namelen(path->nodes[0],
+				path->slots[0], name_ptr, found_name_len);
 		if (found_name_len == namelen) {
-			name_ptr = (unsigned long)(ref + 1);
 			ret = memcmp_extent_buffer(path->nodes[0], name,
 						   name_ptr, namelen);
 			if (ret == 0) {
@@ -1005,6 +1011,7 @@  static inline int __add_inode_ref(struct btrfs_trans_handle *trans,
 {
 	struct btrfs_fs_info *fs_info = root->fs_info;
 	int ret;
+	u16 namelen_ret;
 	char *victim_name;
 	int victim_name_len;
 	struct extent_buffer *leaf;
@@ -1041,6 +1048,11 @@  static inline int __add_inode_ref(struct btrfs_trans_handle *trans,
 			victim_ref = (struct btrfs_inode_ref *)ptr;
 			victim_name_len = btrfs_inode_ref_name_len(leaf,
 								   victim_ref);
+			namelen_ret = btrfs_check_namelen(leaf, path->slots[0],
+						(unsigned long)(victim_ref + 1),
+						victim_name_len);
+			if (namelen_ret != victim_name_len)
+				return -EIO;
 			victim_name = kmalloc(victim_name_len, GFP_NOFS);
 			if (!victim_name)
 				return -ENOMEM;
@@ -1087,6 +1099,7 @@  static inline int __add_inode_ref(struct btrfs_trans_handle *trans,
 	if (!IS_ERR_OR_NULL(extref)) {
 		u32 item_size;
 		u32 cur_offset = 0;
+		u16 namelen_ret;
 		unsigned long base;
 		struct inode *victim_parent;
 
@@ -1099,6 +1112,11 @@  static inline int __add_inode_ref(struct btrfs_trans_handle *trans,
 			extref = (struct btrfs_inode_extref *)(base + cur_offset);
 
 			victim_name_len = btrfs_inode_extref_name_len(leaf, extref);
+			namelen_ret = btrfs_check_namelen(leaf, path->slots[0],
+						(unsigned long)&extref->name,
+						victim_name_len);
+			if (namelen_ret != victim_name_len)
+				return -EIO;
 
 			if (btrfs_inode_extref_parent(leaf, extref) != parent_objectid)
 				goto next;
@@ -1175,16 +1193,20 @@  static inline int __add_inode_ref(struct btrfs_trans_handle *trans,
 	return 0;
 }
 
-static int extref_get_fields(struct extent_buffer *eb, unsigned long ref_ptr,
-			     u32 *namelen, char **name, u64 *index,
-			     u64 *parent_objectid)
+static int extref_get_fields(struct extent_buffer *eb, int slot,
+			     unsigned long ref_ptr, u32 *namelen, char **name,
+			     u64 *index, u64 *parent_objectid)
 {
 	struct btrfs_inode_extref *extref;
-
+	u32 namelen_ret;
 	extref = (struct btrfs_inode_extref *)ref_ptr;
 
 	*namelen = btrfs_inode_extref_name_len(eb, extref);
 	*name = kmalloc(*namelen, GFP_NOFS);
+	namelen_ret = btrfs_check_namelen(eb, slot,
+					(unsigned long)&extref->name, *namelen);
+	if (namelen_ret != *namelen)
+		return -EIO;
 	if (*name == NULL)
 		return -ENOMEM;
 
@@ -1198,14 +1220,18 @@  static int extref_get_fields(struct extent_buffer *eb, unsigned long ref_ptr,
 	return 0;
 }
 
-static int ref_get_fields(struct extent_buffer *eb, unsigned long ref_ptr,
-			  u32 *namelen, char **name, u64 *index)
+static int ref_get_fields(struct extent_buffer *eb, int slot,
+		unsigned long ref_ptr, u32 *namelen, char **name, u64 *index)
 {
 	struct btrfs_inode_ref *ref;
-
+	u32 namelen_ret;
 	ref = (struct btrfs_inode_ref *)ref_ptr;
 
 	*namelen = btrfs_inode_ref_name_len(eb, ref);
+	namelen_ret = btrfs_check_namelen(eb, slot, (unsigned long)(ref + 1),
+					*namelen);
+	if (namelen_ret != *namelen)
+		return -EIO;
 	*name = kmalloc(*namelen, GFP_NOFS);
 	if (*name == NULL)
 		return -ENOMEM;
@@ -1280,8 +1306,8 @@  static noinline int add_inode_ref(struct btrfs_trans_handle *trans,
 
 	while (ref_ptr < ref_end) {
 		if (log_ref_ver) {
-			ret = extref_get_fields(eb, ref_ptr, &namelen, &name,
-						&ref_index, &parent_objectid);
+			ret = extref_get_fields(eb, slot, ref_ptr, &namelen,
+					&name, &ref_index, &parent_objectid);
 			/*
 			 * parent object can change from one array
 			 * item to another.
@@ -1293,7 +1319,7 @@  static noinline int add_inode_ref(struct btrfs_trans_handle *trans,
 				goto out;
 			}
 		} else {
-			ret = ref_get_fields(eb, ref_ptr, &namelen, &name,
+			ret = ref_get_fields(eb, slot, ref_ptr, &namelen, &name,
 					     &ref_index);
 		}
 		if (ret)
@@ -1704,7 +1730,8 @@  static noinline int replay_one_name(struct btrfs_trans_handle *trans,
 				    struct btrfs_dir_item *di,
 				    struct btrfs_key *key)
 {
-	char *name;
+	struct btrfs_fs_info *fs_info = root->fs_info;
+	char *name = NULL;
 	int name_len;
 	struct btrfs_dir_item *dst_di;
 	struct btrfs_key found_key;
@@ -1721,6 +1748,12 @@  static noinline int replay_one_name(struct btrfs_trans_handle *trans,
 		return -EIO;
 
 	name_len = btrfs_dir_name_len(eb, di);
+	ret = verify_dir_item(fs_info, eb, path->slots[0], di);
+	if (ret) {
+		ret = -EIO;
+		goto out;
+	}
+
 	name = kmalloc(name_len, GFP_NOFS);
 	if (!name) {
 		ret = -ENOMEM;
@@ -2102,6 +2135,7 @@  static int replay_xattr_deletes(struct btrfs_trans_handle *trans,
 			      struct btrfs_path *path,
 			      const u64 ino)
 {
+	struct btrfs_fs_info *fs_info = root->fs_info;
 	struct btrfs_key search_key;
 	struct btrfs_path *log_path;
 	int i;
@@ -2143,6 +2177,12 @@  static int replay_xattr_deletes(struct btrfs_trans_handle *trans,
 			u32 this_len = sizeof(*di) + name_len + data_len;
 			char *name;
 
+			if (verify_dir_item(fs_info, path->nodes[0],
+					path->slots[0], di)) {
+				ret = -EIO;
+				goto out;
+			}
+
 			name = kmalloc(name_len, GFP_NOFS);
 			if (!name) {
 				ret = -ENOMEM;
@@ -4524,6 +4564,8 @@  static int btrfs_check_ref_name_override(struct extent_buffer *eb,
 		u64 parent;
 		u32 this_name_len;
 		u32 this_len;
+		u16 namelen_ret;
+
 		unsigned long name_ptr;
 		struct btrfs_dir_item *di;
 
@@ -4546,6 +4588,12 @@  static int btrfs_check_ref_name_override(struct extent_buffer *eb,
 			this_len = sizeof(*extref) + this_name_len;
 		}
 
+		namelen_ret = btrfs_check_namelen(eb, slot, name_ptr,
+						this_name_len);
+		if (namelen_ret != this_name_len) {
+			ret = -EIO;
+			goto out;
+		}
 		if (this_name_len > name_len) {
 			char *new_name;