[PATCH v3 0/9] btrfs: check name_len before read name

[PATCH v3 0/9] btrfs: check name_len before read name

[Su Yue]

When reading out name from inode_ref, dir_item, it's possible that
corrupted name_len leads to read beyond boundary.
Since there are already patches for btrfs-progs, this patchset is
for btrfs.

Introduce 'btrfs_is_name_len_valid' to make check name_len with
item boundary.
If read name from dir_item, use 'verify_dir_item' to do more strict
check. Otherwise, use 'btrfs_is_name_len_valid'.

It's unnessary to do check before every read/memcmp_extent_buffer name.
Checking name_len when read name for the first time in the call graph is
enough.

Changlog:
v2:
	1.Change 'btrfs_check_namelen' to 'btrfs_is_namelen_valid'.
	2.Split patches according call graph.
v3:
	1.Add cases about BTRFS_ROOT_REF_KEY and BTRFS_ROOT_BACKREF_KEY.
	2.Add more comments about how/where extent_buffer is to be read
	for the first time.
	3.Change 'namelen' to 'name_len' in function and changelog.

Su Yue (9):
  btrfs: Introduce btrfs_is_name_len_valid to avoid reading beyond
    boundary
  btrfs: Check name_len with boundary in verify dir_item
  btrfs: Check name_len on add_inode_ref call path
  btrfs: Verify dir_item in replay_xattr_deletes
  btrfs: Check name_len in btrfs_check_ref_name_override
  btrfs: Check name_len before read in iterate_dir_item
  btrfs: Check name_len before read in btrfs_get_name
  btrfs: Check name_len before in btrfs_del_root_ref
  btrfs: Verify dir_item in iterate_object_props

 fs/btrfs/ctree.h     |  4 ++-
 fs/btrfs/dir-item.c  | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 fs/btrfs/export.c    |  5 ++++
 fs/btrfs/inode.c     |  2 +-
 fs/btrfs/props.c     |  7 +++++
 fs/btrfs/root-tree.c |  7 +++++
 fs/btrfs/send.c      |  6 ++++
 fs/btrfs/tree-log.c  | 44 ++++++++++++++++++++-------
 fs/btrfs/xattr.c     |  2 +-
 9 files changed, 146 insertions(+), 16 deletions(-)

-- 
2.13.0

[PATCH v3 1/9] btrfs: Introduce btrfs_is_name_len_valid to avoid reading beyond boundary

[Su Yue]

Introduce function btrfs_is_name_len_valid.

The function compares arg @name_len with item boundary then returns value
represents name_len is valid or not.

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

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 643c70d2b2e6..776b16ec7a47 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -3037,6 +3037,8 @@ struct btrfs_dir_item *btrfs_match_dir_item_name(struct btrfs_fs_info *fs_info,
 						 struct btrfs_path *path,
 						 const char *name,
 						 int name_len);
