| Message ID | 0279bccaf02bbc09d6ac685b37e36aacb60bf9b0.1717476533.git.wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs-progs: print-tree: do sanity checks for dir items | expand |
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
在 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 >
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
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),
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(+)