check: check so offset is not bigger then the leaf

Commit Message

David Sterba June 25, 2015, 4:06 p.m. UTC

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?

---

Compile-tested only.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Josef Bacik June 25, 2015, 4:24 p.m. UTC | #1

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
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

David Sterba June 25, 2015, 4:49 p.m. UTC | #2

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

--- 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) {