* [PATCH] check: check so offset is not bigger then the leaf @ 2015-06-17 23:59 Robert Marklund 2015-06-18 16:44 ` David Sterba 0 siblings, 1 reply; 8+ messages in thread From: Robert Marklund @ 2015-06-17 23:59 UTC (permalink / raw) To: linux-btrfs; +Cc: 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] check: check so offset is not bigger then the leaf 2015-06-17 23:59 [PATCH] check: check so offset is not bigger then the leaf Robert Marklund @ 2015-06-18 16:44 ` David Sterba 2015-06-18 17:16 ` Josef Bacik 2015-06-29 11:16 ` Trollkarlen Marklund 0 siblings, 2 replies; 8+ messages in thread From: David Sterba @ 2015-06-18 16:44 UTC (permalink / raw) To: Robert Marklund; +Cc: linux-btrfs 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 && ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] check: check so offset is not bigger then the leaf 2015-06-18 16:44 ` David Sterba @ 2015-06-18 17:16 ` Josef Bacik 2015-06-25 16:06 ` David Sterba 2015-06-29 11:16 ` Trollkarlen Marklund 1 sibling, 1 reply; 8+ messages in thread From: Josef Bacik @ 2015-06-18 17:16 UTC (permalink / raw) To: dsterba, Robert Marklund, linux-btrfs 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] check: check so offset is not bigger then the leaf 2015-06-18 17:16 ` Josef Bacik @ 2015-06-25 16:06 ` David Sterba 2015-06-25 16:24 ` Josef Bacik 0 siblings, 1 reply; 8+ messages in thread From: David Sterba @ 2015-06-25 16:06 UTC (permalink / raw) To: Josef Bacik; +Cc: dsterba, Robert Marklund, linux-btrfs 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] check: check so offset is not bigger then the leaf 2015-06-25 16:06 ` David Sterba @ 2015-06-25 16:24 ` Josef Bacik 2015-06-25 16:49 ` David Sterba 0 siblings, 1 reply; 8+ messages in thread From: Josef Bacik @ 2015-06-25 16:24 UTC (permalink / raw) To: dsterba, Robert Marklund, linux-btrfs 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] check: check so offset is not bigger then the leaf 2015-06-25 16:24 ` Josef Bacik @ 2015-06-25 16:49 ` David Sterba 0 siblings, 0 replies; 8+ messages in thread From: David Sterba @ 2015-06-25 16:49 UTC (permalink / raw) To: Josef Bacik; +Cc: dsterba, Robert Marklund, linux-btrfs 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] check: check so offset is not bigger then the leaf 2015-06-18 16:44 ` David Sterba 2015-06-18 17:16 ` Josef Bacik @ 2015-06-29 11:16 ` Trollkarlen Marklund 2015-07-01 13:26 ` David Sterba 1 sibling, 1 reply; 8+ messages in thread From: Trollkarlen Marklund @ 2015-06-29 11:16 UTC (permalink / raw) To: dsterba; +Cc: linux-btrfs > 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 && ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] check: check so offset is not bigger then the leaf 2015-06-29 11:16 ` Trollkarlen Marklund @ 2015-07-01 13:26 ` David Sterba 0 siblings, 0 replies; 8+ messages in thread From: David Sterba @ 2015-07-01 13:26 UTC (permalink / raw) To: Trollkarlen Marklund; +Cc: dsterba, linux-btrfs 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2015-07-01 13:26 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-06-17 23:59 [PATCH] check: check so offset is not bigger then the leaf Robert Marklund 2015-06-18 16:44 ` David Sterba 2015-06-18 17:16 ` Josef Bacik 2015-06-25 16:06 ` David Sterba 2015-06-25 16:24 ` Josef Bacik 2015-06-25 16:49 ` David Sterba 2015-06-29 11:16 ` Trollkarlen Marklund 2015-07-01 13:26 ` David Sterba
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.