[PATCH v2 0/9] btrfs: check namelen before read name

[PATCH v2 0/9] btrfs: check namelen 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_namelen_valid' to make check namelen with
item boundary.
If read name from dir_item, use 'verify_dir_item' to do more strict
check. Otherwise, use 'btrfs_is_namelen_valid'.

It's unnessary to do check before every read/memcmp_extent_buffer name.
Checking namelen 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.

Su Yue (9):
  btrfs: Introduce btrfs_is_namelen_valid to avoid reading beyond
    boundary
  btrfs: Check namelen 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 namelen in 'btrfs_check_ref_name_override'
  btrfs: Check name before read in 'iterate_dir_item'
  btrfs: Check namelen before read in 'btrfs_get_name'
  btrfs: Check namelen before in 'btrfs_del_root_ref'
  btrfs: Verify dir_item 'in iterate_object_props'

 fs/btrfs/ctree.h     |  4 ++-
 fs/btrfs/dir-item.c  | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 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  | 43 ++++++++++++++++++-------
 fs/btrfs/xattr.c     |  2 +-
 9 files changed, 149 insertions(+), 16 deletions(-)

-- 
2.13.0

[PATCH v2 1/9] btrfs: Introduce btrfs_is_namelen_valid to avoid reading beyond boundary

[Su Yue]

Introduce function btrfs_is_namelen_valid.

