Re: [PATCH] check: check so offset is not bigger then the leaf
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] check: check so offset is not bigger then the leaf
On 18 Jun 2015, at 19:44, David Sterba dste...@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 robbelibob...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] check: check so offset is not bigger then the leaf
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.
--
To unsubscribe from this list: send the line unsubscribe linux-btrfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] check: check so offset is not bigger then the leaf
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] check: check so offset is not bigger then the leaf
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] check: check so offset is not bigger then the leaf
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 robbelibob...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] check: check so offset is not bigger then the leaf
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] check: check so offset is not bigger then the leaf
This could crash before because of dangerous dangling
offset of pointer.
Signed-off-by: Robert Marklund robbelibob...@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
--
To unsubscribe from this list: send the line unsubscribe linux-btrfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
