[PATCH] btrfs-progs: print-tree: do sanity checks for dir items

[PATCH] btrfs-progs: print-tree: do sanity checks for dir items

[Qu Wenruo]

There is a bug report that with UBSAN enabled, fuzz/006 test case would
crash.

It turns out that the image bko-154021-invalid-drop-level.raw has
invalid dir items, that the name/data len is beyond the item.

And if we try to read beyond the eb boundary, UBSAN got triggered.

Normally in kernel tree-checker would reject such metadata in the first
place, but in btrfs-progs we can not go that strict or we can not do a
lot of repair.

So here just enhance print_dir_item() to do extra sanity checks for
data/name len before reading the contents.

Issue: #805
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 kernel-shared/print-tree.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/kernel-shared/print-tree.c b/kernel-shared/print-tree.c
index 1b9386d87a0a..9a72ba39b426 100644
--- a/kernel-shared/print-tree.c
+++ b/kernel-shared/print-tree.c
@@ -78,6 +78,11 @@ static void print_dir_item(struct extent_buffer *eb, u32 size,
 		printf("\n");
 		name_len = btrfs_dir_name_len(eb, di);
 		data_len = btrfs_dir_data_len(eb, di);
+		if (data_len + name_len + cur > size) {
+			error("invalid length, cur=%u name_len=%u data_len=%u size=%u\n",
+				cur, name_len, data_len, size);
+			break;
+		}
 		len = (name_len <= sizeof(namebuf))? name_len: sizeof(namebuf);
 		printf("\t\ttransid %llu data_len %u name_len %u\n",
 				btrfs_dir_transid(eb, di),
-- 
2.45.2

Re: [PATCH] btrfs-progs: print-tree: do sanity checks for dir items

[Josef Bacik]

On Tue, Jun 04, 2024 at 02:19:08PM +0930, Qu Wenruo wrote:
> There is a bug report that with UBSAN enabled, fuzz/006 test case would
> crash.
> 
> It turns out that the image bko-154021-invalid-drop-level.raw has
> invalid dir items, that the name/data len is beyond the item.
> 
> And if we try to read beyond the eb boundary, UBSAN got triggered.
> 
> Normally in kernel tree-checker would reject such metadata in the first
> place, but in btrfs-progs we can not go that strict or we can not do a
> lot of repair.
> 
> So here just enhance print_dir_item() to do extra sanity checks for
> data/name len before reading the contents.
> 
> Issue: #805
> Signed-off-by: Qu Wenruo <wqu@suse.com>

I'd rather not duplicate this check.

Is the print-tree coming from repair?  If that's the case then I'd manually call
check_leaf to make sure the pointers are all correct before calling print tree,
otherwise if it's from a different tool we need to make sure the strict checking
is happening for that tool, we should only be bypassing the strict checking for
repair.  Thanks,

Josef

Re: [PATCH] btrfs-progs: print-tree: do sanity checks for dir items

[Qu Wenruo]

在 2024/6/5 01:28, Josef Bacik 写道:
> On Tue, Jun 04, 2024 at 02:19:08PM +0930, Qu Wenruo wrote:
>> There is a bug report that with UBSAN enabled, fuzz/006 test case would
>> crash.
>>
>> It turns out that the image bko-154021-invalid-drop-level.raw has
>> invalid dir items, that the name/data len is beyond the item.
>>
>> And if we try to read beyond the eb boundary, UBSAN got triggered.
>>
>> Normally in kernel tree-checker would reject such metadata in the first
>> place, but in btrfs-progs we can not go that strict or we can not do a
>> lot of repair.
>>
>> So here just enhance print_dir_item() to do extra sanity checks for
>> data/name len before reading the contents.
>>
>> Issue: #805
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>
> I'd rather not duplicate this check.
>
> Is the print-tree coming from repair?

Nope, it's "inspect tree-stats" subcommand, it's just really printing
the root of each tree.

Remember we still need to call "btrfs ins dump-tree" to analyze fuzzed
images, if the print-tree is as strict as tree-checker, just image how
miserable the life would be.

Thanks,
Qu

>  If that's the case then I'd manually call
> check_leaf to make sure the pointers are all correct before calling print tree,
> otherwise if it's from a different tool we need to make sure the strict checking
> is happening for that tool, we should only be bypassing the strict checking for
> repair.  Thanks,
>
> Josef
>

Re: [PATCH] btrfs-progs: print-tree: do sanity checks for dir items

[Josef Bacik]

On Wed, Jun 05, 2024 at 07:46:49AM +0930, Qu Wenruo wrote:
> 
> 
> 在 2024/6/5 01:28, Josef Bacik 写道:
> > On Tue, Jun 04, 2024 at 02:19:08PM +0930, Qu Wenruo wrote:
> > > There is a bug report that with UBSAN enabled, fuzz/006 test case would
> > > crash.
> > > 
> > > It turns out that the image bko-154021-invalid-drop-level.raw has
> > > invalid dir items, that the name/data len is beyond the item.
> > > 
> > > And if we try to read beyond the eb boundary, UBSAN got triggered.
> > > 
> > > Normally in kernel tree-checker would reject such metadata in the first
> > > place, but in btrfs-progs we can not go that strict or we can not do a
> > > lot of repair.
> > > 
> > > So here just enhance print_dir_item() to do extra sanity checks for
> > > data/name len before reading the contents.
> > > 
> > > Issue: #805
> > > Signed-off-by: Qu Wenruo <wqu@suse.com>
> > 
> > I'd rather not duplicate this check.
> > 
> > Is the print-tree coming from repair?
> 
> Nope, it's "inspect tree-stats" subcommand, it's just really printing
> the root of each tree.
> 
> Remember we still need to call "btrfs ins dump-tree" to analyze fuzzed
> images, if the print-tree is as strict as tree-checker, just image how
> miserable the life would be.
> 

Ugh good point, ok

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef