btrfs: tree-checker: Add checker for variable length item
PATCH 1/2 btrfs: tree-checker: Add checker for variable length item
For the following types, we have items with variable length:
(With BTRFS_ prefix and _KEY suffix snipped)
DIR_ITEMDIR_INDEXXATTR_ITEMINODE_REFINODE_EXTREFROOT_REFROOT_BACKREF
They all use @name_len to indicate their name length, and XATTR_ITEM has extra @data_len to indicate it data length.
Despite their variable length, it's also possible to have several such structure inside one item.
This patch will add checker to ensure:
1) No structure header and its data cross item boundary
2) Except XATTR_ITEM, no structure should have non-zero @data_len
This checker is especially useful to avoid possible access beyond boundary for fuzzed image.
--- fs/btrfs/tree-checker.c | 123 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 123 insertions(+)
fs/btrfs/tree-checker.c +123 -0
@@ -222,6 +222,120 @@ static int check_csum_item(struct btrfs_root *root, struct extent_buffer *leaf,return 0 ; } +static u32 get_header_size (u8 type) +{ +switch (type) { +case BTRFS_DIR_ITEM_KEY : +case BTRFS_DIR_INDEX_KEY : +case BTRFS_XATTR_ITEM_KEY : +return sizeof (struct btrfs_dir_item ); +case BTRFS_INODE_REF_KEY : +return sizeof (struct btrfs_inode_ref ); +case BTRFS_INODE_EXTREF_KEY : +return sizeof (struct btrfs_inode_extref ); +case BTRFS_ROOT_REF_KEY : +case BTRFS_ROOT_BACKREF_KEY : +return sizeof (struct btrfs_root_ref ); + } +WARN_ON (1 ); +return 0 ; +} + +static u16 get_header_namelen (struct extent_buffer *leaf,u8 type, +u32 header_offset) +{ +/* +* @header_offset is offset starts after leaf header, while the +* accessors expects offset starts from leaf header. +* Sowe need to adds LEAF_DATA_OFFSET here +*/ +unsigned long leaf_offset = header_offset +BTRFS_LEAF_DATA_OFFSET ; + +switch (type) { +case BTRFS_DIR_ITEM_KEY : +case BTRFS_DIR_INDEX_KEY : +case BTRFS_XATTR_ITEM_KEY : +return btrfs_dir_name_len (leaf, (void *)leaf_offset); +case BTRFS_INODE_REF_KEY : +return btrfs_inode_ref_name_len (leaf, (void *)leaf_offset); +case BTRFS_INODE_EXTREF_KEY : +return btrfs_inode_extref_name_len (leaf, (void *)leaf_offset); +case BTRFS_ROOT_REF_KEY : +case BTRFS_ROOT_BACKREF_KEY : +return btrfs_root_ref_name_len (leaf, (void *)leaf_offset); + } +WARN_ON (1 ); +return 0 ; +} + +static u16 get_header_datalen (struct extent_buffer *leaf,u8 type, +unsigned long header_offset) +{ +/* Same as get_header_namelen */ +unsigned long leaf_offset = header_offset +BTRFS_LEAF_DATA_OFFSET ; + +switch (type) { +case BTRFS_DIR_ITEM_KEY : +case BTRFS_DIR_INDEX_KEY : +case BTRFS_XATTR_ITEM_KEY : +return btrfs_dir_data_len (leaf, (void *)leaf_offset); + } +return 0 ; +} + +/* +* For items with variable length, normally with namelen and tailing data. +* Like INODE_REF or XATTR +*/ +static int check_variable_length_item (struct btrfs_root *root, +struct extent_buffer *leaf, +struct btrfs_key *key,int slot) +{ +u8 type = key->type; +u32 item_start =btrfs_item_offset_nr (leaf, slot); +u32 item_end =btrfs_item_end_nr (leaf, slot); +u32 header_size =get_header_size (type); +u32 total_size; +u32 cur = item_start; + +while (cur < item_end) { +u32 namelen; +u32 datalen; + +/* header itself should not cross item boundary */ +if (cur + header_size > item_end) { +generic_err (root, leaf, slot, +"structure header crosses item boundary, have %u expect (%u, %u]" , + cur + header_size, cur, item_end); +return -EUCLEAN ; + } + + namelen =get_header_namelen (leaf, type, cur); + datalen =get_header_datalen (leaf, type, cur); + +/* Only XATTR can own data */ +if (type !=BTRFS_XATTR_ITEM_KEY && datalen) { +generic_err (root, leaf, slot, +"item has invalid data len, have %u expect 0" , + datalen); +return -EUCLEAN ; + } + + total_size = header_size + namelen + datalen; + +/* header and name/data should not cross item boundary */ +if (cur + total_size > item_end) { +generic_err (root, leaf, slot, +"structure data crosses item boundary, have %u expect (%u, %u]" , + cur + total_size, cur + header_size, item_end); +return -EUCLEAN ; + } + + cur += total_size; + } +return 0 ; +} +/* * Common point to switch the item-specific validation. */
@@ -238,6 +352,15 @@ static int check_leaf_item(struct btrfs_root *root,case BTRFS_EXTENT_CSUM_KEY : ret =check_csum_item (root, leaf, key, slot);break ; +case BTRFS_DIR_ITEM_KEY : +case BTRFS_XATTR_ITEM_KEY : +case BTRFS_DIR_INDEX_KEY : +case BTRFS_INODE_REF_KEY : +case BTRFS_INODE_EXTREF_KEY : +case BTRFS_ROOT_REF_KEY : +case BTRFS_ROOT_BACKREF_KEY : + ret =check_variable_length_item (root, leaf, key, slot); +break ; }return ret; }
Signature · 1 line
2.14.3
PATCH 2/2 btrfs: Cleanup existing name_len checks
Since tree-checker has verified leaf when reading from disk, we don't need the existing checker.
This cleanup reverts the following commits:fbc326159a01 ("btrfs: Verify dir_item in iterate_object_props") 64c7b01446f4 ("btrfs: Check name_len before in btrfs_del_root_ref") 488d7c456653 ("btrfs: Check name_len before reading btrfs_get_name") 59b0a7f2c7c1 ("btrfs: Check name_len before read in iterate_dir_item") 3c1d41844896 ("btrfs: Check name_len in btrfs_check_ref_name_override") 8ee8c2d62d5f ("btrfs: Verify dir_item in replay_xattr_deletes") 26a836cec2ea ("btrfs: Check name_len on add_inode_ref call path") e79a33270d05 ("btrfs: Check name_len with boundary in verify dir_item") 19c6dcbfa746 ("btrfs: Introduce btrfs_is_name_len_valid to avoid reading beyond boundary")
--- fs/btrfs/ctree.h | 4 +-- fs/btrfs/dir-item.c | 76 ++-------------------------------------------------- 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, 16 insertions(+), 136 deletions(-)
fs/btrfs/ctree.h +1 -3
@@ -3061,14 +3061,12 @@ 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,int slot, +struct extent_buffer *leaf,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,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,
fs/btrfs/dir-item.c +2 -74
106 lines not on this page · raw message
fs/btrfs/export.c +0 -5
@@ -282,11 +282,6 @@ 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);
fs/btrfs/inode.c +1 -1
@@ -5953,7 +5953,7 @@ static int btrfs_real_readdir(struct file *file, struct dir_context *ctx)if (btrfs_should_delete_dir_index (&del_list, found_key.offset)) goto next; di = btrfs_item_ptr(leaf, slot, struct btrfs_dir_item); -if (verify_dir_item (fs_info, leaf, slot, di)) +if (verify_dir_item (fs_info, leaf, di)) goto next; name_len =btrfs_dir_name_len (leaf, di);
fs/btrfs/props.c +0 -7
@@ -164,7 +164,6 @@ 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 ;
@@ -215,12 +214,6 @@ 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,
fs/btrfs/root-tree.c +0 -7
@@ -387,13 +387,6 @@ 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);
fs/btrfs/send.c +0 -6
@@ -1059,12 +1059,6 @@ 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 = -EIO ; - goto out; - }if (name_len + data_len > buf_len) { buf_len = name_len + data_len;if (is_vmalloc_addr (buf)) {
fs/btrfs/tree-log.c +11 -32
109 lines not on this page · raw message
fs/btrfs/xattr.c +1 -1
@@ -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, slot, di)) { +if (verify_dir_item (fs_info, leaf, di)) { ret = -EIO ; goto err; }
Signature · 1 line
2.14.3
Quoted 54 lines · Qu Wenruo
For the following types, we have items with variable length:
(WithBTRFS_prefix and_KEYsuffix snipped)
DIR_ITEMDIR_INDEXXATTR_ITEMINODE_REFINODE_EXTREFROOT_REFROOT_BACKREFThey all use @name_len to indicate their name length, and
XATTR_ITEMhas extra @data_len to indicate it data length.Despite their variable length, it's also possible to have several such structure inside one item.
This patch will add checker to ensure:
1) No structure header and its data cross item boundary
2) ExceptXATTR_ITEM, no structure should have non-zero @data_lenThis checker is especially useful to avoid possible access beyond boundary for fuzzed image.
Quoted 1 line
Signed-off-by Qu Wenruo <[email protected]>--- fs/btrfs/tree-checker.c | 123 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 123 insertions(+)fs/btrfs/tree-checker.c +17
-0@@ -222,6 +222,120 @@ static int check_csum_item(struct btrfs_root *root, struct extent_buffer *leaf,return 0 ; } +static u32 get_header_size (u8 type) +{ +switch (type) { +case BTRFS_DIR_ITEM_KEY : +case BTRFS_DIR_INDEX_KEY : +case BTRFS_XATTR_ITEM_KEY : +return sizeof (struct btrfs_dir_item ); +case BTRFS_INODE_REF_KEY : +return sizeof (struct btrfs_inode_ref ); +case BTRFS_INODE_EXTREF_KEY : +return sizeof (struct btrfs_inode_extref ); +case BTRFS_ROOT_REF_KEY : +case BTRFS_ROOT_BACKREF_KEY : +return sizeof (struct btrfs_root_ref ); + } +WARN_ON (1 ); +return 0 ;
fs/btrfs/tree-checker.c static int check_csum_item(struct btrfs_root *root, struct extent_buffer *leaf, · line 251
Quoted 10 lines
fs/btrfs/tree-checker.c +10
-0+} + +static u16 get_header_namelen (struct extent_buffer *leaf,u8 type, +u32 header_offset) +{ +/* +* @header_offset is offset starts after leaf header, while the +* accessors expects offset starts from leaf header. +* Sowe need to adds LEAF_DATA_OFFSET here +*/
The comment is confusing (but so are the internals of accessing an extent buffer and all the offset magic happening behind the scenes).
You are essentially opencoding btrfs_item_ptr_offset but instead of pointing at the first entry in the data portion, you really point to 'cur' entry and in order to make it "accessor friendly" you do the + BTRFS_LEAF_DATA_OFFSET. I'd say just put something like :
"This is the same as btrfs_item_ptr_offset but starting at item data pointed by header_offset". In fact I'd suggest renaming header_offset to data_offset
Quoted 38 lines
fs/btrfs/tree-checker.c +38
-0+unsigned long leaf_offset = header_offset +BTRFS_LEAF_DATA_OFFSET ; + +switch (type) { +case BTRFS_DIR_ITEM_KEY : +case BTRFS_DIR_INDEX_KEY : +case BTRFS_XATTR_ITEM_KEY : +return btrfs_dir_name_len (leaf, (void *)leaf_offset); +case BTRFS_INODE_REF_KEY : +return btrfs_inode_ref_name_len (leaf, (void *)leaf_offset); +case BTRFS_INODE_EXTREF_KEY : +return btrfs_inode_extref_name_len (leaf, (void *)leaf_offset); +case BTRFS_ROOT_REF_KEY : +case BTRFS_ROOT_BACKREF_KEY : +return btrfs_root_ref_name_len (leaf, (void *)leaf_offset); + } +WARN_ON (1 ); +return 0 ; +} + +static u16 get_header_datalen (struct extent_buffer *leaf,u8 type, +unsigned long header_offset) +{ +/* Same as get_header_namelen */ +unsigned long leaf_offset = header_offset +BTRFS_LEAF_DATA_OFFSET ; + +switch (type) { +case BTRFS_DIR_ITEM_KEY : +case BTRFS_DIR_INDEX_KEY : +case BTRFS_XATTR_ITEM_KEY : +return btrfs_dir_data_len (leaf, (void *)leaf_offset); + } +return 0 ; +} + +/* +* For items with variable length, normally with namelen and tailing data. +* Like INODE_REF or XATTR +*/
fs/btrfs/tree-checker.c static int check_csum_item(struct btrfs_root *root, struct extent_buffer *leaf, · line 299
Quoted 10 lines
fs/btrfs/tree-checker.c +10
-0+static int check_variable_length_item (struct btrfs_root *root, +struct extent_buffer *leaf, +struct btrfs_key *key,int slot) +{ +u8 type = key->type; +u32 item_start =btrfs_item_offset_nr (leaf, slot); +u32 item_end =btrfs_item_end_nr (leaf, slot); +u32 header_size =get_header_size (type); +u32 total_size; +u32 cur = item_start;
nit: Rename cur to cur_offset to make it clear this is an offset.
Quoted 59 lines
fs/btrfs/tree-checker.c +48
-0+ +while (cur < item_end) { +u32 namelen; +u32 datalen; + +/* header itself should not cross item boundary */ +if (cur + header_size > item_end) { +generic_err (root, leaf, slot, +"structure header crosses item boundary, have %u expect (%u, %u]" , + cur + header_size, cur, item_end); +return -EUCLEAN ; + } + + namelen =get_header_namelen (leaf, type, cur); + datalen =get_header_datalen (leaf, type, cur); + +/* Only XATTR can own data */ +if (type !=BTRFS_XATTR_ITEM_KEY && datalen) { +generic_err (root, leaf, slot, +"item has invalid data len, have %u expect 0" , + datalen); +return -EUCLEAN ; + } + + total_size = header_size + namelen + datalen; + +/* header and name/data should not cross item boundary */ +if (cur + total_size > item_end) { +generic_err (root, leaf, slot, +"structure data crosses item boundary, have %u expect (%u, %u]" , + cur + total_size, cur + header_size, item_end); +return -EUCLEAN ; + } + + cur += total_size; + } +return 0 ; +} +/* * Common point to switch the item-specific validation. */ @@ -238,6 +352,15 @@ static int check_leaf_item(struct btrfs_root *root,case BTRFS_EXTENT_CSUM_KEY : ret =check_csum_item (root, leaf, key, slot);break ; +case BTRFS_DIR_ITEM_KEY : +case BTRFS_XATTR_ITEM_KEY : +case BTRFS_DIR_INDEX_KEY : +case BTRFS_INODE_REF_KEY : +case BTRFS_INODE_EXTREF_KEY : +case BTRFS_ROOT_REF_KEY : +case BTRFS_ROOT_BACKREF_KEY : + ret =check_variable_length_item (root, leaf, key, slot); +break ; }return ret; }
Quoted 62 lines · Nikolay Borisov
Quoted 59 lines · Qu Wenruo
For the following types, we have items with variable length:
(WithBTRFS_prefix and_KEYsuffix snipped)
DIR_ITEMDIR_INDEXXATTR_ITEMINODE_REFINODE_EXTREFROOT_REFROOT_BACKREFThey all use @name_len to indicate their name length, and
XATTR_ITEMhas extra @data_len to indicate it data length.Despite their variable length, it's also possible to have several such structure inside one item.
This patch will add checker to ensure:
1) No structure header and its data cross item boundary
2) ExceptXATTR_ITEM, no structure should have non-zero @data_lenThis checker is especially useful to avoid possible access beyond boundary for fuzzed image.
Quoted 1 line
Signed-off-by Qu Wenruo <[email protected]>--- fs/btrfs/tree-checker.c | 123 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 123 insertions(+)fs/btrfs/tree-checker.c +22
-0@@ -222,6 +222,120 @@ static int check_csum_item(struct btrfs_root *root, struct extent_buffer *leaf,return 0 ; } +static u32 get_header_size (u8 type) +{ +switch (type) { +case BTRFS_DIR_ITEM_KEY : +case BTRFS_DIR_INDEX_KEY : +case BTRFS_XATTR_ITEM_KEY : +return sizeof (struct btrfs_dir_item ); +case BTRFS_INODE_REF_KEY : +return sizeof (struct btrfs_inode_ref ); +case BTRFS_INODE_EXTREF_KEY : +return sizeof (struct btrfs_inode_extref ); +case BTRFS_ROOT_REF_KEY : +case BTRFS_ROOT_BACKREF_KEY : +return sizeof (struct btrfs_root_ref ); + } +WARN_ON (1 ); +return 0 ; +} + +static u16 get_header_namelen (struct extent_buffer *leaf,u8 type, +u32 header_offset) +{
Quoted 8 lines
fs/btrfs/tree-checker.c static int check_csum_item(struct btrfs_root *root, struct extent_buffer *leaf, · line 251
Quoted 5 lines
fs/btrfs/tree-checker.c +5
-0+/* +* @header_offset is offset starts after leaf header, while the +* accessors expects offset starts from leaf header. +* Sowe need to adds LEAF_DATA_OFFSET here +*/ The comment is confusing (but so are the internals of accessing an extent buffer and all the offset magic happening behind the scenes).
The sentence inside the brackets is the point. :)
Quoted 9 lines
You are essentially opencoding
btrfs_item_ptr_offsetbut instead of pointing at the first entry in the data portion, you really point to 'cur' entry and in order to make it "accessor friendly" you do the +BTRFS_LEAF_DATA_OFFSET. I'd say just put something like :"This is the same as
btrfs_item_ptr_offsetbut starting at item data pointed byheader_offset". In fact I'd suggest renamingheader_offsettodata_offset
Only if we can ensure all offsets passed in are starting after header, then I'm OK to rename it to "data_offset"
The "header_offset" here is to info any caller that one should pass "normal" offset here, to avoid possible LEAF_DATA_OFFSET hassle.
Thanks,
Qu
Quoted 116 lines
Quoted 39 lines
fs/btrfs/tree-checker.c +38
-0+unsigned long leaf_offset = header_offset +BTRFS_LEAF_DATA_OFFSET ; + +switch (type) { +case BTRFS_DIR_ITEM_KEY : +case BTRFS_DIR_INDEX_KEY : +case BTRFS_XATTR_ITEM_KEY : +return btrfs_dir_name_len (leaf, (void *)leaf_offset); +case BTRFS_INODE_REF_KEY : +return btrfs_inode_ref_name_len (leaf, (void *)leaf_offset); +case BTRFS_INODE_EXTREF_KEY : +return btrfs_inode_extref_name_len (leaf, (void *)leaf_offset); +case BTRFS_ROOT_REF_KEY : +case BTRFS_ROOT_BACKREF_KEY : +return btrfs_root_ref_name_len (leaf, (void *)leaf_offset); + } +WARN_ON (1 ); +return 0 ; +} + +static u16 get_header_datalen (struct extent_buffer *leaf,u8 type, +unsigned long header_offset) +{ +/* Same as get_header_namelen */ +unsigned long leaf_offset = header_offset +BTRFS_LEAF_DATA_OFFSET ; + +switch (type) { +case BTRFS_DIR_ITEM_KEY : +case BTRFS_DIR_INDEX_KEY : +case BTRFS_XATTR_ITEM_KEY : +return btrfs_dir_data_len (leaf, (void *)leaf_offset); + } +return 0 ; +} + +/* +* For items with variable length, normally with namelen and tailing data. +* Like INODE_REF or XATTR +*/ fs/btrfs/tree-checker.c static int check_csum_item(struct btrfs_root *root, struct extent_buffer *leaf, · line 299
Quoted 10 lines
fs/btrfs/tree-checker.c +10
-0+static int check_variable_length_item (struct btrfs_root *root, +struct extent_buffer *leaf, +struct btrfs_key *key,int slot) +{ +u8 type = key->type; +u32 item_start =btrfs_item_offset_nr (leaf, slot); +u32 item_end =btrfs_item_end_nr (leaf, slot); +u32 header_size =get_header_size (type); +u32 total_size; +u32 cur = item_start;nit: Rename cur to
cur_offsetto make it clear this is an offset.Quoted 49 lines
fs/btrfs/tree-checker.c +41
-0+ +while (cur < item_end) { +u32 namelen; +u32 datalen; + +/* header itself should not cross item boundary */ +if (cur + header_size > item_end) { +generic_err (root, leaf, slot, +"structure header crosses item boundary, have %u expect (%u, %u]" , + cur + header_size, cur, item_end); +return -EUCLEAN ; + } + + namelen =get_header_namelen (leaf, type, cur); + datalen =get_header_datalen (leaf, type, cur); + +/* Only XATTR can own data */ +if (type !=BTRFS_XATTR_ITEM_KEY && datalen) { +generic_err (root, leaf, slot, +"item has invalid data len, have %u expect 0" , + datalen); +return -EUCLEAN ; + } + + total_size = header_size + namelen + datalen; + +/* header and name/data should not cross item boundary */ +if (cur + total_size > item_end) { +generic_err (root, leaf, slot, +"structure data crosses item boundary, have %u expect (%u, %u]" , + cur + total_size, cur + header_size, item_end); +return -EUCLEAN ; + } + + cur += total_size; + } +return 0 ; +} +/* * Common point to switch the item-specific validation. */ @@ -238,6 +352,15 @@ static int check_leaf_item(struct btrfs_root *root,case BTRFS_EXTENT_CSUM_KEY : ret =check_csum_item (root, leaf, key, slot);break ; +case BTRFS_DIR_ITEM_KEY : +case BTRFS_XATTR_ITEM_KEY :fs/btrfs/tree-checker.c static int check_leaf_item(struct btrfs_root *root, · line 366
Quoted 10 lines
fs/btrfs/tree-checker.c +7
-0+case BTRFS_DIR_INDEX_KEY : +case BTRFS_INODE_REF_KEY : +case BTRFS_INODE_EXTREF_KEY : +case BTRFS_ROOT_REF_KEY : +case BTRFS_ROOT_BACKREF_KEY : + ret =check_variable_length_item (root, leaf, key, slot); +break ; }return ret; }--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 82 lines · Qu Wenruo
Quoted 62 lines · Nikolay Borisov
Quoted 59 lines · Qu Wenruo
For the following types, we have items with variable length:
(WithBTRFS_prefix and_KEYsuffix snipped)
DIR_ITEMDIR_INDEXXATTR_ITEMINODE_REFINODE_EXTREFROOT_REFROOT_BACKREFThey all use @name_len to indicate their name length, and
XATTR_ITEMhas extra @data_len to indicate it data length.Despite their variable length, it's also possible to have several such structure inside one item.
This patch will add checker to ensure:
1) No structure header and its data cross item boundary
2) ExceptXATTR_ITEM, no structure should have non-zero @data_lenThis checker is especially useful to avoid possible access beyond boundary for fuzzed image.
Quoted 1 line
Signed-off-by Qu Wenruo <[email protected]>--- fs/btrfs/tree-checker.c | 123 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 123 insertions(+)fs/btrfs/tree-checker.c +22
-0@@ -222,6 +222,120 @@ static int check_csum_item(struct btrfs_root *root, struct extent_buffer *leaf,return 0 ; } +static u32 get_header_size (u8 type) +{ +switch (type) { +case BTRFS_DIR_ITEM_KEY : +case BTRFS_DIR_INDEX_KEY : +case BTRFS_XATTR_ITEM_KEY : +return sizeof (struct btrfs_dir_item ); +case BTRFS_INODE_REF_KEY : +return sizeof (struct btrfs_inode_ref ); +case BTRFS_INODE_EXTREF_KEY : +return sizeof (struct btrfs_inode_extref ); +case BTRFS_ROOT_REF_KEY : +case BTRFS_ROOT_BACKREF_KEY : +return sizeof (struct btrfs_root_ref ); + } +WARN_ON (1 ); +return 0 ; +} + +static u16 get_header_namelen (struct extent_buffer *leaf,u8 type, +u32 header_offset) +{Quoted 8 lines
fs/btrfs/tree-checker.c static int check_csum_item(struct btrfs_root *root, struct extent_buffer *leaf, · line 251
Quoted 5 lines
fs/btrfs/tree-checker.c +5
-0+/* +* @header_offset is offset starts after leaf header, while the +* accessors expects offset starts from leaf header. +* Sowe need to adds LEAF_DATA_OFFSET here +*/ The comment is confusing (but so are the internals of accessing an extent buffer and all the offset magic happening behind the scenes).
The sentence inside the brackets is the point. :)
Quoted 6 lines
You are essentially opencoding
btrfs_item_ptr_offsetbut instead of pointing at the first entry in the data portion, you really point to 'cur' entry and in order to make it "accessor friendly" you do the +BTRFS_LEAF_DATA_OFFSET. I'd say just put something like :
Quoted 6 lines
Quoted 3 lines
"This is the same as
btrfs_item_ptr_offsetbut starting at item data pointed byheader_offset". In fact I'd suggest renamingheader_offsettodata_offsetOnly if we can ensure all offsets passed in are starting after header, then I'm OK to rename it to "
data_offset"
That's one of the things which confused me. Which header:
1. There is the btrfs_leaf which is at the beginning of the of the leaf item and in order to access a pointer inside the leaf extent_buffer via the helpers you really want the offset which skips this header and points to the data item hence your + BTRFS_LEAF_DATA_OFFSET code
2. (The one which I assume you meant) There is the actual struct in the data portion of the item, which acts as a header for the variable length data (either name or name + data).
Since this function is only ever called from check_variable_length_item and it guarantees that the passed value always points inside the data portion I believe this is fine, no ?
Quoted 124 lines
The "
header_offset" here is to info any caller that one should pass "normal" offset here, to avoid possibleLEAF_DATA_OFFSEThassle.Thanks,
QuQuoted 116 lines
Quoted 39 lines
fs/btrfs/tree-checker.c +38
-0+unsigned long leaf_offset = header_offset +BTRFS_LEAF_DATA_OFFSET ; + +switch (type) { +case BTRFS_DIR_ITEM_KEY : +case BTRFS_DIR_INDEX_KEY : +case BTRFS_XATTR_ITEM_KEY : +return btrfs_dir_name_len (leaf, (void *)leaf_offset); +case BTRFS_INODE_REF_KEY : +return btrfs_inode_ref_name_len (leaf, (void *)leaf_offset); +case BTRFS_INODE_EXTREF_KEY : +return btrfs_inode_extref_name_len (leaf, (void *)leaf_offset); +case BTRFS_ROOT_REF_KEY : +case BTRFS_ROOT_BACKREF_KEY : +return btrfs_root_ref_name_len (leaf, (void *)leaf_offset); + } +WARN_ON (1 ); +return 0 ; +} + +static u16 get_header_datalen (struct extent_buffer *leaf,u8 type, +unsigned long header_offset) +{ +/* Same as get_header_namelen */ +unsigned long leaf_offset = header_offset +BTRFS_LEAF_DATA_OFFSET ; + +switch (type) { +case BTRFS_DIR_ITEM_KEY : +case BTRFS_DIR_INDEX_KEY : +case BTRFS_XATTR_ITEM_KEY : +return btrfs_dir_data_len (leaf, (void *)leaf_offset); + } +return 0 ; +} + +/* +* For items with variable length, normally with namelen and tailing data. +* Like INODE_REF or XATTR +*/ fs/btrfs/tree-checker.c static int check_csum_item(struct btrfs_root *root, struct extent_buffer *leaf, · line 299
Quoted 10 lines
fs/btrfs/tree-checker.c +10
-0+static int check_variable_length_item (struct btrfs_root *root, +struct extent_buffer *leaf, +struct btrfs_key *key,int slot) +{ +u8 type = key->type; +u32 item_start =btrfs_item_offset_nr (leaf, slot); +u32 item_end =btrfs_item_end_nr (leaf, slot); +u32 header_size =get_header_size (type); +u32 total_size; +u32 cur = item_start;nit: Rename cur to
cur_offsetto make it clear this is an offset.Quoted 49 lines
fs/btrfs/tree-checker.c +41
-0+ +while (cur < item_end) { +u32 namelen; +u32 datalen; + +/* header itself should not cross item boundary */ +if (cur + header_size > item_end) { +generic_err (root, leaf, slot, +"structure header crosses item boundary, have %u expect (%u, %u]" , + cur + header_size, cur, item_end); +return -EUCLEAN ; + } + + namelen =get_header_namelen (leaf, type, cur); + datalen =get_header_datalen (leaf, type, cur); + +/* Only XATTR can own data */ +if (type !=BTRFS_XATTR_ITEM_KEY && datalen) { +generic_err (root, leaf, slot, +"item has invalid data len, have %u expect 0" , + datalen); +return -EUCLEAN ; + } + + total_size = header_size + namelen + datalen; + +/* header and name/data should not cross item boundary */ +if (cur + total_size > item_end) { +generic_err (root, leaf, slot, +"structure data crosses item boundary, have %u expect (%u, %u]" , + cur + total_size, cur + header_size, item_end); +return -EUCLEAN ; + } + + cur += total_size; + } +return 0 ; +} +/* * Common point to switch the item-specific validation. */ @@ -238,6 +352,15 @@ static int check_leaf_item(struct btrfs_root *root,case BTRFS_EXTENT_CSUM_KEY : ret =check_csum_item (root, leaf, key, slot);break ; +case BTRFS_DIR_ITEM_KEY : +case BTRFS_XATTR_ITEM_KEY :fs/btrfs/tree-checker.c static int check_leaf_item(struct btrfs_root *root, · line 366
Quoted 10 lines
fs/btrfs/tree-checker.c +7
-0+case BTRFS_DIR_INDEX_KEY : +case BTRFS_INODE_REF_KEY : +case BTRFS_INODE_EXTREF_KEY : +case BTRFS_ROOT_REF_KEY : +case BTRFS_ROOT_BACKREF_KEY : + ret =check_variable_length_item (root, leaf, key, slot); +break ; }return ret; }--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 97 lines · Qu Wenruo
For the following types, we have items with variable length:
(WithBTRFS_prefix and_KEYsuffix snipped)
DIR_ITEMDIR_INDEXXATTR_ITEMINODE_REFINODE_EXTREFROOT_REFROOT_BACKREFThey all use @name_len to indicate their name length, and
XATTR_ITEMhas extra @data_len to indicate it data length.Despite their variable length, it's also possible to have several such structure inside one item.
This patch will add checker to ensure:
1) No structure header and its data cross item boundary
2) ExceptXATTR_ITEM, no structure should have non-zero @data_lenThis checker is especially useful to avoid possible access beyond boundary for fuzzed image.
Signed-off-by Qu Wenruo <[email protected]>--- fs/btrfs/tree-checker.c | 123 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 123 insertions(+)fs/btrfs/tree-checker.c +59
-0@@ -222,6 +222,120 @@ static int check_csum_item(struct btrfs_root *root, struct extent_buffer *leaf,return 0 ; } +static u32 get_header_size (u8 type) +{ +switch (type) { +case BTRFS_DIR_ITEM_KEY : +case BTRFS_DIR_INDEX_KEY : +case BTRFS_XATTR_ITEM_KEY : +return sizeof (struct btrfs_dir_item ); +case BTRFS_INODE_REF_KEY : +return sizeof (struct btrfs_inode_ref ); +case BTRFS_INODE_EXTREF_KEY : +return sizeof (struct btrfs_inode_extref ); +case BTRFS_ROOT_REF_KEY : +case BTRFS_ROOT_BACKREF_KEY : +return sizeof (struct btrfs_root_ref ); + } +WARN_ON (1 ); +return 0 ; +} + +static u16 get_header_namelen (struct extent_buffer *leaf,u8 type, +u32 header_offset) +{ +/* +* @header_offset is offset starts after leaf header, while the +* accessors expects offset starts from leaf header. +* Sowe need to adds LEAF_DATA_OFFSET here +*/ +unsigned long leaf_offset = header_offset +BTRFS_LEAF_DATA_OFFSET ; + +switch (type) { +case BTRFS_DIR_ITEM_KEY : +case BTRFS_DIR_INDEX_KEY : +case BTRFS_XATTR_ITEM_KEY : +return btrfs_dir_name_len (leaf, (void *)leaf_offset); +case BTRFS_INODE_REF_KEY : +return btrfs_inode_ref_name_len (leaf, (void *)leaf_offset); +case BTRFS_INODE_EXTREF_KEY : +return btrfs_inode_extref_name_len (leaf, (void *)leaf_offset); +case BTRFS_ROOT_REF_KEY : +case BTRFS_ROOT_BACKREF_KEY : +return btrfs_root_ref_name_len (leaf, (void *)leaf_offset); + } +WARN_ON (1 ); +return 0 ; +} + +static u16 get_header_datalen (struct extent_buffer *leaf,u8 type, +unsigned long header_offset) +{ +/* Same as get_header_namelen */ +unsigned long leaf_offset = header_offset +BTRFS_LEAF_DATA_OFFSET ; + +switch (type) { +case BTRFS_DIR_ITEM_KEY : +case BTRFS_DIR_INDEX_KEY : +case BTRFS_XATTR_ITEM_KEY : +return btrfs_dir_data_len (leaf, (void *)leaf_offset); + } +return 0 ;
fs/btrfs/tree-checker.c static int check_csum_item(struct btrfs_root *root, struct extent_buffer *leaf, · line 293
Quoted 10 lines
fs/btrfs/tree-checker.c +10
-0+} + +/* +* For items with variable length, normally with namelen and tailing data. +* Like INODE_REF or XATTR +*/ +static int check_variable_length_item (struct btrfs_root *root, +struct extent_buffer *leaf, +struct btrfs_key *key,int slot) +{
One more thing - you are only validating the boundaries of such variable length items, so make this specific. I.e rename the function to something like:
validate_variable_boundariescheck_variable_length_item_boundarycheck_item_boundaries
Quoted 65 lines
fs/btrfs/tree-checker.c +54
-0+u8 type = key->type; +u32 item_start =btrfs_item_offset_nr (leaf, slot); +u32 item_end =btrfs_item_end_nr (leaf, slot); +u32 header_size =get_header_size (type); +u32 total_size; +u32 cur = item_start; + +while (cur < item_end) { +u32 namelen; +u32 datalen; + +/* header itself should not cross item boundary */ +if (cur + header_size > item_end) { +generic_err (root, leaf, slot, +"structure header crosses item boundary, have %u expect (%u, %u]" , + cur + header_size, cur, item_end); +return -EUCLEAN ; + } + + namelen =get_header_namelen (leaf, type, cur); + datalen =get_header_datalen (leaf, type, cur); + +/* Only XATTR can own data */ +if (type !=BTRFS_XATTR_ITEM_KEY && datalen) { +generic_err (root, leaf, slot, +"item has invalid data len, have %u expect 0" , + datalen); +return -EUCLEAN ; + } + + total_size = header_size + namelen + datalen; + +/* header and name/data should not cross item boundary */ +if (cur + total_size > item_end) { +generic_err (root, leaf, slot, +"structure data crosses item boundary, have %u expect (%u, %u]" , + cur + total_size, cur + header_size, item_end); +return -EUCLEAN ; + } + + cur += total_size; + } +return 0 ; +} +/* * Common point to switch the item-specific validation. */ @@ -238,6 +352,15 @@ static int check_leaf_item(struct btrfs_root *root,case BTRFS_EXTENT_CSUM_KEY : ret =check_csum_item (root, leaf, key, slot);break ; +case BTRFS_DIR_ITEM_KEY : +case BTRFS_XATTR_ITEM_KEY : +case BTRFS_DIR_INDEX_KEY : +case BTRFS_INODE_REF_KEY : +case BTRFS_INODE_EXTREF_KEY : +case BTRFS_ROOT_REF_KEY : +case BTRFS_ROOT_BACKREF_KEY : + ret =check_variable_length_item (root, leaf, key, slot); +break ; }return ret; }
Quoted 106 lines · Nikolay Borisov
Quoted 103 lines · Qu Wenruo
For the following types, we have items with variable length:
(WithBTRFS_prefix and_KEYsuffix snipped)
DIR_ITEMDIR_INDEXXATTR_ITEMINODE_REFINODE_EXTREFROOT_REFROOT_BACKREFThey all use @name_len to indicate their name length, and
XATTR_ITEMhas extra @data_len to indicate it data length.Despite their variable length, it's also possible to have several such structure inside one item.
This patch will add checker to ensure:
1) No structure header and its data cross item boundary
2) ExceptXATTR_ITEM, no structure should have non-zero @data_lenThis checker is especially useful to avoid possible access beyond boundary for fuzzed image.
Signed-off-by Qu Wenruo <[email protected]>--- fs/btrfs/tree-checker.c | 123 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 123 insertions(+)fs/btrfs/tree-checker.c +65
-0@@ -222,6 +222,120 @@ static int check_csum_item(struct btrfs_root *root, struct extent_buffer *leaf,return 0 ; } +static u32 get_header_size (u8 type) +{ +switch (type) { +case BTRFS_DIR_ITEM_KEY : +case BTRFS_DIR_INDEX_KEY : +case BTRFS_XATTR_ITEM_KEY : +return sizeof (struct btrfs_dir_item ); +case BTRFS_INODE_REF_KEY : +return sizeof (struct btrfs_inode_ref ); +case BTRFS_INODE_EXTREF_KEY : +return sizeof (struct btrfs_inode_extref ); +case BTRFS_ROOT_REF_KEY : +case BTRFS_ROOT_BACKREF_KEY : +return sizeof (struct btrfs_root_ref ); + } +WARN_ON (1 ); +return 0 ; +} + +static u16 get_header_namelen (struct extent_buffer *leaf,u8 type, +u32 header_offset) +{ +/* +* @header_offset is offset starts after leaf header, while the +* accessors expects offset starts from leaf header. +* Sowe need to adds LEAF_DATA_OFFSET here +*/ +unsigned long leaf_offset = header_offset +BTRFS_LEAF_DATA_OFFSET ; + +switch (type) { +case BTRFS_DIR_ITEM_KEY : +case BTRFS_DIR_INDEX_KEY : +case BTRFS_XATTR_ITEM_KEY : +return btrfs_dir_name_len (leaf, (void *)leaf_offset); +case BTRFS_INODE_REF_KEY : +return btrfs_inode_ref_name_len (leaf, (void *)leaf_offset); +case BTRFS_INODE_EXTREF_KEY : +return btrfs_inode_extref_name_len (leaf, (void *)leaf_offset); +case BTRFS_ROOT_REF_KEY : +case BTRFS_ROOT_BACKREF_KEY : +return btrfs_root_ref_name_len (leaf, (void *)leaf_offset); + } +WARN_ON (1 ); +return 0 ; +} + +static u16 get_header_datalen (struct extent_buffer *leaf,u8 type, +unsigned long header_offset) +{ +/* Same as get_header_namelen */ +unsigned long leaf_offset = header_offset +BTRFS_LEAF_DATA_OFFSET ; + +switch (type) { +case BTRFS_DIR_ITEM_KEY : +case BTRFS_DIR_INDEX_KEY : +case BTRFS_XATTR_ITEM_KEY : +return btrfs_dir_data_len (leaf, (void *)leaf_offset); + } +return 0 ; +} + +/* +* For items with variable length, normally with namelen and tailing data. +* Like INODE_REF or XATTR +*/
Quoted 8 lines
fs/btrfs/tree-checker.c static int check_csum_item(struct btrfs_root *root, struct extent_buffer *leaf, · line 293
Quoted 4 lines
fs/btrfs/tree-checker.c +4
-0+static int check_variable_length_item (struct btrfs_root *root, +struct extent_buffer *leaf, +struct btrfs_key *key,int slot) +{One more thing - you are only validating the boundaries of such variable length items, so make this specific. I.e rename the function to something like:
If you follow the naming schema in tree-checker, you'll find that we (at least myself) is naming these function always using "check_" prefix, so "validate_" prefix seems less consistent.
Further more, the naming has its level"
leaf (all items boundary already checked)
|- leaf_item (hub wrapper)
|- csum_item
|- extent_data_item
So here check_<something>_item follows the original level according to the naming schema.
Further more, although we're checking boundary, the truth is, we are check the name_len/data_len *members*, so the check_<something>_item still makes sence.
If really want to change the name, I prefer some naming can show that we're checking several items which share the same variable length property.
So at least, none of the alternative seems to fit the schema.
Thanks,
Qu
Quoted 75 lines
validate_variable_boundariescheck_variable_length_item_boundarycheck_item_boundariesQuoted 55 lines
fs/btrfs/tree-checker.c +47
-0+u8 type = key->type; +u32 item_start =btrfs_item_offset_nr (leaf, slot); +u32 item_end =btrfs_item_end_nr (leaf, slot); +u32 header_size =get_header_size (type); +u32 total_size; +u32 cur = item_start; + +while (cur < item_end) { +u32 namelen; +u32 datalen; + +/* header itself should not cross item boundary */ +if (cur + header_size > item_end) { +generic_err (root, leaf, slot, +"structure header crosses item boundary, have %u expect (%u, %u]" , + cur + header_size, cur, item_end); +return -EUCLEAN ; + } + + namelen =get_header_namelen (leaf, type, cur); + datalen =get_header_datalen (leaf, type, cur); + +/* Only XATTR can own data */ +if (type !=BTRFS_XATTR_ITEM_KEY && datalen) { +generic_err (root, leaf, slot, +"item has invalid data len, have %u expect 0" , + datalen); +return -EUCLEAN ; + } + + total_size = header_size + namelen + datalen; + +/* header and name/data should not cross item boundary */ +if (cur + total_size > item_end) { +generic_err (root, leaf, slot, +"structure data crosses item boundary, have %u expect (%u, %u]" , + cur + total_size, cur + header_size, item_end); +return -EUCLEAN ; + } + + cur += total_size; + } +return 0 ; +} +/* * Common point to switch the item-specific validation. */ @@ -238,6 +352,15 @@ static int check_leaf_item(struct btrfs_root *root,case BTRFS_EXTENT_CSUM_KEY : ret =check_csum_item (root, leaf, key, slot);break ; +case BTRFS_DIR_ITEM_KEY : +case BTRFS_XATTR_ITEM_KEY :fs/btrfs/tree-checker.c static int check_leaf_item(struct btrfs_root *root, · line 366
Quoted 10 lines
fs/btrfs/tree-checker.c +7
-0+case BTRFS_DIR_INDEX_KEY : +case BTRFS_INODE_REF_KEY : +case BTRFS_INODE_EXTREF_KEY : +case BTRFS_ROOT_REF_KEY : +case BTRFS_ROOT_BACKREF_KEY : + ret =check_variable_length_item (root, leaf, key, slot); +break ; }return ret; }--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 131 lines · Qu Wenruo
Quoted 106 lines · Nikolay Borisov
Quoted 103 lines · Qu Wenruo
For the following types, we have items with variable length:
(WithBTRFS_prefix and_KEYsuffix snipped)
DIR_ITEMDIR_INDEXXATTR_ITEMINODE_REFINODE_EXTREFROOT_REFROOT_BACKREFThey all use @name_len to indicate their name length, and
XATTR_ITEMhas extra @data_len to indicate it data length.Despite their variable length, it's also possible to have several such structure inside one item.
This patch will add checker to ensure:
1) No structure header and its data cross item boundary
2) ExceptXATTR_ITEM, no structure should have non-zero @data_lenThis checker is especially useful to avoid possible access beyond boundary for fuzzed image.
Signed-off-by Qu Wenruo <[email protected]>--- fs/btrfs/tree-checker.c | 123 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 123 insertions(+)fs/btrfs/tree-checker.c +65
-0@@ -222,6 +222,120 @@ static int check_csum_item(struct btrfs_root *root, struct extent_buffer *leaf,return 0 ; } +static u32 get_header_size (u8 type) +{ +switch (type) { +case BTRFS_DIR_ITEM_KEY : +case BTRFS_DIR_INDEX_KEY : +case BTRFS_XATTR_ITEM_KEY : +return sizeof (struct btrfs_dir_item ); +case BTRFS_INODE_REF_KEY : +return sizeof (struct btrfs_inode_ref ); +case BTRFS_INODE_EXTREF_KEY : +return sizeof (struct btrfs_inode_extref ); +case BTRFS_ROOT_REF_KEY : +case BTRFS_ROOT_BACKREF_KEY : +return sizeof (struct btrfs_root_ref ); + } +WARN_ON (1 ); +return 0 ; +} + +static u16 get_header_namelen (struct extent_buffer *leaf,u8 type, +u32 header_offset) +{ +/* +* @header_offset is offset starts after leaf header, while the +* accessors expects offset starts from leaf header. +* Sowe need to adds LEAF_DATA_OFFSET here +*/ +unsigned long leaf_offset = header_offset +BTRFS_LEAF_DATA_OFFSET ; + +switch (type) { +case BTRFS_DIR_ITEM_KEY : +case BTRFS_DIR_INDEX_KEY : +case BTRFS_XATTR_ITEM_KEY : +return btrfs_dir_name_len (leaf, (void *)leaf_offset); +case BTRFS_INODE_REF_KEY : +return btrfs_inode_ref_name_len (leaf, (void *)leaf_offset); +case BTRFS_INODE_EXTREF_KEY : +return btrfs_inode_extref_name_len (leaf, (void *)leaf_offset); +case BTRFS_ROOT_REF_KEY : +case BTRFS_ROOT_BACKREF_KEY : +return btrfs_root_ref_name_len (leaf, (void *)leaf_offset); + } +WARN_ON (1 ); +return 0 ; +} + +static u16 get_header_datalen (struct extent_buffer *leaf,u8 type, +unsigned long header_offset) +{ +/* Same as get_header_namelen */ +unsigned long leaf_offset = header_offset +BTRFS_LEAF_DATA_OFFSET ; + +switch (type) { +case BTRFS_DIR_ITEM_KEY : +case BTRFS_DIR_INDEX_KEY : +case BTRFS_XATTR_ITEM_KEY : +return btrfs_dir_data_len (leaf, (void *)leaf_offset); + } +return 0 ; +} + +/* +* For items with variable length, normally with namelen and tailing data. +* Like INODE_REF or XATTR +*/ Quoted 8 lines
fs/btrfs/tree-checker.c static int check_csum_item(struct btrfs_root *root, struct extent_buffer *leaf, · line 293
Quoted 4 lines
fs/btrfs/tree-checker.c +4
-0+static int check_variable_length_item (struct btrfs_root *root, +struct extent_buffer *leaf, +struct btrfs_key *key,int slot) +{One more thing - you are only validating the boundaries of such variable length items, so make this specific. I.e rename the function to something like:
If you follow the naming schema in tree-checker, you'll find that we (at least myself) is naming these function always using "
check_" prefix, so "validate_" prefix seems less consistent.Further more, the naming has its level"
leaf (all items boundary already checked)
|-leaf_item(hub wrapper)
|-csum_item
|-extent_data_itemSo here check_<something>_item follows the original level according to the naming schema.
Quoted 8 lines
Further more, although we're checking boundary, the truth is, we are check the name_len/data_len *members*, so the check_<something>_item still makes sence.
If really want to change the name, I prefer some naming can show that we're checking several items which share the same variable length property.
So at least, none of the alternative seems to fit the schema.
The reason why I wanted this function renamed is that we have this function which performs only some checks and then we have the verify_dir_item which performs different checks for the same item. So why can't those function be turned into one ? I'm not too hung up on the actual naming!
Quoted 80 lines
Thanks,
QuQuoted 75 lines
validate_variable_boundariescheck_variable_length_item_boundarycheck_item_boundariesQuoted 55 lines
fs/btrfs/tree-checker.c +47
-0+u8 type = key->type; +u32 item_start =btrfs_item_offset_nr (leaf, slot); +u32 item_end =btrfs_item_end_nr (leaf, slot); +u32 header_size =get_header_size (type); +u32 total_size; +u32 cur = item_start; + +while (cur < item_end) { +u32 namelen; +u32 datalen; + +/* header itself should not cross item boundary */ +if (cur + header_size > item_end) { +generic_err (root, leaf, slot, +"structure header crosses item boundary, have %u expect (%u, %u]" , + cur + header_size, cur, item_end); +return -EUCLEAN ; + } + + namelen =get_header_namelen (leaf, type, cur); + datalen =get_header_datalen (leaf, type, cur); + +/* Only XATTR can own data */ +if (type !=BTRFS_XATTR_ITEM_KEY && datalen) { +generic_err (root, leaf, slot, +"item has invalid data len, have %u expect 0" , + datalen); +return -EUCLEAN ; + } + + total_size = header_size + namelen + datalen; + +/* header and name/data should not cross item boundary */ +if (cur + total_size > item_end) { +generic_err (root, leaf, slot, +"structure data crosses item boundary, have %u expect (%u, %u]" , + cur + total_size, cur + header_size, item_end); +return -EUCLEAN ; + } + + cur += total_size; + } +return 0 ; +} +/* * Common point to switch the item-specific validation. */ @@ -238,6 +352,15 @@ static int check_leaf_item(struct btrfs_root *root,case BTRFS_EXTENT_CSUM_KEY : ret =check_csum_item (root, leaf, key, slot);break ; +case BTRFS_DIR_ITEM_KEY : +case BTRFS_XATTR_ITEM_KEY :fs/btrfs/tree-checker.c static int check_leaf_item(struct btrfs_root *root, · line 366
Quoted 10 lines
fs/btrfs/tree-checker.c +7
-0+case BTRFS_DIR_INDEX_KEY : +case BTRFS_INODE_REF_KEY : +case BTRFS_INODE_EXTREF_KEY : +case BTRFS_ROOT_REF_KEY : +case BTRFS_ROOT_BACKREF_KEY : + ret =check_variable_length_item (root, leaf, key, slot); +break ; }return ret; }--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 141 lines · Nikolay Borisov
Quoted 138 lines · Qu Wenruo
Quoted 106 lines · Nikolay Borisov
Quoted 103 lines · Qu Wenruo
For the following types, we have items with variable length:
(WithBTRFS_prefix and_KEYsuffix snipped)
DIR_ITEMDIR_INDEXXATTR_ITEMINODE_REFINODE_EXTREFROOT_REFROOT_BACKREFThey all use @name_len to indicate their name length, and
XATTR_ITEMhas extra @data_len to indicate it data length.Despite their variable length, it's also possible to have several such structure inside one item.
This patch will add checker to ensure:
1) No structure header and its data cross item boundary
2) ExceptXATTR_ITEM, no structure should have non-zero @data_lenThis checker is especially useful to avoid possible access beyond boundary for fuzzed image.
Signed-off-by Qu Wenruo <[email protected]>--- fs/btrfs/tree-checker.c | 123 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 123 insertions(+)fs/btrfs/tree-checker.c +65
-0@@ -222,6 +222,120 @@ static int check_csum_item(struct btrfs_root *root, struct extent_buffer *leaf,return 0 ; } +static u32 get_header_size (u8 type) +{ +switch (type) { +case BTRFS_DIR_ITEM_KEY : +case BTRFS_DIR_INDEX_KEY : +case BTRFS_XATTR_ITEM_KEY : +return sizeof (struct btrfs_dir_item ); +case BTRFS_INODE_REF_KEY : +return sizeof (struct btrfs_inode_ref ); +case BTRFS_INODE_EXTREF_KEY : +return sizeof (struct btrfs_inode_extref ); +case BTRFS_ROOT_REF_KEY : +case BTRFS_ROOT_BACKREF_KEY : +return sizeof (struct btrfs_root_ref ); + } +WARN_ON (1 ); +return 0 ; +} + +static u16 get_header_namelen (struct extent_buffer *leaf,u8 type, +u32 header_offset) +{ +/* +* @header_offset is offset starts after leaf header, while the +* accessors expects offset starts from leaf header. +* Sowe need to adds LEAF_DATA_OFFSET here +*/ +unsigned long leaf_offset = header_offset +BTRFS_LEAF_DATA_OFFSET ; + +switch (type) { +case BTRFS_DIR_ITEM_KEY : +case BTRFS_DIR_INDEX_KEY : +case BTRFS_XATTR_ITEM_KEY : +return btrfs_dir_name_len (leaf, (void *)leaf_offset); +case BTRFS_INODE_REF_KEY : +return btrfs_inode_ref_name_len (leaf, (void *)leaf_offset); +case BTRFS_INODE_EXTREF_KEY : +return btrfs_inode_extref_name_len (leaf, (void *)leaf_offset); +case BTRFS_ROOT_REF_KEY : +case BTRFS_ROOT_BACKREF_KEY : +return btrfs_root_ref_name_len (leaf, (void *)leaf_offset); + } +WARN_ON (1 ); +return 0 ; +} + +static u16 get_header_datalen (struct extent_buffer *leaf,u8 type, +unsigned long header_offset) +{ +/* Same as get_header_namelen */ +unsigned long leaf_offset = header_offset +BTRFS_LEAF_DATA_OFFSET ; + +switch (type) { +case BTRFS_DIR_ITEM_KEY : +case BTRFS_DIR_INDEX_KEY : +case BTRFS_XATTR_ITEM_KEY : +return btrfs_dir_data_len (leaf, (void *)leaf_offset); + } +return 0 ; +} + +/* +* For items with variable length, normally with namelen and tailing data. +* Like INODE_REF or XATTR +*/ Quoted 8 lines
fs/btrfs/tree-checker.c static int check_csum_item(struct btrfs_root *root, struct extent_buffer *leaf, · line 293
Quoted 4 lines
fs/btrfs/tree-checker.c +4
-0+static int check_variable_length_item (struct btrfs_root *root, +struct extent_buffer *leaf, +struct btrfs_key *key,int slot) +{One more thing - you are only validating the boundaries of such variable length items, so make this specific. I.e rename the function to something like:
If you follow the naming schema in tree-checker, you'll find that we (at least myself) is naming these function always using "
check_" prefix, so "validate_" prefix seems less consistent.Further more, the naming has its level"
leaf (all items boundary already checked)
|-leaf_item(hub wrapper)
|-csum_item
|-extent_data_itemSo here check_<something>_item follows the original level according to the naming schema.
Further more, although we're checking boundary, the truth is, we are check the name_len/data_len *members*, so the check_<something>_item still makes sence.
If really want to change the name, I prefer some naming can show that we're checking several items which share the same variable length property.
Quoted 7 lines
Quoted 1 line
So at least, none of the alternative seems to fit the schema.
The reason why I wanted this function renamed is that we have this function which performs only some checks and then we have the
verify_dir_itemwhich performs different checks for the same item. So why can't those function be turned into one ? I'm not too hung up on the actual naming!
Well, this makes sense.
I should merge them up.
Since verify_dir_item() just do extra check on types, they can be easily merged.
The original plan is to have check_<some extra item> under check_variable_length_item() if needed.
Considering only type and maximum name len need to be addded, it's quite an easy work.
I'll add them in next update. (With extra possible checks).
Thanks,
Qu
Quoted 81 lines
Quoted 81 lines
Thanks,
QuQuoted 75 lines
validate_variable_boundariescheck_variable_length_item_boundarycheck_item_boundariesQuoted 55 lines
fs/btrfs/tree-checker.c +47
-0+u8 type = key->type; +u32 item_start =btrfs_item_offset_nr (leaf, slot); +u32 item_end =btrfs_item_end_nr (leaf, slot); +u32 header_size =get_header_size (type); +u32 total_size; +u32 cur = item_start; + +while (cur < item_end) { +u32 namelen; +u32 datalen; + +/* header itself should not cross item boundary */ +if (cur + header_size > item_end) { +generic_err (root, leaf, slot, +"structure header crosses item boundary, have %u expect (%u, %u]" , + cur + header_size, cur, item_end); +return -EUCLEAN ; + } + + namelen =get_header_namelen (leaf, type, cur); + datalen =get_header_datalen (leaf, type, cur); + +/* Only XATTR can own data */ +if (type !=BTRFS_XATTR_ITEM_KEY && datalen) { +generic_err (root, leaf, slot, +"item has invalid data len, have %u expect 0" , + datalen); +return -EUCLEAN ; + } + + total_size = header_size + namelen + datalen; + +/* header and name/data should not cross item boundary */ +if (cur + total_size > item_end) { +generic_err (root, leaf, slot, +"structure data crosses item boundary, have %u expect (%u, %u]" , + cur + total_size, cur + header_size, item_end); +return -EUCLEAN ; + } + + cur += total_size; + } +return 0 ; +} +/* * Common point to switch the item-specific validation. */ @@ -238,6 +352,15 @@ static int check_leaf_item(struct btrfs_root *root,case BTRFS_EXTENT_CSUM_KEY : ret =check_csum_item (root, leaf, key, slot);break ; +case BTRFS_DIR_ITEM_KEY : +case BTRFS_XATTR_ITEM_KEY :fs/btrfs/tree-checker.c static int check_leaf_item(struct btrfs_root *root, · line 366
Quoted 10 lines
fs/btrfs/tree-checker.c +7
-0+case BTRFS_DIR_INDEX_KEY : +case BTRFS_INODE_REF_KEY : +case BTRFS_INODE_EXTREF_KEY : +case BTRFS_ROOT_REF_KEY : +case BTRFS_ROOT_BACKREF_KEY : + ret =check_variable_length_item (root, leaf, key, slot); +break ; }return ret; }--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html