| Message ID | 1434585553-8697-1-git-send-email-robbelibobban@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 && -- 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 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 -- 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 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 && -- 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, 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. -- 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/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 &&
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(+)