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

[David Sterba <dsterba@suse.cz>]

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.