The function compares arg @namelen with item boundary then returns value
represents namelen 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..70d8778f849b 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_namelen_valid(struct extent_buffer *leaf, int slot,
+			    unsigned long start, u16 namelen);
 
 /* 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..fbda228192ed 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_namelen_valid(struct extent_buffer *leaf, int slot,
+			    unsigned long start, u16 namelen)
+{
+	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 + namelen;
+	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;
+	default:
+		size = 0;
+	}
+
+	if (!size) {
+		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)namelen);
+	return ret;
+}
-- 
2.13.0

Re: [PATCH v2 1/9] btrfs: Introduce btrfs_is_namelen_valid to avoid reading beyond boundary

[Nikolay Borisov]

On  1.06.2017 11:57, Su Yue wrote:
> Introduce function btrfs_is_namelen_valid.
> 
> The function compares arg @namelen with item boundary then returns value
> represents namelen 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..70d8778f849b 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_namelen_valid(struct extent_buffer *leaf, int slot,
> +			    unsigned long start, u16 namelen);
>  
>  /* 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..fbda228192ed 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_namelen_valid(struct extent_buffer *leaf, int slot,
> +			    unsigned long start, u16 namelen)
> +{
> +	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 + namelen;
> +	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;
> +	default:
> +		size = 0;
> +	}
> +
> +	if (!size) {
> +		ret = false;
> +		goto out;
> +	}

Why don't you fold this check directly in the 'default' case in the
switch. It makes more clear the intention that this function only
handles the items, explicitly listed in the 'case' statements.

> +
> +	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)namelen);
> +	return ret;
> +}
> 

Re: [PATCH v2 1/9] btrfs: Introduce btrfs_is_namelen_valid to avoid reading beyond boundary

[Su Yue]

On 06/01/2017 05:44 PM, Nikolay Borisov wrote:
> 
> 
> On  1.06.2017 11:57, Su Yue wrote:
>> Introduce function btrfs_is_namelen_valid.
>>
>> The function compares arg @namelen with item boundary then returns value
>> represents namelen 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..70d8778f849b 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_namelen_valid(struct extent_buffer *leaf, int slot,
>> +			    unsigned long start, u16 namelen);
>>   
>>   /* 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..fbda228192ed 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_namelen_valid(struct extent_buffer *leaf, int slot,
>> +			    unsigned long start, u16 namelen)
>> +{
>> +	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 + namelen;
>> +	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;
>> +	default:
>> +		size = 0;
>> +	}
>> +
>> +	if (!size) {
>> +		ret = false;
>> +		goto out;
>> +	}
> 
> Why don't you fold this check directly in the 'default' case in the
> switch. It makes more clear the intention that this function only
> handles the items, explicitly listed in the 'case' statements.
> 
Thanks for your advice! :)
>> +
>> +	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)namelen);
>> +	return ret;
>> +}
>>
> 
> 

[PATCH v2 2/9] 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.

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

Solve it by adding a parameter 'slot' and check namelen with item boundary
by calling 'btrfs_is_namelen_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 70d8778f849b..42e519728a94 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 fbda228192ed..f9d1ca76ca04 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_namelen_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 v2 3/9] btrfs: Check name len on add_inode_ref call path

[Su Yue]

'add_inode_ref' calls 'ref_get_fields' and 'extref_get_fields' to read
ref/extref name. Check namelen before read in those two.

The call path also includes 'btrfs_match_dir_item_name' to read
dir_item name in the parent dir.
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 f9d1ca76ca04..38dc5176cc5b 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..7d98858df44f 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_namelen_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_namelen_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

Re: [PATCH v2 3/9] btrfs: Check name len on add_inode_ref call path

[Nikolay Borisov]

On  1.06.2017 11:57, Su Yue wrote:
> 'add_inode_ref' calls 'ref_get_fields' and 'extref_get_fields' to read
> ref/extref name. Check namelen before read in those two.
> 
> The call path also includes 'btrfs_match_dir_item_name' to read
> dir_item name in the parent dir.
> 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 f9d1ca76ca04..38dc5176cc5b 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..7d98858df44f 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_namelen_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_namelen_valid(eb, slot, (unsigned long)(ref + 1),
> +				    *namelen))
> +		return -EIO;

I'd like to use this to raise a point - shouldn't btrfs actually try to
utilize a bit more the EUCLEAN error code. Both xfs/ext4 do define their
EFSCORRUPTED to EUCLEAN and signal that a structure is corrupted and
needs cleaning. Presumably when we btrfs_is_namelen_valid fails (or
other validation function) this means the data on=disk is corrupted
rather than there was an error during I/O which -EIO implies. Currently
btrfs uses EUCLEAN in only 3 instances in disk-io.c I'd like to solicit
opinions from other developers what their take on that is?


> +
>  	*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;
> 

Re: [PATCH v2 3/9] btrfs: Check name len on add_inode_ref call path

[David Sterba]

On Thu, Jun 01, 2017 at 12:53:53PM +0300, Nikolay Borisov wrote:
> > +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_namelen_valid(eb, slot, (unsigned long)(ref + 1),
> > +				    *namelen))
> > +		return -EIO;
> 
> I'd like to use this to raise a point - shouldn't btrfs actually try to
> utilize a bit more the EUCLEAN error code. Both xfs/ext4 do define their
> EFSCORRUPTED to EUCLEAN and signal that a structure is corrupted and
> needs cleaning. Presumably when we btrfs_is_namelen_valid fails (or
> other validation function) this means the data on=disk is corrupted
> rather than there was an error during I/O which -EIO implies. Currently
> btrfs uses EUCLEAN in only 3 instances in disk-io.c I'd like to solicit
> opinions from other developers what their take on that is?

We've come accross EUCLEAN in the past, eg. in this thread
https://lkml.kernel.org/r/1473870467-18721-1-git-send-email-bo.li.liu@oracle.com

Use of EUCLEAN is really sparse in btrfs and now everything is EIO,
while we could make a distinction between EIO and EUCLEAN. It "just"
needs to evaluate all callpaths if the errorcode is expected, the
semantics is defined and the behaviour is consistent with the existing
filesystems that use it.

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

[Su Yue]

Call 'verify_dir_item' to check namelen 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 7d98858df44f..ae51144571e2 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 v2 5/9] btrfs: Check namelen in 'btrfs_check_ref_name_override'

[Su Yue]

Call 'btrfs_is_namelen_valid' before reading name.

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

diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index ae51144571e2..fdab0f021197 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -4562,6 +4562,11 @@ static int btrfs_check_ref_name_override(struct extent_buffer *eb,
 			this_len = sizeof(*extref) + this_name_len;
 		}
 
+		ret = btrfs_is_namelen_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 v2 6/9] btrfs: Check name before read in 'iterate_dir_item'

[Su Yue]

Since 'iterate_dir_item' checks namelen in its way,
use 'btrfs_is_namelen_valid' not 'verify_dir_item'.

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

Re: [PATCH v2 6/9] btrfs: Check name before read in 'iterate_dir_item'

[Nikolay Borisov]

On  1.06.2017 11:57, Su Yue wrote:
> Since 'iterate_dir_item' checks namelen in its way,
> use 'btrfs_is_namelen_valid' not 'verify_dir_item'.
> 
> 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..caf96af106e6 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_namelen_valid(eb, path->slots[0],
> +			  (unsigned long)(di + 1), name_len + data_len);
> +		if (!ret) {
> +			ret = -ENAMETOOLONG;

In 5/9 and 7/9 the return values upon btrfs_is_namelen_valid failure are
different. Shouldn't the failure root cause (corrupted datastructures)
always be the same when btrfs_is_namelen_valid fails? E.g. in the case
of send we shouldn't really have entries which are ENAMETOOLONG, since
they should've been rejected at time they were originally created with
ENAMETOOLONG. And in case corruption happened and iterate_dir_item
observes failure from btrfs_is_namelen_valid then this should be
EIO/EUCLEAN ?


> +			goto out;
> +		}
>  		if (name_len + data_len > buf_len) {
>  			buf_len = name_len + data_len;
>  			if (is_vmalloc_addr(buf)) {
> 

Re: [PATCH v2 6/9] btrfs: Check name before read in 'iterate_dir_item'

[David Sterba]

On Thu, Jun 01, 2017 at 12:58:12PM +0300, Nikolay Borisov wrote:
> 
> 
> On  1.06.2017 11:57, Su Yue wrote:
> > Since 'iterate_dir_item' checks namelen in its way,
> > use 'btrfs_is_namelen_valid' not 'verify_dir_item'.
> > 
> > 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..caf96af106e6 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_namelen_valid(eb, path->slots[0],
> > +			  (unsigned long)(di + 1), name_len + data_len);
> > +		if (!ret) {
> > +			ret = -ENAMETOOLONG;
> 
> In 5/9 and 7/9 the return values upon btrfs_is_namelen_valid failure are
> different. Shouldn't the failure root cause (corrupted datastructures)
> always be the same when btrfs_is_namelen_valid fails? E.g. in the case
> of send we shouldn't really have entries which are ENAMETOOLONG, since
> they should've been rejected at time they were originally created with
> ENAMETOOLONG. And in case corruption happened and iterate_dir_item
> observes failure from btrfs_is_namelen_valid then this should be
> EIO/EUCLEAN ?

Agreed.

[PATCH v2 7/9] btrfs: Check namelen before read in 'btrfs_get_name'

[Su Yue]

Call 'btrfs_is_namelen_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..213e971e8dcf 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_namelen_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 v2 8/9] btrfs: Check namelen before in 'btrfs_del_root_ref'

[Su Yue]

Call btrfs_is_namelen_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..7a5450600723 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_namelen_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

Re: [PATCH v2 8/9] btrfs: Check namelen before in 'btrfs_del_root_ref'

[David Sterba]

On Thu, Jun 01, 2017 at 04:57:15PM +0800, Su Yue wrote:
> Call btrfs_is_namelen_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..7a5450600723 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_namelen_valid(leaf, path->slots[0], ptr,
> +					     name_len);
> +		if (!ret) {
> +			err = -EIO;

This results in many fstests failures, eg.

[ 1886.766605] run fstests btrfs/008 at 2017-06-05 17:18:52
[ 1897.043952] BTRFS: device fsid 136be123-23f0-4fa7-bcbe-2001c90fb638 devid 1 transid 5 /dev/sdb6
[ 1897.100674] BTRFS info (device sdb6): disk space caching is enabled
[ 1897.100684] BTRFS info (device sdb6): has skinny extents
[ 1897.100689] BTRFS info (device sdb6): flagging fs with big metadata feature
[ 1897.106492] BTRFS info (device sdb6): detected SSD devices, enabling SSD mode
[ 1897.107360] BTRFS info (device sdb6): creating UUID tree
[ 1897.304521] BTRFS critical (device sdb5): invalid dir item name len: 7
[ 1897.304541] BTRFS: error (device sdb5) in btrfs_unlink_subvol:4244: errno=-5 IO failure
[ 1897.304548] BTRFS info (device sdb5): forced readonly
[ 1897.304557] BTRFS: error (device sdb5) in btrfs_ioctl_snap_destroy:2508: errno=-5 IO failure
[ 1897.465795] BTRFS error (device sdb5): cleaner transaction attach returned -30

with current for-next, so I'll remove the branch with namelen validation for now.

> +			goto out;
> +		}
> +
>  		WARN_ON(memcmp_extent_buffer(leaf, name, ptr, name_len));
>  		*sequence = btrfs_root_ref_sequence(leaf, ref);
>  
> -- 
> 2.13.0
> 
> 
> 
> --
> 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

[PATCH v2 9/9] btrfs: Verify dir_item 'in iterate_object_props'

[Su Yue]

Call 'Verify dir_item' before memcmp_extent_buffer.

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 v2 0/9] btrfs: check namelen before read name

[David Sterba]

On Thu, Jun 01, 2017 at 04:57:07PM +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_namelen_valid' to make check namelen with
> item boundary.
> If read name from dir_item, use 'verify_dir_item' to do more strict
> check. Otherwise, use 'btrfs_is_namelen_valid'.
> 
> It's unnessary to do check before every read/memcmp_extent_buffer name.
> Checking namelen 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.

Now it looks much better, thanks. I briefly went through the patchset
and checked the callgraphs and haven't spotted any serious problems
(beyond what's been commented already).

The changelogs could be improved a bit, and mention how/where the extent
buffer is read for the first time. For example using btrfs_search_slot,
or by other means (like iterating leaves).

One thing that I'd still like to discuss: whether to use namelen or
name_len.  As name_len matches the member name, I think it should also
be used in the helper name and subject lines.

> Su Yue (9):
>   btrfs: Introduce btrfs_is_namelen_valid to avoid reading beyond
>     boundary
>   btrfs: Check namelen 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 namelen in 'btrfs_check_ref_name_override'
>   btrfs: Check name before read in 'iterate_dir_item'
>   btrfs: Check namelen before read in 'btrfs_get_name'
>   btrfs: Check namelen before in 'btrfs_del_root_ref'
>   btrfs: Verify dir_item 'in iterate_object_props'

It's not necessary to quote the function in the subject.

Re: [PATCH v2 0/9] btrfs: check namelen before read name

[David Sterba]

On Fri, Jun 02, 2017 at 07:34:19PM +0200, David Sterba wrote:
> On Thu, Jun 01, 2017 at 04:57:07PM +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_namelen_valid' to make check namelen with
> > item boundary.
> > If read name from dir_item, use 'verify_dir_item' to do more strict
> > check. Otherwise, use 'btrfs_is_namelen_valid'.
> > 
> > It's unnessary to do check before every read/memcmp_extent_buffer name.
> > Checking namelen 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.
> 
> Now it looks much better, thanks. I briefly went through the patchset
> and checked the callgraphs and haven't spotted any serious problems
> (beyond what's been commented already).
> 
> The changelogs could be improved a bit, and mention how/where the extent
> buffer is read for the first time. For example using btrfs_search_slot,
> or by other means (like iterating leaves).
> 
> One thing that I'd still like to discuss: whether to use namelen or
> name_len.  As name_len matches the member name, I think it should also
> be used in the helper name and subject lines.
> 
> > Su Yue (9):
> >   btrfs: Introduce btrfs_is_namelen_valid to avoid reading beyond
> >     boundary
> >   btrfs: Check namelen 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 namelen in 'btrfs_check_ref_name_override'
> >   btrfs: Check name before read in 'iterate_dir_item'
> >   btrfs: Check namelen before read in 'btrfs_get_name'
> >   btrfs: Check namelen before in 'btrfs_del_root_ref'
> >   btrfs: Verify dir_item 'in iterate_object_props'
> 
> It's not necessary to quote the function in the subject.

As all requested changes do not affect code, I'll add this patchset to
for-next so we whave more test coverage.