[PATCH 1/3] btrfs: Introduce btrfs_check_namelen to avoid reading beyond boundary

[PATCH 1/3] btrfs: Introduce btrfs_check_namelen to avoid reading beyond boundary

[Su Yue]

When reading out name from inode_ref, dir_item, it's possible that
corrupted name_len lead to read beyond boundary.

Since there are already patches for btrfs-progs, this is for btrfs.

Introduce function btrfs_check_namelen, it should be called before reading
name from extent_buffer.
The function compares arg @namelen with boundary then returns 'proper'
namelen.

Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
---
 fs/btrfs/ctree.h    |  2 ++
 fs/btrfs/dir-item.c | 22 ++++++++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 643c70d2b2e6..f49e04e7612b 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -3074,6 +3074,8 @@ int btrfs_find_name_in_ext_backref(struct btrfs_path *path,
 				   int name_len,
 				   struct btrfs_inode_extref **extref_ret);
 
+u16 btrfs_check_namelen(struct extent_buffer *leaf, int slot,
+			unsigned long start, u32 namelen);
 /* file-item.c */
 struct btrfs_dio_private;
 int btrfs_del_csums(struct btrfs_trans_handle *trans,
diff --git a/fs/btrfs/dir-item.c b/fs/btrfs/dir-item.c
index c24d615e3d7f..7af5ad8e9a3c 100644
--- a/fs/btrfs/dir-item.c
+++ b/fs/btrfs/dir-item.c
@@ -484,3 +484,25 @@ int verify_dir_item(struct btrfs_fs_info *fs_info,
 
 	return 0;
 }
+
+/*
+ * Returns >0: the value @namelen after cut according item boundary
+ * Returns  0: on error
+ */
+u16 btrfs_check_namelen(struct extent_buffer *leaf, int slot,
+			unsigned long start, u32 namelen)
+{
+	u32 end;
+	u64 ret = namelen;
+
+	end = btrfs_leaf_data(leaf) + btrfs_item_end_nr(leaf, slot);
+
+	if (start > end)
+		return 0;
+	if (start + namelen > end) {
+		ret = end - start;
+		if (ret > U16_MAX)
+			ret = 0;
+	}
+	return ret;
+}
-- 
2.13.0

[PATCH 2/3] btrfs: check namelen with boundary in verify dir_item

[Su Yue]

Origin 'verify_dir_item' verify namelen of dir_item with fixed values
but no item boundary.
If corrupted namelen was not bigger than the fixed value, for example 255,
the function will think the dir_item is fine. And then reading beyond
boundary will cause crash.

Add a parameter 'slot' and check namelen with item boundary by
calling 'btrfs_check_namelen'.

Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
---
 fs/btrfs/ctree.h    |  2 +-
 fs/btrfs/dir-item.c | 13 ++++++++++++-
 fs/btrfs/inode.c    |  2 +-
 fs/btrfs/tree-log.c |  4 ++--
 fs/btrfs/xattr.c    |  2 +-
 5 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index f49e04e7612b..99dd2c25ac6b 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -3031,7 +3031,7 @@ struct btrfs_dir_item *btrfs_lookup_xattr(struct btrfs_trans_handle *trans,
 					  const char *name, u16 name_len,
 					  int mod);
 int verify_dir_item(struct btrfs_fs_info *fs_info,
-		    struct extent_buffer *leaf,
+		    struct extent_buffer *leaf, int slot,
 		    struct btrfs_dir_item *dir_item);
 struct btrfs_dir_item *btrfs_match_dir_item_name(struct btrfs_fs_info *fs_info,
 						 struct btrfs_path *path,
diff --git a/fs/btrfs/dir-item.c b/fs/btrfs/dir-item.c
index 7af5ad8e9a3c..82390f36101c 100644
--- a/fs/btrfs/dir-item.c
+++ b/fs/btrfs/dir-item.c
@@ -395,7 +395,7 @@ 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, 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]);
@@ -453,9 +453,11 @@ int btrfs_delete_one_dir_name(struct btrfs_trans_handle *trans,
 
 int verify_dir_item(struct btrfs_fs_info *fs_info,
 		    struct extent_buffer *leaf,
+		    int slot,
 		    struct btrfs_dir_item *dir_item)
 {
 	u16 namelen = BTRFS_NAME_LEN;
+	u16 namelen_ret;
 	u8 type = btrfs_dir_type(leaf, dir_item);
 
 	if (type >= BTRFS_FT_MAX) {
@@ -472,6 +474,15 @@ int verify_dir_item(struct btrfs_fs_info *fs_info,
 		return 1;
 	}
 
+	namelen = btrfs_dir_name_len(leaf, dir_item);
+	namelen_ret = btrfs_check_namelen(leaf, slot,
+			(unsigned long)(dir_item + 1), namelen);
+	if (namelen_ret != namelen) {
+		btrfs_crit(fs_info, "invalid dir item name len: %u",
+		       (unsigned)btrfs_dir_name_len(leaf, dir_item));
+		return 1;
+	}
+
 	/* BTRFS_MAX_XATTR_SIZE is the same for all dir items */
 	if ((btrfs_dir_data_len(leaf, dir_item) +
 	     btrfs_dir_name_len(leaf, dir_item)) >
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 17cbe9306faf..df948569c393 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -5934,7 +5934,7 @@ static int btrfs_real_readdir(struct file *file, struct dir_context *ctx)
 		ctx->pos = found_key.offset;
 
 		di = btrfs_item_ptr(leaf, slot, struct btrfs_dir_item);
-		if (verify_dir_item(fs_info, leaf, di))
+		if (verify_dir_item(fs_info, leaf, slot, di))
 			goto next;
 
 		name_len = btrfs_dir_name_len(leaf, di);
diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index ccfe9fe7754a..1930f28edcdd 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -1841,7 +1841,7 @@ static noinline int replay_one_dir_item(struct btrfs_trans_handle *trans,
 	ptr_end = ptr + item_size;
 	while (ptr < ptr_end) {
 		di = (struct btrfs_dir_item *)ptr;
-		if (verify_dir_item(fs_info, eb, di))
+		if (verify_dir_item(fs_info, eb, slot, di))
 			return -EIO;
 		name_len = btrfs_dir_name_len(eb, di);
 		ret = replay_one_name(trans, root, path, eb, di, key);
@@ -2017,7 +2017,7 @@ static noinline int check_item_in_log(struct btrfs_trans_handle *trans,
 	ptr_end = ptr + item_size;
 	while (ptr < ptr_end) {
 		di = (struct btrfs_dir_item *)ptr;
-		if (verify_dir_item(fs_info, eb, di)) {
+		if (verify_dir_item(fs_info, eb, slot, di)) {
 			ret = -EIO;
 			goto out;
 		}
diff --git a/fs/btrfs/xattr.c b/fs/btrfs/xattr.c
index b3cbf80c5acf..2c7e53f9ff1b 100644
--- a/fs/btrfs/xattr.c
+++ b/fs/btrfs/xattr.c
@@ -336,7 +336,7 @@ ssize_t btrfs_listxattr(struct dentry *dentry, char *buffer, size_t size)
 			u32 this_len = sizeof(*di) + name_len + data_len;
 			unsigned long name_ptr = (unsigned long)(di + 1);
 
-			if (verify_dir_item(fs_info, leaf, di)) {
+			if (verify_dir_item(fs_info, leaf, slot, di)) {
 				ret = -EIO;
 				goto err;
 			}
-- 
2.13.0

Re: [PATCH 2/3] btrfs: check namelen with boundary in verify dir_item

[David Sterba]

On Thu, May 25, 2017 at 10:09:07AM +0800, Su Yue wrote:
> @@ -472,6 +474,15 @@ int verify_dir_item(struct btrfs_fs_info *fs_info,
>  		return 1;
>  	}
>  
> +	namelen = btrfs_dir_name_len(leaf, dir_item);
> +	namelen_ret = btrfs_check_namelen(leaf, slot,
> +			(unsigned long)(dir_item + 1), namelen);
> +	if (namelen_ret != namelen) {
> +		btrfs_crit(fs_info, "invalid dir item name len: %u",
> +		       (unsigned)btrfs_dir_name_len(leaf, dir_item));
> +		return 1;
> +	}

Extending verify_dir_item is ok, as we always have the slot parameter to
pass, no problem here.

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

[Su Yue]

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(-)

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;
 
-- 
2.13.0

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

[David Sterba]

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.

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

[Su Yue]

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.
> 

Re: [PATCH 1/3] btrfs: Introduce btrfs_check_namelen to avoid reading beyond boundary

[David Sterba]

On Thu, May 25, 2017 at 10:09:06AM +0800, Su Yue wrote:
> When reading out name from inode_ref, dir_item, it's possible that
> corrupted name_len lead to read beyond boundary.
> 
> Since there are already patches for btrfs-progs, this is for btrfs.
> 
> Introduce function btrfs_check_namelen, it should be called before reading
> name from extent_buffer.
> The function compares arg @namelen with boundary then returns 'proper'
> namelen.
> 
> Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>

Such validation is useful, but I'm concerned about the proposed
implementation and usage pattern.

> +/*
> + * Returns >0: the value @namelen after cut according item boundary
> + * Returns  0: on error
> + */
> +u16 btrfs_check_namelen(struct extent_buffer *leaf, int slot,
> +			unsigned long start, u32 namelen)

This function does not match its name, it does not check, but somehow
sanitizes the input length read from the leaf. From a "check" I'd expect
some good/bad result, so it could be used in an if statement.

> +{
> +	u32 end;
> +	u64 ret = namelen;
> +
> +	end = btrfs_leaf_data(leaf) + btrfs_item_end_nr(leaf, slot);
> +
> +	if (start > end)
> +		return 0;
> +	if (start + namelen > end) {
> +		ret = end - start;
> +		if (ret > U16_MAX)

Where does this limit come from?

> +			ret = 0;
> +	}
> +	return ret;

ret is u64, function returns u16.

> +}

Re: [PATCH 1/3] btrfs: Introduce btrfs_check_namelen to avoid reading beyond boundary

[David Sterba]

On Mon, May 29, 2017 at 05:22:43PM +0200, David Sterba wrote:
> On Thu, May 25, 2017 at 10:09:06AM +0800, Su Yue wrote:
> > When reading out name from inode_ref, dir_item, it's possible that
> > corrupted name_len lead to read beyond boundary.
> > 
> > Since there are already patches for btrfs-progs, this is for btrfs.
> > 
> > Introduce function btrfs_check_namelen, it should be called before reading
> > name from extent_buffer.
> > The function compares arg @namelen with boundary then returns 'proper'
> > namelen.
> > 
> > Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
> 
> Such validation is useful, but I'm concerned about the proposed
> implementation and usage pattern.

After reading the other patches again, I think the function name can
stay but will return bool and verifies if the namelen parameter matches
the verified value. That way you can get rid of all the local variables
and checks everywhere.

That way the additional check won't be missed like in this hunk from
patch 3:

@@ -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) {

namelen_ret is set but unused.