| Message ID | 20170525020908.25830-1-suy.fnst@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > +} -- 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
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. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --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; +}
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(+)