+bool btrfs_is_name_len_valid(struct extent_buffer *leaf, int slot,
+			     unsigned long start, u16 name_len);
 
 /* orphan.c */
 int btrfs_insert_orphan_item(struct btrfs_trans_handle *trans,
diff --git a/fs/btrfs/dir-item.c b/fs/btrfs/dir-item.c
index c24d615e3d7f..b54c920c8412 100644
--- a/fs/btrfs/dir-item.c
+++ b/fs/btrfs/dir-item.c
@@ -484,3 +484,76 @@ int verify_dir_item(struct btrfs_fs_info *fs_info,
 
 	return 0;
 }
+
+bool btrfs_is_name_len_valid(struct extent_buffer *leaf, int slot,
+			     unsigned long start, u16 name_len)
+{
+	struct btrfs_fs_info *fs_info = leaf->fs_info;
+	struct btrfs_key key;
+	u32 read_start;
+	u32 read_end;
+	u32 item_start;
+	u32 item_end;
+	u32 size;
+	bool ret = true;
+
+	ASSERT(start > btrfs_leaf_data(leaf));
+
+	read_start = start - btrfs_leaf_data(leaf);
+	read_end = read_start + name_len;
+	item_start = btrfs_item_offset_nr(leaf, slot);
+	item_end = btrfs_item_end_nr(leaf, slot);
+
+	btrfs_item_key_to_cpu(leaf, &key, slot);
+
+	switch (key.type) {
+	case BTRFS_DIR_ITEM_KEY:
+	case BTRFS_XATTR_ITEM_KEY:
+	case BTRFS_DIR_INDEX_KEY:
+		size = sizeof(struct btrfs_dir_item);
+		break;
+	case BTRFS_INODE_REF_KEY:
+		size = sizeof(struct btrfs_inode_ref);
+		break;
+	case BTRFS_INODE_EXTREF_KEY:
+		size = sizeof(struct btrfs_inode_extref);
+		break;
+	case BTRFS_ROOT_REF_KEY:
+	case BTRFS_ROOT_BACKREF_KEY:
+		size = sizeof(struct btrfs_root_ref);
+		break;
+	default:
+		ret = false;
+		goto out;
+	}
+
+	if (read_start < item_start) {
+		ret = false;
+		goto out;
+	}
+	if (read_end > item_end) {
+		ret = false;
+		goto out;
+	}
+
+	/* there shall be item(s) before name */
+	if (read_start - item_start < size) {
+		ret = false;
+		goto out;
+	}
+
+	/*
+	 * This may be the last item in the slot
+	 * Or same type item(s) is left between read_end and item_end
+	 */
+	if (item_end != read_end &&
+	    item_end - read_end < size) {
+		ret = false;
+		goto out;
+	}
+out:
+	if (!ret)
+		btrfs_crit(fs_info, "invalid dir item name len: %u",
+			   (unsigned int)name_len);
+	return ret;
+}
-- 
2.13.0

[PATCH v3 2/9] btrfs: Check name_len with boundary in verify dir_item

[Su Yue]

Origin 'verify_dir_item' verifies name_len of dir_item with fixed values
but no item boundary.
If corrupted name_len 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.

Example:
	1. Corrupt one dir_item name_len 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 ]---

