[PATCH] check: check so offset is not bigger then the leaf

[PATCH] check: check so offset is not bigger then the leaf

[Robert Marklund]

This could crash before because of dangerous dangling
offset of pointer.

Signed-off-by: Robert Marklund <robbelibobban@gmail.com>
---
 cmds-check.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/cmds-check.c b/cmds-check.c
index 778f141..da36758 100644
--- a/cmds-check.c
+++ b/cmds-check.c
@@ -8906,6 +8906,16 @@ static int build_roots_info_cache(struct btrfs_fs_info *info)
 			goto next;
 
 		ei = btrfs_item_ptr(leaf, slot, struct btrfs_extent_item);
+
+		if ((long long)ei > info->extent_root->leafsize) {
+			 fprintf(stderr, "Bad leaf = %p, slot = %d\n", leaf, slot);
+			 fprintf(stderr, "item ptr = %p\n", ei);
+			 fprintf(stderr, "objectid = %llx\n", found_key.objectid);
+			 fprintf(stderr, "type     = %x\n", found_key.type);
+			 fprintf(stderr, "offset   = %llx\n", found_key.offset);
+			 goto next;
+		}
+
 		flags = btrfs_extent_flags(leaf, ei);
 
 		if (found_key.type == BTRFS_EXTENT_ITEM_KEY &&
-- 
2.1.0

Re: [PATCH] check: check so offset is not bigger then the leaf

[David Sterba]

On Thu, Jun 18, 2015 at 01:59:13AM +0200, Robert Marklund wrote:
> This could crash before because of dangerous dangling
> offset of pointer.

That's right, this can happen. There are more btrfs_item_ptr that would
be good to validate that way, namely in the checker as it's most likely
to see corrupted data.

I think it's worth to add a wrapper macro for that, that would be like

(int) btrfs_item_ptr_validate(ei, leaf, slot, struct ..., *optional_key)

and return 0 if it's ok, 1 if there's a problem and prints the details.

> Signed-off-by: Robert Marklund <robbelibobban@gmail.com>
> ---
>  cmds-check.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/cmds-check.c b/cmds-check.c
> index 778f141..da36758 100644
> --- a/cmds-check.c
> +++ b/cmds-check.c
> @@ -8906,6 +8906,16 @@ static int build_roots_info_cache(struct btrfs_fs_info *info)
>  			goto next;
>  
>  		ei = btrfs_item_ptr(leaf, slot, struct btrfs_extent_item);
> +
> +		if ((long long)ei > info->extent_root->leafsize) {
> +			 fprintf(stderr, "Bad leaf = %p, slot = %d\n", leaf, slot);
> +			 fprintf(stderr, "item ptr = %p\n", ei);
> +			 fprintf(stderr, "objectid = %llx\n", found_key.objectid);
> +			 fprintf(stderr, "type     = %x\n", found_key.type);
> +			 fprintf(stderr, "offset   = %llx\n", found_key.offset);

Hm, I'm not sure whether to continue or fail at this point.

Do you have a crafted filesystem image that can reproduce that or was
that found by code inspection?

> +			 goto next;
> +		}
> +
>  		flags = btrfs_extent_flags(leaf, ei);
>  
>  		if (found_key.type == BTRFS_EXTENT_ITEM_KEY &&

Re: [PATCH] check: check so offset is not bigger then the leaf

[Josef Bacik]

On 06/18/2015 09:44 AM, David Sterba wrote:
> On Thu, Jun 18, 2015 at 01:59:13AM +0200, Robert Marklund wrote:
>> This could crash before because of dangerous dangling
>> offset of pointer.
>
> That's right, this can happen. There are more btrfs_item_ptr that would
> be good to validate that way, namely in the checker as it's most likely
> to see corrupted data.
>

The check_block stuff should be doing this, if it isn't that's where we 
need to fix it.  Thanks,

Josef

Re: [PATCH] check: check so offset is not bigger then the leaf

[David Sterba]

On Thu, Jun 18, 2015 at 10:16:54AM -0700, Josef Bacik wrote:
> On 06/18/2015 09:44 AM, David Sterba wrote:
> > On Thu, Jun 18, 2015 at 01:59:13AM +0200, Robert Marklund wrote:
> >> This could crash before because of dangerous dangling
> >> offset of pointer.
> >
> > That's right, this can happen. There are more btrfs_item_ptr that would
> > be good to validate that way, namely in the checker as it's most likely
> > to see corrupted data.
> >
> 
> The check_block stuff should be doing this, if it isn't that's where we 
> need to fix it.  Thanks,

Something like that?

--- a/ctree.c
+++ b/ctree.c
@@ -521,6 +521,19 @@ btrfs_check_leaf(struct btrfs_root *root, struct btrfs_disk_key *parent_key,
                        goto fail;
                }
        }
+
+       for (i = 0; i < nritems; i++) {
+               void *tmp;
+
+               tmp = btrfs_item_ptr(buf, i, void);
+               if ((long)tmp >= BTRFS_LEAF_DATA_SIZE(root)) {
+                       ret = BTRFS_TREE_BLOCK_INVALID_OFFSETS;
+                       fprintf(stderr, "bad item pointer %lu\n",
+                               (long)tmp);
+                       goto fail;
+               }
+       }
+
        return BTRFS_TREE_BLOCK_CLEAN;
 fail:
        if (btrfs_header_owner(buf) == BTRFS_EXTENT_TREE_OBJECTID) {
---

Compile-tested only.

Re: [PATCH] check: check so offset is not bigger then the leaf

[Josef Bacik]

On 06/25/2015 09:06 AM, David Sterba wrote:
> On Thu, Jun 18, 2015 at 10:16:54AM -0700, Josef Bacik wrote:
>> On 06/18/2015 09:44 AM, David Sterba wrote:
>>> On Thu, Jun 18, 2015 at 01:59:13AM +0200, Robert Marklund wrote:
>>>> This could crash before because of dangerous dangling
>>>> offset of pointer.
>>>
>>> That's right, this can happen. There are more btrfs_item_ptr that would
>>> be good to validate that way, namely in the checker as it's most likely
>>> to see corrupted data.
>>>
>>
>> The check_block stuff should be doing this, if it isn't that's where we
>> need to fix it.  Thanks,
>
> Something like that?
>
> --- a/ctree.c
> +++ b/ctree.c
> @@ -521,6 +521,19 @@ btrfs_check_leaf(struct btrfs_root *root, struct btrfs_disk_key *parent_key,
>                          goto fail;
>                  }
>          }
> +
> +       for (i = 0; i < nritems; i++) {
> +               void *tmp;
> +
> +               tmp = btrfs_item_ptr(buf, i, void);
> +               if ((long)tmp >= BTRFS_LEAF_DATA_SIZE(root)) {
> +                       ret = BTRFS_TREE_BLOCK_INVALID_OFFSETS;
> +                       fprintf(stderr, "bad item pointer %lu\n",
> +                               (long)tmp);
> +                       goto fail;
> +               }
> +       }

I'd just do

if (btrfs_item_end_nr(buf, i) >= BTRFS_LEAF_DATA_SIZE(root))

that way you catch problems with offset and size.  Thanks,

Josef

Re: [PATCH] check: check so offset is not bigger then the leaf

[David Sterba]

On Thu, Jun 25, 2015 at 09:24:10AM -0700, Josef Bacik wrote:
> > +
> > +       for (i = 0; i < nritems; i++) {
> > +               void *tmp;
> > +
> > +               tmp = btrfs_item_ptr(buf, i, void);
> > +               if ((long)tmp >= BTRFS_LEAF_DATA_SIZE(root)) {
> > +                       ret = BTRFS_TREE_BLOCK_INVALID_OFFSETS;
> > +                       fprintf(stderr, "bad item pointer %lu\n",
> > +                               (long)tmp);
> > +                       goto fail;
> > +               }
> > +       }
> 
> I'd just do
> 
> if (btrfs_item_end_nr(buf, i) >= BTRFS_LEAF_DATA_SIZE(root))
> 
> that way you catch problems with offset and size.  Thanks,

Ah right, my check would not catch 'offset + size >= leaf data size'
if 'offset < leaf data size'. Patch welcome.

Re: [PATCH] check: check so offset is not bigger then the leaf

[Trollkarlen Marklund]

> On 18 Jun 2015, at 19:44, David Sterba <dsterba@suse.cz> wrote:
> 
> On Thu, Jun 18, 2015 at 01:59:13AM +0200, Robert Marklund wrote:
>> This could crash before because of dangerous dangling
>> offset of pointer.
> 
> That's right, this can happen. There are more btrfs_item_ptr that would
> be good to validate that way, namely in the checker as it's most likely
> to see corrupted data.
> 
> I think it's worth to add a wrapper macro for that, that would be like
> 
> (int) btrfs_item_ptr_validate(ei, leaf, slot, struct ..., *optional_key)
> 
> and return 0 if it's ok, 1 if there's a problem and prints the details.
> 
>> Signed-off-by: Robert Marklund <robbelibobban@gmail.com>
>> ---
>> cmds-check.c | 10 ++++++++++
>> 1 file changed, 10 insertions(+)
>> 
>> diff --git a/cmds-check.c b/cmds-check.c
>> index 778f141..da36758 100644
>> --- a/cmds-check.c
>> +++ b/cmds-check.c
>> @@ -8906,6 +8906,16 @@ static int build_roots_info_cache(struct btrfs_fs_info *info)
>> 			goto next;
>> 
>> 		ei = btrfs_item_ptr(leaf, slot, struct btrfs_extent_item);
>> +
>> +		if ((long long)ei > info->extent_root->leafsize) {
>> +			 fprintf(stderr, "Bad leaf = %p, slot = %d\n", leaf, slot);
>> +			 fprintf(stderr, "item ptr = %p\n", ei);
>> +			 fprintf(stderr, "objectid = %llx\n", found_key.objectid);
>> +			 fprintf(stderr, "type     = %x\n", found_key.type);
>> +			 fprintf(stderr, "offset   = %llx\n", found_key.offset);
> 
> Hm, I'm not sure whether to continue or fail at this point.
> 

Im not either :)
But for me its better to keep trying until you hot the wall for real.


> Do you have a crafted filesystem image that can reproduce that or was
> that found by code inspection?

I have a failed filesystem caused by a failing disk that I tried to fix/recover.
Then i stumbled on this, and later on on some more places other then this.
Ill submit that also and in a nicer way when my filesystem is rescued. 
 

> 
>> +			 goto next;
>> +		}
>> +
>> 		flags = btrfs_extent_flags(leaf, ei);
>> 
>> 		if (found_key.type == BTRFS_EXTENT_ITEM_KEY &&

Re: [PATCH] check: check so offset is not bigger then the leaf

[David Sterba]

On Mon, Jun 29, 2015 at 02:16:28PM +0300, Trollkarlen Marklund wrote:
> > Do you have a crafted filesystem image that can reproduce that or was
> > that found by code inspection?
> 
> I have a failed filesystem caused by a failing disk that I tried to fix/recover.
> Then i stumbled on this, and later on on some more places other then this.
> Ill submit that also and in a nicer way when my filesystem is rescued. 

FYI, I've committed a patch based on the discussion in the thread.