Solve it by adding a parameter @slot and check name_len with item boundary
by calling 'btrfs_is_name_len_valid'.

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

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 776b16ec7a47..98a69c58fe57 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 b54c920c8412..806b9fe0178a 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;
+	int ret;
 	u8 type = btrfs_dir_type(leaf, dir_item);
 
 	if (type >= BTRFS_FT_MAX) {
@@ -472,6 +474,12 @@ int verify_dir_item(struct btrfs_fs_info *fs_info,
 		return 1;
 	}
 
+	namelen = btrfs_dir_name_len(leaf, dir_item);
+	ret = btrfs_is_name_len_valid(leaf, slot,
+				      (unsigned long)(dir_item + 1), namelen);
+	if (!ret)
+		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

[PATCH v3 3/9] btrfs: Check name_len on add_inode_ref call path

[Su Yue]

'replay_one_buffer' first reads buffers and dispatches items accroding
item type.
In this patch, 'add_inode_ref' handles inode_ref and inode_extref.
Then 'add_inode_ref' calls 'ref_get_fields' and 'extref_get_fields' to read
ref/extref name for the first time.
So Checking name_len before read in those two is fine.

'add_inode_ref' also calls 'inode_in_dir' to match inode_ref/extef in
parent_dir.
The call graph includes 'btrfs_match_dir_item_name' to read dir_item name
in the parent dir.
Checking first dir_item is not enough. Change it to verify every
dir_item while doing matches.

Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
---
 fs/btrfs/dir-item.c |  4 ++--
 fs/btrfs/tree-log.c | 27 ++++++++++++++++++---------
 2 files changed, 20 insertions(+), 11 deletions(-)

diff --git a/fs/btrfs/dir-item.c b/fs/btrfs/dir-item.c
index 806b9fe0178a..dc42066cbaeb 100644
--- a/fs/btrfs/dir-item.c
+++ b/fs/btrfs/dir-item.c
@@ -395,8 +395,6 @@ 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) {
@@ -405,6 +403,8 @@ struct btrfs_dir_item *btrfs_match_dir_item_name(struct btrfs_fs_info *fs_info,
 			btrfs_dir_data_len(leaf, dir_item);
 		name_ptr = (unsigned long)(dir_item + 1);
 
+		if (verify_dir_item(fs_info, leaf, path->slots[0], dir_item))
+			return NULL;
 		if (btrfs_dir_name_len(leaf, dir_item) == name_len &&
 		    memcmp_extent_buffer(leaf, name, name_ptr, name_len) == 0)
 			return dir_item;
diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index 1930f28edcdd..11cf38fb3a49 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -1175,15 +1175,19 @@ 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;
 
 	extref = (struct btrfs_inode_extref *)ref_ptr;
 
 	*namelen = btrfs_inode_extref_name_len(eb, extref);
+	if (!btrfs_is_name_len_valid(eb, slot, (unsigned long)&extref->name,
+				     *namelen))
+		return -EIO;
+
 	*name = kmalloc(*namelen, GFP_NOFS);
 	if (*name == NULL)
 		return -ENOMEM;
@@ -1198,14 +1202,19 @@ 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;
 
 	ref = (struct btrfs_inode_ref *)ref_ptr;
 
 	*namelen = btrfs_inode_ref_name_len(eb, ref);
+	if (!btrfs_is_name_len_valid(eb, slot, (unsigned long)(ref + 1),
+				     *namelen))
+		return -EIO;
+
 	*name = kmalloc(*namelen, GFP_NOFS);
 	if (*name == NULL)
 		return -ENOMEM;
@@ -1280,8 +1289,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,8 +1302,8 @@ static noinline int add_inode_ref(struct btrfs_trans_handle *trans,
 				goto out;
 			}
 		} else {
-			ret = ref_get_fields(eb, ref_ptr, &namelen, &name,
-					     &ref_index);
+			ret = ref_get_fields(eb, slot, ref_ptr, &namelen,
+					     &name, &ref_index);
 		}
 		if (ret)
 			goto out;
-- 
2.13.0

[PATCH v3 4/9] btrfs: Verify dir_item in replay_xattr_deletes

[Su Yue]

'replay_xattr_deletes' calls 'btrfs_search_slot' to get buffer and
reads name.

Call 'verify_dir_item' to check name_len in 'replay_xattr_deletes'
in avoid of read out of boundary.

Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
---
 fs/btrfs/tree-log.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index 11cf38fb3a49..06c7ceb07282 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -2111,6 +2111,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;
@@ -2152,6 +2153,12 @@ static int replay_xattr_deletes(struct btrfs_trans_handle *trans,
 			u32 this_len = sizeof(*di) + name_len + data_len;
 			char *name;
 
+			ret = verify_dir_item(fs_info, path->nodes[0],
+					      path->slots[0], di);
+			if (ret) {
+				ret = -EIO;
+				goto out;
+			}
 			name = kmalloc(name_len, GFP_NOFS);
 			if (!name) {
 				ret = -ENOMEM;
-- 
2.13.0

[PATCH v3 5/9] btrfs: Check name_len in btrfs_check_ref_name_override

[Su Yue]

In 'btrfs_log_inode', 'btrfs_search_forward' gets the buffer and then
'btrfs_check_ref_name_override' will read name from inode_ref/inode_extref
for the first time.

Call 'btrfs_is_name_len_valid' before reading name.

Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
---
 fs/btrfs/tree-log.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index 06c7ceb07282..f20ef211a73d 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -4562,6 +4562,12 @@ static int btrfs_check_ref_name_override(struct extent_buffer *eb,
 			this_len = sizeof(*extref) + this_name_len;
 		}
 
+		ret = btrfs_is_name_len_valid(eb, slot, name_ptr,
+					      this_name_len);
+		if (!ret) {
+			ret = -EIO;
+			goto out;
+		}
 		if (this_name_len > name_len) {
 			char *new_name;
 
-- 
2.13.0

[PATCH v3 6/9] btrfs: Check name_len before read in iterate_dir_item

[Su Yue]

Since 'iterate_dir_item' checks name_len in its way,
so use 'btrfs_is_name_len_valid' not 'verify_dir_item' to make more strict
name_len check.

Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
---
 fs/btrfs/send.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index fc496a6f842a..f0e33f7f221e 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -1069,6 +1069,12 @@ static int iterate_dir_item(struct btrfs_root *root, struct btrfs_path *path,
 			}
 		}
 
+		ret = btrfs_is_name_len_valid(eb, path->slots[0],
+			  (unsigned long)(di + 1), name_len + data_len);
+		if (!ret) {
+			ret = -ENAMETOOLONG;
+			goto out;
+		}
 		if (name_len + data_len > buf_len) {
 			buf_len = name_len + data_len;
 			if (is_vmalloc_addr(buf)) {
-- 
2.13.0

[PATCH v3 7/9] btrfs: Check name_len before read in btrfs_get_name

[Su Yue]

In 'btrfs_get_name', it does 'btrfs_search_slot' and reads name from
inode_ref/root_ref.

Call btrfs_is_name_len_valid in btrfs_get_name.

Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
---
 fs/btrfs/export.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/fs/btrfs/export.c b/fs/btrfs/export.c
index 87144c9f9593..fa66980726c9 100644
--- a/fs/btrfs/export.c
+++ b/fs/btrfs/export.c
@@ -282,6 +282,11 @@ static int btrfs_get_name(struct dentry *parent, char *name,
 		name_len = btrfs_inode_ref_name_len(leaf, iref);
 	}
 
+	ret = btrfs_is_name_len_valid(leaf, path->slots[0], name_ptr, name_len);
+	if (!ret) {
+		btrfs_free_path(path);
+		return -EIO;
+	}
 	read_extent_buffer(leaf, name, name_ptr, name_len);
 	btrfs_free_path(path);
 
-- 
2.13.0

[PATCH v3 8/9] btrfs: Check name_len before in btrfs_del_root_ref

[Su Yue]

'btrfs_del_root_ref' does search_slot and reads name from root_ref.
Call 'btrfs_is_name_len_valid' before memcmp.

Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
---
 fs/btrfs/root-tree.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c
index 7d6bc308bf43..460db0cb2d07 100644
--- a/fs/btrfs/root-tree.c
+++ b/fs/btrfs/root-tree.c
@@ -390,6 +390,13 @@ 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);
+		ret = btrfs_is_name_len_valid(leaf, path->slots[0], ptr,
+					      name_len);
+		if (!ret) {
+			err = -EIO;
+			goto out;
+		}
+
 		WARN_ON(memcmp_extent_buffer(leaf, name, ptr, name_len));
 		*sequence = btrfs_root_ref_sequence(leaf, ref);
 
-- 
2.13.0

[PATCH v3 9/9] btrfs: Verify dir_item in iterate_object_props

[Su Yue]

Call 'Verify dir_item' before 'memcmp_extent_buffer' reading name from
dir_item.

Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
---
 fs/btrfs/props.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
index d6cb155ef7a1..4b23ae5d0e5c 100644
--- a/fs/btrfs/props.c
+++ b/fs/btrfs/props.c
@@ -164,6 +164,7 @@ static int iterate_object_props(struct btrfs_root *root,
 						 size_t),
 				void *ctx)
 {
+	struct btrfs_fs_info *fs_info = root->fs_info;
 	int ret;
 	char *name_buf = NULL;
 	char *value_buf = NULL;
@@ -214,6 +215,12 @@ static int iterate_object_props(struct btrfs_root *root,
 			name_ptr = (unsigned long)(di + 1);
 			data_ptr = name_ptr + name_len;
 
+			if (verify_dir_item(fs_info, leaf,
+					    path->slots[0], di)) {
+				ret = -EIO;
+				goto out;
+			}
+
 			if (name_len <= XATTR_BTRFS_PREFIX_LEN ||
 			    memcmp_extent_buffer(leaf, XATTR_BTRFS_PREFIX,
 						 name_ptr,
-- 
2.13.0

Re: [PATCH v3 1/9] btrfs: Introduce btrfs_is_name_len_valid to avoid reading beyond boundary

[David Sterba]

On Tue, Jun 06, 2017 at 05:57:00PM +0800, Su Yue wrote:
> +	if (item_end != read_end &&
> +	    item_end - read_end < size) {

This fits on line, no need to split. Fixed.

Re: [PATCH v3 6/9] btrfs: Check name_len before read in iterate_dir_item

[David Sterba]

On Tue, Jun 06, 2017 at 05:57:05PM +0800, Su Yue wrote:
> Since 'iterate_dir_item' checks name_len in its way,
> so use 'btrfs_is_name_len_valid' not 'verify_dir_item' to make more strict
> name_len check.
> 
> Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
> ---
>  fs/btrfs/send.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
> index fc496a6f842a..f0e33f7f221e 100644
> --- a/fs/btrfs/send.c
> +++ b/fs/btrfs/send.c
> @@ -1069,6 +1069,12 @@ static int iterate_dir_item(struct btrfs_root *root, struct btrfs_path *path,
>  			}
>  		}
>  
> +		ret = btrfs_is_name_len_valid(eb, path->slots[0],
> +			  (unsigned long)(di + 1), name_len + data_len);
> +		if (!ret) {
> +			ret = -ENAMETOOLONG;

Nikolai pointed out in previous patch iteration that this should be EIO,
I'm fixing it at commit time.

Re: [PATCH v3 0/9] btrfs: check name_len before read name

[David Sterba]

On Tue, Jun 06, 2017 at 05:56:59PM +0800, Su Yue wrote:
> When reading out name from inode_ref, dir_item, it's possible that
> corrupted name_len leads to read beyond boundary.
> Since there are already patches for btrfs-progs, this patchset is
> for btrfs.
> 
> Introduce 'btrfs_is_name_len_valid' to make check name_len with
> item boundary.
> If read name from dir_item, use 'verify_dir_item' to do more strict
> check. Otherwise, use 'btrfs_is_name_len_valid'.
> 
> It's unnessary to do check before every read/memcmp_extent_buffer name.
> Checking name_len when read name for the first time in the call graph is
> enough.
> 
> Changlog:
> v2:
> 	1.Change 'btrfs_check_namelen' to 'btrfs_is_namelen_valid'.
> 	2.Split patches according call graph.
> v3:
> 	1.Add cases about BTRFS_ROOT_REF_KEY and BTRFS_ROOT_BACKREF_KEY.
> 	2.Add more comments about how/where extent_buffer is to be read
> 	for the first time.
> 	3.Change 'namelen' to 'name_len' in function and changelog.

Thanks, overall looks good to me now. I've edited the changelogs, the
quoting of functions looks a bit strange to read, I hope you don't mind
me doing that.

I'll add the branch to for-next again, will add more reviewed-by should
they come. You don't need to resend the patchset, but if you find
something to fix, please let me know, we'll see if it's worth a separate
patch or an in-place fix.

Re: [PATCH v3 0/9] btrfs: check name_len before read name

[David Sterba]

On Tue, Jun 06, 2017 at 05:56:59PM +0800, Su Yue wrote:
> When reading out name from inode_ref, dir_item, it's possible that
> corrupted name_len leads to read beyond boundary.
> Since there are already patches for btrfs-progs, this patchset is
> for btrfs.
> 
> Introduce 'btrfs_is_name_len_valid' to make check name_len with
> item boundary.
> If read name from dir_item, use 'verify_dir_item' to do more strict
> check. Otherwise, use 'btrfs_is_name_len_valid'.
> 
> It's unnessary to do check before every read/memcmp_extent_buffer name.
> Checking name_len when read name for the first time in the call graph is
> enough.
> 
> Changlog:
> v2:
> 	1.Change 'btrfs_check_namelen' to 'btrfs_is_namelen_valid'.
> 	2.Split patches according call graph.
> v3:
> 	1.Add cases about BTRFS_ROOT_REF_KEY and BTRFS_ROOT_BACKREF_KEY.
> 	2.Add more comments about how/where extent_buffer is to be read
> 	for the first time.
> 	3.Change 'namelen' to 'name_len' in function and changelog.

I still see some fstests fail, eg. btrfs/048

trfs/048 30s ...       [18:05:48] [18:06:18] - output mismatch (see /root/test/mmtests/work/testdisk/sources/xfstests-git-installed/results//btrfs/048.out.bad)
    --- tests/btrfs/048.out     2017-04-24 16:56:02.268104570 +0200
    +++ /root/test/mmtests/work/testdisk/sources/xfstests-git-installed/results//btrfs/048.out.bad      2017-06-15 18:06:18.996635908 +0200
    @@ -30,22 +30,13 @@
     ERROR: failed to set compression for SCRATCH_MNT/testdir/file1: Invalid argument
     ***
     compression=lzo
    -compression=lzo
    -compression=zlib
     ***
    -compression=lzo
    ...

in syslog with several lines

[ 2156.970285] BTRFS critical (device sdb6): invalid dir item name len: 17

and

[ 2157.220219] BTRFS error (device sdb6): error loading props for ino 259 (root 5): -5

similarly btrfs/059 fails. Have you seen such errors during your testing?

Re: [PATCH v3 0/9] btrfs: check name_len before read name

[Su Yue]

On 06/15/2017 11:57 PM, David Sterba wrote:
> On Tue, Jun 06, 2017 at 05:56:59PM +0800, Su Yue wrote:
>> When reading out name from inode_ref, dir_item, it's possible that
>> corrupted name_len leads to read beyond boundary.
>> Since there are already patches for btrfs-progs, this patchset is
>> for btrfs.
>>
>> Introduce 'btrfs_is_name_len_valid' to make check name_len with
>> item boundary.
>> If read name from dir_item, use 'verify_dir_item' to do more strict
>> check. Otherwise, use 'btrfs_is_name_len_valid'.
>>
>> It's unnessary to do check before every read/memcmp_extent_buffer name.
>> Checking name_len when read name for the first time in the call graph is
>> enough.
>>
>> Changlog:
>> v2:
>> 	1.Change 'btrfs_check_namelen' to 'btrfs_is_namelen_valid'.
>> 	2.Split patches according call graph.
>> v3:
>> 	1.Add cases about BTRFS_ROOT_REF_KEY and BTRFS_ROOT_BACKREF_KEY.
>> 	2.Add more comments about how/where extent_buffer is to be read
>> 	for the first time.
>> 	3.Change 'namelen' to 'name_len' in function and changelog.
> 
> I still see some fstests fail, eg. btrfs/048
> 
> trfs/048 30s ...       [18:05:48] [18:06:18] - output mismatch (see /root/test/mmtests/work/testdisk/sources/xfstests-git-installed/results//btrfs/048.out.bad)
>      --- tests/btrfs/048.out     2017-04-24 16:56:02.268104570 +0200
>      +++ /root/test/mmtests/work/testdisk/sources/xfstests-git-installed/results//btrfs/048.out.bad      2017-06-15 18:06:18.996635908 +0200
>      @@ -30,22 +30,13 @@
>       ERROR: failed to set compression for SCRATCH_MNT/testdir/file1: Invalid argument
>       ***
>       compression=lzo
>      -compression=lzo
>      -compression=zlib
>       ***
>      -compression=lzo
>      ...
> 
> in syslog with several lines
> 
> [ 2156.970285] BTRFS critical (device sdb6): invalid dir item name len: 17
> 
> and
> 
> [ 2157.220219] BTRFS error (device sdb6): error loading props for ino 259 (root 5): -5
> 
> similarly btrfs/059 fails. Have you seen such errors during your testing?
> 
> 
TBH, I just realized I messed up the test because of wrong git operations.
The problem is caused by that btrfs_is_name_len_vallid() does not handle 
data_len in XATTR_ITEM.
Sorry for my carelessness.